-
Notifications
You must be signed in to change notification settings - Fork 867
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
Enable span suppression by SpanKey by default #5779
Conversation
ad3cf14
to
4e0df4d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR got a bit bigger than I expected it would be, but frankly ~half of it is just fixing the tests. The first commit contains the actual implementation and logic changes, the rest are just test/errorprone/spotless fixes.
...src/main/java/io/opentelemetry/instrumentation/api/instrumenter/SpanSuppressionStrategy.java
Show resolved
Hide resolved
...ation/aws-sdk/aws-sdk-1.11/javaagent-unit-tests/src/test/java/TracingRequestHandlerTest.java
Show resolved
Hide resolved
if (hasHttpClientSpan()) { | ||
offset = 1 | ||
span(1) { | ||
name "HTTP POST" | ||
kind CLIENT | ||
childOf span(0) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The AWS SQS.SendMessage
span implements both RPC and HTTP conventions, thus it suppresses this HTTP span -- they both had the exact same net/HTTP attributes anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@anuraaga this sounds like a good thing, just wanted to get your 👀 on it
...common/src/main/java/io/opentelemetry/instrumentation/testing/InstrumentationTestRunner.java
Show resolved
Hide resolved
66bee8a
to
e8abe95
Compare
...tion-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/SpanSuppressor.java
Outdated
Show resolved
Hide resolved
...tion-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/SpanSuppressor.java
Show resolved
Hide resolved
...tation-api-semconv/src/main/java/io/opentelemetry/instrumentation/api/server/ServerSpan.java
Show resolved
Hide resolved
...on-api/src/test/java/io/opentelemetry/instrumentation/api/instrumenter/InstrumenterTest.java
Outdated
Show resolved
Hide resolved
if (hasHttpClientSpan()) { | ||
offset = 1 | ||
span(1) { | ||
name "HTTP POST" | ||
kind CLIENT | ||
childOf span(0) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@anuraaga this sounds like a good thing, just wanted to get your 👀 on it
...ntelemetry-api/opentelemetry-api-1.0/testing/src/main/java/AgentSpanTestingInstrumenter.java
Outdated
Show resolved
Hide resolved
...agent/src/main/java/io/opentelemetry/javaagent/instrumentation/twilio/TwilioAsyncMarker.java
Show resolved
Hide resolved
"$SemanticAttributes.HTTP_URL.key" "https://api.twilio.com/2010-04-01/Accounts/abc/Messages.json" | ||
"$SemanticAttributes.HTTP_STATUS_CODE.key" 200 | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and we get the underlying http span 🎉
testing-common/src/main/java/io/opentelemetry/instrumentation/testing/TestInstrumenters.java
Show resolved
Hide resolved
...src/main/java/io/opentelemetry/instrumentation/api/instrumenter/SpanSuppressionStrategy.java
Show resolved
Hide resolved
e25c629
to
6c82d64
Compare
* Enable span suppression by SpanKey by default * fix HTTP tests (probably) * add exception for camel * remove suppression tests from @WithSpan instrumentations * remove suppression tests from @WithSpan instrumentation; spring boot autoconfigure * fix twilio tests * fix netty-based HTTP clients, remove AWS SDK 1.11 unit test * fix elasticsearch tests * codenarc * spotless * fix AWS SDK 1.11 tests * remove a TODO * code review comments * fix merge conflict Co-authored-by: Trask Stalnaker <[email protected]>
Resolves #5735 -- now suppression by
SpanKey
is the default suppression strategy; there is a configuration option that allows to choose the strategy.Resolves #3965 -- adds a new "none" mode that does not suppress spans at all.
This PR includes a significant refactoring of the span suppression code, and includes a couple of new
SpanKey
s.CC @lmolkova
(I'll mark it ready for review once I fix all the tests 🤞 )