Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Suppress capturing responses #473

Merged

Conversation

gregschohn
Copy link
Collaborator

@gregschohn gregschohn commented Dec 12, 2023

Description

This is the successor to dropping captures for requests. It builds upon some of the refactoring that was done for the instrumentation work.

  • Category: Bug fix, Enhancement
  • Why these changes are required? To suppress capturing response packets when the requests weren't captured.
  • What is the old behavior before changes and new behavior after changes? Spurious response packets would have followed the dropped observation, or if none of the request was present, they could have followed the last request/response, which would have resulted in invalid looking responses from the source cluster (to the point where it could have been impossible to determine what the responses actually were).

Testing

[Please provide details of testing done: unit testing, integration testing and manual testing]

Check List

  • New functionality includes testing
    • All tests pass, including unit test, integration test and doctest
  • New functionality has been documented
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

…thod for others to leverage.

Signed-off-by: Greg Schohn <[email protected]>
Most of this is just playing, but making the StreamManager implement AutoCloseable gives a place to end spans to show how long a serializer/connection factory was relevant for.

Signed-off-by: Greg Schohn <[email protected]>
…to the collector to prometheus, zipkin, etc

Signed-off-by: Greg Schohn <[email protected]>
Signed-off-by: Greg Schohn <[email protected]>
…lotted within the same graph in prometheus.

Signed-off-by: Greg Schohn <[email protected]>
…ics into an optional.

Dropping the optionals makes the code simpler and if we don't want to do logging, we can just not fill in the configuration for the SDK.

Signed-off-by: Greg Schohn <[email protected]>
…troduce some more typesafe wrappers for contexts.

Lots more to come.

Signed-off-by: Greg Schohn <[email protected]>
…explicitly passing strongly typed context objects.

Signed-off-by: Greg Schohn <[email protected]>
Signed-off-by: Greg Schohn <[email protected]>
Make sure that the context is using the right requestKey, which also will have the appropriate indices as per the test context.

Signed-off-by: Greg Schohn <[email protected]>
…ayerRequestContext to the replayer

Signed-off-by: Greg Schohn <[email protected]>
Don't bother showing the Kakfa offloader just buffering (was called recordStream).  Now the offloader span is a child span of the connection span from the handler, so we can see the handler gathering the request/response (or waiting for the response).

Signed-off-by: Greg Schohn <[email protected]>
That makes it a separate state for the logging handler superclass.

Signed-off-by: Greg Schohn <[email protected]>
…rocessor.

Prometheus metrics already have an export_name that is unique, the processors weren't doing anything useful, & the namespace was appending EVERYTHING from one of the two services.

Signed-off-by: Greg Schohn <[email protected]>
…d (less so for now) metrics can be exported across more of the lifetime of a request/connection.

Signed-off-by: Greg Schohn <[email protected]>
Signed-off-by: Greg Schohn <[email protected]>

# Conflicts:
#	TrafficCapture/nettyWireLogging/src/main/java/org/opensearch/migrations/trafficcapture/netty/ConditionallyReliableLoggingHttpRequestHandler.java
#	TrafficCapture/nettyWireLogging/src/main/java/org/opensearch/migrations/trafficcapture/netty/LoggingHttpRequestHandler.java
#	TrafficCapture/nettyWireLogging/src/test/java/org/opensearch/migrations/trafficcapture/netty/ConditionallyReliableLoggingHttpRequestHandlerTest.java
#	TrafficCapture/trafficCaptureProxyServer/src/main/java/org/opensearch/migrations/trafficcapture/proxyserver/netty/ProxyChannelInitializer.java
#	TrafficCapture/trafficReplayer/src/main/java/org/opensearch/migrations/replay/Accumulation.java
#	TrafficCapture/trafficReplayer/src/main/java/org/opensearch/migrations/replay/CapturedTrafficToHttpTransactionAccumulator.java
#	TrafficCapture/trafficReplayer/src/main/java/org/opensearch/migrations/replay/RequestResponsePacketPair.java
#	TrafficCapture/trafficReplayer/src/main/java/org/opensearch/migrations/replay/RequestSenderOrchestrator.java
#	TrafficCapture/trafficReplayer/src/test/java/org/opensearch/migrations/replay/SimpleCapturedTrafficToHttpTransactionAccumulatorTest.java
…when the request was ignored and remove the now-unused file for responses.

I'd like to revisit this eventually to make sure that it's as efficient as possible and to organize it better.  However, it does get the job done for now and tests were updated to confirm.

Signed-off-by: Greg Schohn <[email protected]>
Copy link

codecov bot commented Dec 12, 2023

Codecov Report

Attention: 127 lines in your changes are missing coverage. Please review.

Comparison is base (4ee54ef) 73.57% compared to head (7de4009) 74.48%.

Files Patch % Lines
...arch/migrations/tracing/SimpleMeteringClosure.java 40.47% 47 Missing and 3 partials ⚠️
...tions/trafficcapture/netty/LoggingHttpHandler.java 84.71% 15 Missing and 9 partials ⚠️
...atahandlers/http/HttpJsonTransformingConsumer.java 72.41% 7 Missing and 1 partial ⚠️
...ions/trafficcapture/tracing/ConnectionContext.java 56.25% 7 Missing ⚠️
...netty/ConditionallyReliableLoggingHttpHandler.java 53.33% 1 Missing and 6 partials ⚠️
...ficcapture/kafkaoffloader/KafkaCaptureFactory.java 84.21% 2 Missing and 4 partials ⚠️
...afficcapture/netty/RequestContextStateMachine.java 0.00% 6 Missing ⚠️
...tions/trafficcapture/proxyserver/CaptureProxy.java 0.00% 3 Missing ⚠️
.../opensearch/migrations/replay/TrafficReplayer.java 66.66% 1 Missing and 2 partials ⚠️
...ure/kafkaoffloader/tracing/KafkaRecordContext.java 90.47% 2 Missing ⚠️
... and 8 more
Additional details and impacted files
@@             Coverage Diff              @@
##               main     #473      +/-   ##
============================================
+ Coverage     73.57%   74.48%   +0.90%     
- Complexity     1181     1271      +90     
============================================
  Files           124      135      +11     
  Lines          4886     5162     +276     
  Branches        439      458      +19     
============================================
+ Hits           3595     3845     +250     
- Misses          996     1010      +14     
- Partials        295      307      +12     
Flag Coverage Δ
unittests 74.48% <77.64%> (+0.90%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Comment on lines +95 to +97
if (captureState.captureIgnoreState == CaptureIgnoreState.IGNORE_RESPONSE) {
captureState.captureIgnoreState = CaptureIgnoreState.CAPTURE;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trying to understand this... If the state is to ignore response and we have a read we will set to capture. Is that because reads in this case will only be for request packets and we can reset back to capture or is something else happening here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default assumption is that all requests and their responses will be captured until a request is marked to IGNORE_REQUEST. This is a reset since a new request has begun... and yes, reads are only for requests and writes are only for responses and that's why we now have two states for ignore.

@gregschohn gregschohn merged commit 7de4009 into opensearch-project:main Feb 9, 2024
6 of 7 checks passed
@gregschohn gregschohn deleted the SuppressCapturingResponses branch February 16, 2024 21:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants