search

open-telemetry / opentelemetry-dotnet-contrib

This repository contains set of components extending functionality of the OpenTelemetry .NET SDK. Instrumentation libraries, exporters, and other components can find their home here.
https://opentelemetry.io
Apache License 2.0
475 stars 283 forks source link

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

Open cijothomas opened 2 years ago

cijothomas commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

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.

martinjt commented 5 months ago

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