Skip to content

[Instrumentation.AspNetCore] Handle cases where the HttpContext is a property of the payload (ASP.NET Core 2.x support) #2724

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

mathew-tolj
Copy link

@mathew-tolj mathew-tolj commented Apr 29, 2025

Fixes open-telemetry/opentelemetry-dotnet#4126, open-telemetry/opentelemetry-dotnet#5019

Changes

Fixes an issue where using OpenTelemetry.Instrumentation.AspNetCore with ASP.NET Core 2.x (the only version still officially support on .NET Framework) does not export any metrics or traces for incoming requests. This is achieved by checking if the HttpContext is a property of the payload in cases where the payload cannot be cast to HttpContext directly. This allows OpenTelemetry.Instrumentation.AspNetCore to continue processing the event instead of short circuiting.

Merge requirement checklist

  • CONTRIBUTING guidelines followed (license requirements, nullable enabled, static analysis, etc.)
  • Unit tests added/updated
  • Appropriate CHANGELOG.md files updated for non-trivial changes
  • Changes in public API reviewed (if applicable)

Copy link

linux-foundation-easycla bot commented Apr 29, 2025

CLA Signed


The committers listed above are authorized under a signed CLA.

@github-actions github-actions bot added the comp:instrumentation.aspnetcore Things related to OpenTelemetry.Instrumentation.AspNetCore label Apr 29, 2025
@mathew-tolj mathew-tolj marked this pull request as ready for review April 29, 2025 23:24
@mathew-tolj mathew-tolj requested a review from a team as a code owner April 29, 2025 23:24
@Kielek
Copy link
Contributor

Kielek commented Apr 30, 2025

@mathew-tolj, thank you for the contribution.
I think that I need to clarify couple things with @alanwest and @rajkumar-rangaraj.

Based on this

This package was never intended to work with .NET Framework. In fact, it will not if we will do not have .NET Standard 2.0 target for all packages in contrib.

What is your perspective on adding such support for this legacy stuff?

@mathew-tolj , If other maintainers will be fine with extending support we need to do couple other things

  1. Consider net462 as an additional target framework in the csproj.
  2. Extending test coverage for .NET Framework. See already linked file.
  3. Updating documentation, probably in both cases, whether we will be adding this support or not.

@mathew-tolj
Copy link
Author

I did notice that there weren't any existing tests for the .NET Standard target that this PR is for so that is why I had not pushed any changes to the test projects. It is pretty easy to add a test to the .NET 8 and 9 that tests extraction of the HttpContext from an anonymous object but I wasn't sure that was the way to go as that doesn't represent the real world scenario accurately.

I have also attempted to add a .NET Framework target to the test projects and that is mostly working although it is not without it's own challenges and I am not sure if the code is of a good enough standard to push. As of now there are 3 remaining obstacles:

  • There appears to be an issue with compilation of the Razor pages under .NET Framework that I have not figured out yet.
  • One of the tests focused on exception handling fails because the exception is bubbling up and short circuiting the test.
  • The biggest issue is that there is no way to the route template in the OpenTelemetry code when using ASP.NET 2.x as endpoint routing support was not added until ASP.NET Core 3.0. This mean that the activity name is not updated and the http.route tag is not added. The missing tag is especially problematic as a lot of the tests use it as part of their assertions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:instrumentation.aspnetcore Things related to OpenTelemetry.Instrumentation.AspNetCore
Projects
None yet
Development

Successfully merging this pull request may close these issues.

.NET Framework dependency issue
2 participants