Skip to content
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+ #3246

Merged

Conversation

Kielek
Copy link
Contributor

@Kielek Kielek commented Jan 24, 2024

Why

Fixes #3212
Replaces: #3231
Handles changes from open-telemetry/opentelemetry-dotnet#3391
Needs changes from open-telemetry/opentelemetry-dotnet#5252

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.
Tested locally with library build on open-telemetry/opentelemetry-dotnet#5252

Checklist

  • CHANGELOG.md is updated.
  • [ ] Documentation is updated.
  • New features are covered by tests.

@github-actions github-actions bot requested a review from theletterf January 24, 2024 15:53
@rajkumar-rangaraj
Copy link
Contributor

Needs changes from open-telemetry/opentelemetry-dotnet#5252

This currently proposed PR should solve the issue, so there is no need to propose a change in the instrumentation library.

More details

The instrumentation library's behavior varies based on the .NET version in use:

  • For .NET 6.0:
    The library utilizes .AddSource("OpenTelemetry.Instrumentation.AspNetCore") combined with .AddLegacySource("Microsoft.AspNetCore.Hosting.HttpRequestIn").

    • Here, the ActivitySourceName is set from the HttpInListener class in the Instrumentation library.
  • For .NET 7.0 and above:
    The setup is streamlined to just builder.AddSource("Microsoft.AspNetCore"). In this scenario, the ActivitySourceName is set directly from the Framework.

  • Auto-Instrumentation:
    The currently proposed code automatically configures the instrumentation based on the .NET environment. The changes in this PR should resolve the issue, eliminating the need for any further alterations to the instrumentation library itself.

if (Environment.Version.Major == 6)
{
    return builder
           .AddSource("OpenTelemetry.Instrumentation.AspNetCore")
           .AddLegacySource("Microsoft.AspNetCore.Hosting.HttpRequestIn");
}

return builder.AddSource("Microsoft.AspNetCore");

We have already covered this for the metric part, and it does not require any changes.

@Kielek Please let me know your thoughts.

@vishweshbankwar FYI.

@Kielek
Copy link
Contributor Author

Kielek commented Jan 25, 2024

@rajkumar-rangaraj, please check the whole execution flow in typical, manual/library instrumentation:

  1. Call AddAspNetCoreInstrumentation.
  2. Under the hood it is registering sources as you descrbed by AddAspNetCoreInstrumentationSources. This requirements can be addressed on our side, and in fact are included in this PR.
  3. AspNetCoreInstrumentation is created. It relays on HttpInListener. For all .NET versions. Including .NET 7 and .NET 8.
  4. When instrumentation in handled this part of the code is called:
    https://github.com/open-telemetry/opentelemetry-dotnet/blob/Instrumentation.AspNetCore-1.7.0/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs#L128-L135. This is the part we cannot handle without changes in the instrumentation library (+ some other minor, conditional compilation).

I can narrow PR changes to the instrumentation library and apply changes only to HttpInListener (including removing entry in the changelog). It will be sufficient for us, but it will be not sufficient for other similar cases.

BTW, all tests in this PR should be green if the changes in the instrumentation library are not necessary.

For metrics, it is working as you described. There is no any internal/instrumentation class under the hood. For .NET8+ is is just calling MeterProviderBuilder.AddSource multiple times: https://github.com/open-telemetry/opentelemetry-dotnet/blob/Instrumentation.AspNetCore-1.7.0/src/OpenTelemetry.Instrumentation.AspNetCore/AspNetCoreInstrumentationMeterProviderBuilderExtensions.cs#L22-L39

@vishweshbankwar
Copy link
Member

4. When instrumentation in handled this part of the code is called:
https://github.com/open-telemetry/opentelemetry-dotnet/blob/Instrumentation.AspNetCore-1.7.0/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs#L128-L135. This is the part we cannot handle without changes in the instrumentation library (+ some other minor, conditional compilation).

This should be ok if the #else part executes for net7 or greater. Unless you are running in to some specific issue.

@Kielek Kielek marked this pull request as ready for review February 12, 2024 06:38
@Kielek Kielek requested a review from a team February 12, 2024 06:38
@Kielek
Copy link
Contributor Author

Kielek commented Feb 12, 2024

1.7.1 packages released. We have agreed with Raj and Vishwesh that it is locally the best option to proceed.

@Kielek Kielek merged commit bd3855d into open-telemetry:main Feb 12, 2024
32 checks passed
@Kielek Kielek deleted the aspnetcore-net6-lib-working-on-net7-v2 branch February 12, 2024 07:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants