-
Notifications
You must be signed in to change notification settings - Fork 840
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
supress zipkin exporters instrumentations #6552
supress zipkin exporters instrumentations #6552
Conversation
@@ -7,6 +7,7 @@ | |||
|
|||
import io.opentelemetry.api.metrics.MeterProvider; | |||
import io.opentelemetry.exporter.internal.ExporterMetrics; | |||
import io.opentelemetry.exporter.internal.InstrumentationUtil; |
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.
import io.opentelemetry.exporter.internal.InstrumentationUtil; | |
import io.opentelemetry.api.internal.InstrumentationUtil; |
Should be updated to use the new API introduced in #6546
1b67b2b
to
300712b
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6552 +/- ##
=========================================
Coverage 90.05% 90.06%
- Complexity 6275 6276 +1
=========================================
Files 697 697
Lines 18944 18949 +5
Branches 1858 1858
=========================================
+ Hits 17060 17066 +6
Misses 1307 1307
+ Partials 577 576 -1 ☔ View full report in Codecov by Sentry. |
for test what i would expect to see is a new test in ZipkinSpanExporterTest which weaves in the same code (maybe via copy paste) as tests done here Line 19 in 6fc1d21
let me know if other hints would help |
another hint is remember BytesMessageSender is a type you can wrap. so you can catch the suppression by customizing this in your test. ThIs would result in the same confidence as the existing okhttp test I mention, basically if you can read the suppression intent is done, the test is good. this is different than an end to end test but may be enough to close this out. |
.endpoint("https://localhost") | ||
.encoding(Encoding.PROTO3) | ||
.build()) { | ||
sender.send(Collections.singletonList(new byte[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.
I don't understand how this test is exercising the code added in this PR.
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.
I updated the test.
|
||
@Test | ||
void testSuppressInstrumentation() { | ||
AtomicBoolean suppressInstrumentation = |
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.
Maybe think of it a different way, make a fake sender, not a real one.
Then pass that to ZipkinSpanExporter ctor.
Then in a suppression closure, do the actual export
After the closure, you can verify sent() and suppressed()
make sense?
class SuppressCatchingSender extends BytesMessageSender.Base {
AtomicBoolean sent = new AtomicBoolean();
AtomicBoolean suppressed = new AtomicBoolean();
SuppressCatchingSender() {
super(Encoding.JSON);
}
@Override public int messageMaxBytes() {
return 1024;
}
@Override public void send(List<byte[]> encodedSpans) {
sent.set(true);
suppressed.set(InstrumentationUtil.shouldSuppressInstrumentation(Context.current()));
}
@Override public void close() throws IOException {
}
}
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.
Thank you very much for your help.
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.
LGTM (non-binding approve)
exporters/zipkin/src/test/java/io/opentelemetry/exporter/zipkin/ZipkinSpanExporterTest.java
Outdated
Show resolved
Hide resolved
exporters/zipkin/src/test/java/io/opentelemetry/exporter/zipkin/ZipkinSpanExporterTest.java
Show resolved
Hide resolved
5781ae1
to
46e05d6
Compare
draft PR for #5967