-
Notifications
You must be signed in to change notification settings - Fork 95
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
Fix ASP.NET Core Traces Instrumentation for .NET7+ #3231
Conversation
As a follow up for this PR we should revisit strategy for loading other |
4bbc054
to
e855afe
Compare
Use semi-native support by Microsoft.AspNetCore
e855afe
to
7da059f
Compare
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, one small Q about the lib/<tfm >
for one assembly in the store for net7.0
apps.
test/IntegrationTests/BuildTests.DistributionStructure_alpine-linux.verified.txt
Outdated
Show resolved
Hide resolved
@@ -37,7 +37,11 @@ public void SubmitTraces(string propagators) | |||
return true; | |||
}); | |||
Span? serverSpan = null; | |||
#if NET7_0_OR_GREATER | |||
collector.Expect("Microsoft.AspNetCore", span => |
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.
Can we solve this in a way that we can always use the same instrumentation source?
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 is not "our" decision. The difference comes from the (semi)native support for ASP.NET Core instrumentation in net7+ and from "OpenTelemetry.Instrumentation.AspNetCore" package.
I do not see possibility to have the same source for both versions. BTW same case is for http(client) instrumentation (already covered),
@@ -3,5 +3,6 @@ | |||
<PackageReference Include="System.Diagnostics.DiagnosticSource" /> | |||
<PackageReference Include="Microsoft.Extensions.Logging.Abstractions" /> | |||
<PackageReference Include="Microsoft.Extensions.Logging.Configuration" /> | |||
<PackageReference Include="OpenTelemetry.Instrumentation.AspNetCore" /> |
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.
OpenTelemetry.Instrumentation.AspNetCore
has a dependency on web components like Microsoft.AspNetCore.App
. When AdditionalDeps is applied to other .NET type applications, it may interfere with those applications. Therefore, it is not recommended to include web components in the store for auto-instrumentation purposes.
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.
Do you see better idea to load dependecy targeted fo .NET7 in net7 runtime?
Based on what I see in store chnages, there is no new libraries included into it. I will double check it tommorow.
…ate items or use the Update functionality to ensure a consistent restore behavior. The duplicate 'PackageVersion' items are: Microsoft.Extensions.Logging.Abstractions 8.0.0, Microsoft.Extensions.Logging.Abstractions 8.0.0; Microsoft.Extensions.Logging.Configuration 8.0.0, Microsoft.Extensions.
Handled by #3246 |
Why
Fixes #3212
Handles changes from open-telemetry/opentelemetry-dotnet#3391
What
Fixes AspNetCore Instrumentation for traces for .NET7.
It is now producing
Microsoft.AspNetCore
as it should.What is more
OpenTelemetry.Instrumentation.AspNetCore
and its dependencies is loaded from additional store. It has to be loaded with dedicated-compiled version for each .NET version.Tests
Modified CI.
Checklist
CHANGELOG.md
is updated.[ ] Documentation is updated.Newfeatures are covered by tests.