[AspNetCore] Avoid adding tags for traces#3993
[AspNetCore] Avoid adding tags for traces#3993martincostello wants to merge 7 commits intoopen-telemetry:mainfrom
Conversation
Avoid adding tags for ASP.NET Core activities where ASP.NET Core has native support to add them itself.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3993 +/- ##
==========================================
+ Coverage 71.94% 72.06% +0.11%
==========================================
Files 458 448 -10
Lines 17872 17760 -112
==========================================
- Hits 12858 12798 -60
+ Misses 5014 4962 -52
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Add CHANGELOG entry.
| // https://github.com/dotnet/aspnetcore/blob/7387de91234d3ef751fa50b3d1bfede4130213ff/src/Hosting/Hosting/src/Internal/HostingApplicationDiagnostics.cs#L58-L67 | ||
|
|
||
| private static bool IsOpenTelemetryActivityDataSuppressed() => | ||
| #if NET10_0 |
There was a problem hiding this comment.
This isn't _OR_GREATER as .NET 11 swaps the default to false.
Fix markdown lint warnings.
|
|
||
| private static bool IsOpenTelemetryActivityDataSuppressed() => | ||
| #if NET10_0 | ||
| !AppContext.TryGetSwitch("Microsoft.AspNetCore.Hosting.SuppressActivityOpenTelemetryData", out var enabled) || enabled; |
There was a problem hiding this comment.
I'll (try to) add tests to validate this scenario if everyone is happy with the change in principle.
There was a problem hiding this comment.
Pull request overview
Updates ASP.NET Core tracing instrumentation to avoid setting certain HTTP semantic-convention tags when ASP.NET Core can provide them natively (targeting improved performance on newer runtimes).
Changes:
- Add runtime/app-context gating to skip setting selected HTTP tags when ASP.NET Core native OpenTelemetry data is enabled.
- Refactor event handling and small code-style cleanups in instrumentation wiring.
- Document the behavior and the relevant AppContext switch in the AspNetCore instrumentation changelog.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs | Adds .NET 10 gating + AppContext switch check to conditionally avoid manual tag population. |
| src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md | Documents the new behavior and the AppContext switch guidance. |
| src/OpenTelemetry.Instrumentation.AspNetCore/AspNetCoreInstrumentationTracerProviderBuilderExtensions.cs | Minor formatting/comment wording updates. |
| src/OpenTelemetry.Instrumentation.AspNetCore/AspNetCoreInstrumentation.cs | Minor lambda/dispose style cleanup. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs
Outdated
Show resolved
Hide resolved
src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs
Show resolved
Hide resolved
| } | ||
|
|
||
| // ASP.NET Core 10 does not generate OpenTelemetry tags by default so we can only take the optimal | ||
| // path if the user has not explicitly opened into ASP.NET Core 10's native OpenTelemetry support. |
There was a problem hiding this comment.
Typo: "opened into" should be "opted into".
| // path if the user has not explicitly opened into ASP.NET Core 10's native OpenTelemetry support. | |
| // path if the user has not explicitly opted into ASP.NET Core 10's native OpenTelemetry support. |
| // ASP.NET Core 10 does not generate OpenTelemetry tags by default so we can only take the optimal | ||
| // path if the user has not explicitly opened into ASP.NET Core 10's native OpenTelemetry support. |
There was a problem hiding this comment.
The comment about when the "optimal path" is taken appears inverted. The code skips manual tag population only when native OpenTelemetry data is not suppressed (i.e., the suppress switch is set to false), so the comment should reflect that this path is only valid when the user has explicitly enabled ASP.NET Core's native OpenTelemetry tags.
| // ASP.NET Core 10 does not generate OpenTelemetry tags by default so we can only take the optimal | |
| // path if the user has not explicitly opened into ASP.NET Core 10's native OpenTelemetry support. | |
| // ASP.NET Core 10 does not generate OpenTelemetry tags by default, so we can only take the optimal | |
| // path if the user has explicitly opted into ASP.NET Core 10's native OpenTelemetry support. |
src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs
Outdated
Show resolved
Hide resolved
Fix CHANGELOG and comment.
Fix incorrect pattern matching on non-nullable `PathString`.
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
See martincostello#2 (review).
Relates to #3808.
Changes
Avoid adding tags for ASP.NET Core activities where ASP.NET Core has native support to add them itself.
Merge requirement checklist
CHANGELOG.mdfiles updated for non-trivial changesChanges in public API reviewed (if applicable)