-
Notifications
You must be signed in to change notification settings - Fork 770
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
Avoid accessing Id on activity start #2659
Avoid accessing Id on activity start #2659
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2659 +/- ##
==========================================
+ Coverage 82.89% 82.91% +0.01%
==========================================
Files 249 249
Lines 8705 8707 +2
==========================================
+ Hits 7216 7219 +3
+ Misses 1489 1488 -1
|
src/OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModule/AspNetTelemetryEventSource.cs
Outdated
Show resolved
Hide resolved
src/OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModule/AspNetTelemetryEventSource.cs
Outdated
Show resolved
Hide resolved
} | ||
else | ||
{ | ||
activityId = string.Concat("00-", activity.TraceId.ToHexString(), "-", activity.SpanId.ToHexString()); |
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.
Consider using one concat instead of two (might be good to to benchmark and see what's the actual perf/alloc difference).
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.
// * Summary *
BenchmarkDotNet=v0.13.1, OS=Windows 10.0.22000
Intel Core i7-8850H CPU 2.60GHz (Coffee Lake), 1 CPU, 12 logical and 6 physical cores
.NET SDK=6.0.100
[Host] : .NET 5.0.12 (5.0.1221.52207), X64 RyuJIT
DefaultJob : .NET 5.0.12 (5.0.1221.52207), X64 RyuJIT
Method | Mean | Error | StdDev |
---|---|---|---|
OneConcat | 66.48 ns | 1.381 ns | 2.455 ns |
TwoConcat | 60.69 ns | 1.014 ns | 0.792 ns |
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.
looks like two concat works better.
Co-authored-by: Reiley Yang <reyang@microsoft.com>
Co-authored-by: Reiley Yang <reyang@microsoft.com>
src/OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModule/AspNetTelemetryEventSource.cs
Outdated
Show resolved
Hide resolved
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.
Might worth a changelog update. |
src/OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModule/AspNetTelemetryEventSource.cs
Outdated
Show resolved
Hide resolved
@vishweshbankwar Please update the description to explain the impact of the associated issue in the HttpClient/GrpcClient instrumentation or any others using Propagators and leveraging Activity.Id. We were living with this issue for a loooon time until you found it - so could you open an issue to track improving the unit/functional tests. |
Workaround for:
dotnet/runtime#61857
Changes
In case of legacy activities (i.e. new Activity()), we run the sampler after the activity is started. When the sampler runs it updates the
ActivityTraceFlags
property based on the sampling result. However, updating the trace flags on activity does not get reflected onId
if theId
was accessed before sampler runs. This results is incorrect propagation when the propagator usesId
property to set thetraceparent
header. This PR prevents accessingId
before sampler runs by manually creating it using TraceId, SpanId and ActivityTraceFlags.For significant contributions please make sure you have completed the following items:
CHANGELOG.md
updated for non-trivial changes