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

Review the conditional compilations in ASPNET Core and other instrumentations. #1777

Open
cijothomas opened this issue Mar 11, 2022 · 3 comments
Labels
bug Something isn't working comp:instrumentation.aspnetcore Things related to OpenTelemetry.Instrumentation.AspNetCore help wanted Extra attention is needed infra Infra work - CI/CD, code coverage, linters

Comments

@cijothomas
Copy link
Member

cijothomas commented Mar 11, 2022

Due to the addition of more targets like net5.0 and net6.0 to the instrumention projects, the conditional checks inside the code seems broken now.

example:
https://github.com/open-telemetry/opentelemetry-dotnet/blob/main/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs#L313 check for
#if NETSTANDARD2_1, means that code path is not triggered for net5.0 or net6.0

Opening an issue to do a round of reviews to make sure the conditional compilation flags are still correct, after the addition of new project targets.

@TimothyMothra
Copy link
Contributor

@cijothomas In your PR, what was the reason for using #if NETSTANDARD2_1_OR_GREATER || NETCOREAPP3_1_OR_GREATER instead of just #if NETSTANDARD2_1_OR_GREATER ?

I would have expected the single to work, I'm curious if you found an issue with that.

@cijothomas
Copy link
Member Author

Because the project had NETCOREAPP3.1/NET5.0/NET6.0 targets (along with netstandard), and I don't think NETSTANDARD2_1 covers that. Happy to correct myself, if this is not the case.

@vishweshbankwar vishweshbankwar transferred this issue from open-telemetry/opentelemetry-dotnet May 14, 2024
@Kielek Kielek added infra Infra work - CI/CD, code coverage, linters comp:instrumentation.aspnetcore Things related to OpenTelemetry.Instrumentation.AspNetCore labels May 17, 2024
@martinjt
Copy link
Member

martinjt commented Jun 2, 2024

Closing as a lot has changed in the last 2 years, it can be reopened if needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working comp:instrumentation.aspnetcore Things related to OpenTelemetry.Instrumentation.AspNetCore help wanted Extra attention is needed infra Infra work - CI/CD, code coverage, linters
Projects
None yet
Development

No branches or pull requests

4 participants