-
Notifications
You must be signed in to change notification settings - Fork 762
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
Modify ASP.NET Core instrumentation to take advantage of sampling decision #1899
Modify ASP.NET Core instrumentation to take advantage of sampling decision #1899
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1899 +/- ##
=======================================
Coverage 84.41% 84.42%
=======================================
Files 188 188
Lines 6103 6105 +2
=======================================
+ Hits 5152 5154 +2
Misses 951 951
|
src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs
Outdated
Show resolved
Hide resolved
src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs
Outdated
Show resolved
Hide resolved
src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs
Outdated
Show resolved
Hide resolved
request = new HttpRequestMessage(HttpMethod.Get, "/api/GetChildActivityBaggageContext"); | ||
request.Headers.Add("Baggage", "key1=value1,key2=value2"); | ||
response = await client.SendAsync(request); | ||
var childActivityBaggageContext = JsonConvert.DeserializeObject<IReadOnlyDictionary<string, string>>(response.Content.ReadAsStringAsync().Result); |
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.
you can avoid the additional json parsing, by making the test app return 2 strings contaning traceparent and tracestate (activity.Id, activity.TraceState) respectively.
src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs
Show resolved
Hide resolved
// Test TraceContext Propagation | ||
var expectedTraceState = "rojo=1,congo=2"; | ||
var request = new HttpRequestMessage(HttpMethod.Get, "/api/GetChildActivityTraceContext"); | ||
request.Headers.Add("traceparent", $"00-{expectedTraceId}-{expectedParentSpanId}-01"); |
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.
portions of this test would pass by default, as asp.net creates activity from traceparent always, irrespective of sampling in Otel.
To ensure thorough test, use a custom propagator (with some header like Foo), and validate the scenarios. Also add cases where sending traceparent with "00" and "01" as sampling flags
…tpInListener.cs Co-authored-by: Cijo Thomas <cithomas@microsoft.com>
…tpInListener.cs Co-authored-by: Cijo Thomas <cithomas@microsoft.com>
src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs
Show resolved
Hide resolved
…or-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.
Lets add to changelog that FIlter/Enrich is only called if AllDataRequested is true.
…etCoreInstrumentation-For-SuppressInstrumentation
…entation-For-SuppressInstrumentation' into utpilla/Optimize-AspNetCoreInstrumentation-For-SuppressInstrumentation
Changes