[Instrumentation.AspNetCore] Tests are preventing attribute changes

[RassK]

What

#3338 tries to normalize route templates according to spec.

Issues with tests:

• Single test verifies spans and metrics
• Single test verifies controllable code and uncontrollable

I guess most of the newer metrics that are pushed by the framework are OTel native, so contrib does not have control over values. This is bundled together with metrics that are pushed from the package. The result is a lock up to any changes.

I wanted to bring up the limitations to see what are the possible paths forward. Also for the future when OTel is more fast changing than large frameworks - that's where the separate package was actually helpful.

[martincostello]

My first thought is to split the tests into two sets so that "in-framework" and "in-package" tests can diverge in the expectations.

I already had to make some changes in #3100 to prepare for .NET 10 in #2822 to account for differences in behaviour across versions.

[RassK]

My idea currently is to do following in order:

1. Split spans and metrics (since spans have no issues)
2. Split metrics between in-framework and in-package.

And additionally think if there is a path to take back some control, so OTel can fix and move faster. That for sure means there should be a separate package, which asp.net core anyway has tons.

[RassK]

@martincostello is it correct that tests do not hit even HttpInMetricsListener any more ?

[martincostello]

It doesn't sound right - there should be some test coverage for it somewhere.

[RassK]

None of the tests hit HttpInMetricsListener.OnStopEventWritten() while debugging .NET 8 and .NET 9 OpenTelemetry.Instrumentation.AspNetCore.Tests.

HttpInMetricsListener was added in #2403 for netstandard builds, but tests do not have netstandard builds.

[martincostello]

Tests can't target netstandard2.0 itself, but they should still be able to reference the assembly to test it from a net8.0 or net9.0 target.

[Kielek]

@RassK, it might be true that HttpInMetricsListener is not executed any longer, what is more, typically it should be not used by any supported .NET version. If you want to make there any changes, you should be able to add .NET6 target.

It is leftover/backward compatibility file. It will be great to reconsider removing this at some point. I am not sure when/if it can be done. @alanwest, any thoughts here?

[alanwest]

It is leftover/backward compatibility file. It will be great to reconsider removing this at some point. I am not sure when/if it can be done. @alanwest, any thoughts here?

It would be nice to remove it at some point, but I think it warrants a major version bump. We maintain a netstandard2.0 build which means users using a version of .NET that have gone out of support will begin to depend on the netstandard2.0 build. If they upgrade to the latest version of the instrumentation they'd lose metric support. This is a breaking change we decided we would not make without a major version bump.

[alanwest]

I guess most of the newer metrics that are pushed by the framework are OTel native, so contrib does not have control over values. This is bundled together with metrics that are pushed from the package. The result is a lock up to any changes.

These tests were purposefully testing code that we control and code we do not control. This is because we need to maintain consistency between our instrumentation and the instrumentation owned by the ASP.NET Core team.

A couple years ago, the ASP.NET Core team natively instrumented ASP.NET Core to emit metrics. They do not currently fully manage the activities created. They start the activity natively, but they do not maintain the attributes added to the activity, it is still the responsibility of our instrumentation to do this.

The values of the attributes added to metrics and activities should not diverge. If we wish to make a change then we need to collaborate with the ASP.NET Core team so that they can make the same change to the metrics they produce.

I see the test cases were split in #3350. Sorry I missed this PR, but I'm not so sure this is a good idea. Won't this make it more difficult to see if metrics and spans diverge?

[RassK]

I see the test cases were split in #3350. Sorry I missed this PR, but I'm not so sure this is a good idea. Won't this make it more difficult to see if metrics and spans diverge?

They will diverge soon after if you split the source of truth. If you want them not to diverge, the source of truth must be the same / single one (so preferably the fastest upgradable path owns the control). These limitation are exactly what caused this simple issue to roll on for years (you opened the issue in 2023!), and not being fixed.

[RassK]

There is broken consistency already for Netstandard (linking last release):
https://github.com/open-telemetry/opentelemetry-dotnet-contrib/blob/Instrumentation.AspNetCore-1.14.0-rc.1/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs#L246
https://github.com/open-telemetry/opentelemetry-dotnet-contrib/blob/Instrumentation.AspNetCore-1.14.0-rc.1/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs#L95

[alanwest]

I spoke with @Kielek today regarding diverging behavior between spans and metrics.

I do not agree that this is entirely out of our control just because we don't own both libraries. Though I do agree that the metric and span instrumentation being split across two libraries makes moving forward more complex for us.

We partnered closely with the ASP.NET Core team when we stabilized this instrumentation. We should continue that partnership and propose to them the changes we want to see. Some options we could discuss with them:

1. The ASP.NET Core team could mirror the changes we want to make to spans on the metrics they produce.
2. If the ASP.NET Core team provided us a hook to change the value of an attribute on a metric then we could own keeping spans and metrics the same.
3. The ASP.NET Core team could take on full ownership of both metric and span instrumentation for ASP.NET Core. That is, in addition to starting the span they would also add all the appropriate attributes. This option would allow us to eventually retire our instrumentation package.

I personally think option 3 would be great. It has been considered by the ASP.NET Core team in the past, but I'm not sure what their current appetite for it is. That said, I think the other two options would also be satisfactory.

[RassK]

If fixes can be made only in major version changes, then it's going to paralyze a large part of OTel .NET (since very large part of the applications are about ASP.NET). All supported versions (by OTel .NET) should be fixed if a bug / misinterpretation occurs and users should be able to bump the version asap. Instrumentation.AspNet and Instrumentation.AspNetCore take a very good care of such issues. Attributes and values should be / can be maintained by dotnet contrib, while the framework team can provide infra.

See #3338 (comment)