-
Notifications
You must be signed in to change notification settings - Fork 759
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 instrumentation in Zipkin exporter #1150
Suppress instrumentation in Zipkin exporter #1150
Conversation
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.
test/OpenTelemetry.Exporter.Zipkin.Tests/ZipkinExporterTests.cs
Outdated
Show resolved
Hide resolved
There is a bug, but I think it is in the Ah, found it. The ZipkinExportTests adds its own listener: opentelemetry-dotnet/test/OpenTelemetry.Exporter.Zipkin.Tests/ZipkinExporterTests.cs Lines 47 to 54 in f6ef67b
Ok, what I learned from this is that I think our Is this the expected behavior from the .NET side? That is, if:
|
Codecov Report
@@ Coverage Diff @@
## master #1150 +/- ##
==========================================
+ Coverage 77.10% 77.41% +0.31%
==========================================
Files 219 219
Lines 6259 6253 -6
==========================================
+ Hits 4826 4841 +15
+ Misses 1433 1412 -21
|
@@ -68,7 +68,7 @@ static TracerProviderSdk() | |||
// Callback when Activity is started. | |||
ActivityStarted = (activity) => | |||
{ | |||
if (activity.IsAllDataRequested) | |||
if (!Sdk.SuppressInstrumentation && activity.IsAllDataRequested) |
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.
we shouldn't need this as we already prevented the activity from being created in the 1st place right?
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.
See the info I left about this above ⬆️. Basically, if any listener says 👍, then all listeners ActivityStarted
/ActivityStopped
gets fired. I posed the question of whether this is the expected behavior from .NET.
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.
Opened dotnet/runtime#41306 to validate whether this is expected or not.
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.
Great find @alanwest. I seem to remember seeing some discussion about that behavior somewhere and that is the correct behavior but let's see what they say.
test/OpenTelemetry.Exporter.Zipkin.Tests/ZipkinExporterTests.cs
Outdated
Show resolved
Hide resolved
test/OpenTelemetry.Exporter.Zipkin.Tests/ZipkinExporterTests.cs
Outdated
Show resolved
Hide resolved
var zipkinExporter = new ZipkinExporter(exporterOptions); | ||
var exportActivityProcessor = new BatchExportActivityProcessor(zipkinExporter, scheduledDelayMilliseconds: 1); | ||
|
||
var openTelemetrySdk = Sdk.CreateTracerProviderBuilder() |
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.
@cijothomas I guess we want to use traceProvider
everywhere?
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.
yes. I'll doing mass cleanup for other places.
test/OpenTelemetry.Exporter.Zipkin.Tests/ZipkinExporterTests.cs
Outdated
Show resolved
Hide resolved
…:alanwest/opentelemetry-dotnet into alanwest/zipkin-suppress-instrumentation
@alanwest sorry this got delayed and has conflict now :) PLease resolve and we are good to go. |
…uppress-instrumentation
@@ -44,32 +44,10 @@ public class HttpClientInstrumentationOptions | |||
/// </summary> | |||
public Func<HttpRequestMessage, bool> FilterFunc { get; set; } | |||
|
|||
internal static bool IsInternalUrl(Uri requestUri) |
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.
It was a long road but we finally got this removed! 🎊
internal bool EventFilter(string activityName, object arg1) | ||
{ | ||
if (TryParseHttpRequestMessage(activityName, arg1, out HttpRequestMessage requestMessage)) | ||
{ | ||
Uri requestUri; |
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.
@cijothomas @alanwest @eddynaka We could probably improve the perf on this now by changing the order of things.
internal bool EventFilter(string activityName, object arg1)
{
return (this.FilterFunc == null || !TryParseHttpRequestMessage(activityName, arg1, out HttpRequestMessage requestMessage))
? true
: this.FilterFunc(requestMessage);
}
Maybe also add the inline hint on this & TryParseHttpRequestMessage?
Changes
We had previously discussed that it should be up to the exporter author to deal with instrumentation suppression (see: #1098 (comment)). This PR suppresses instrumentation from Zipkin.
For significant contributions please make sure you have completed the following items: