Make request span name settable (via Activity.DisplayName)

[johncrim]

This PR provides a fix for #44971. In short, it must be possible to set the span name; and the otel exporter must not ignore an explicitly set value.

More comments in the code.

Related to: https://github.com/open-telemetry/opentelemetry-dotnet-contrib/issues/1948 but this problem is much worse - that issue could be left as is, but this one must be fixed IMO.

fixes: #44971

[github-actions[bot]]

Thank you for your contribution @johncrim! We will review the pull request and get back to you soon.

[TimothyMothra]

Hi @johncrim, I'm sorry for the delay, I finally had time to look at this.

Thank you for your investigation. I was particularly interested in your note that the value we're constructing in our helper methods was available in Activity.DisplayName. I investigated this because we don't want to do extra work if it's not necessary. Unfortunately, I found some cases where the value in DisplayName was not sufficient so we won't be making any changes here.

The more important matter is your need to override span name. You're not alone, we're getting a lot of customer asks/complaints regarding a need to override specific fields in the Application Insights schema. I can share that I am working on a project to identify all of these gaps and provide a workaround. It's still early days so I don't have an ETA to share.

I do appreciate all the issues you report so please keep the feedback coming! You can also provide feedback to my team here: [email protected]

cc: @mattmccleary

[johncrim]

Hi @TimothyMothra - thank you for the update. While I've been frustrated with some of the issues, you and your team have been responsive to the feedback, so I'm happy to do my part to help make it better. Thank you for working with me and the community. I do have at least 1 more significant issue that I haven't documented yet (more on the otel lib side), and a few other suggestions, so I'll reach out to your team email soon.

On this topic, I think the "right" approach is described here - clearly the aspnetcore instrumentation library and azmon exporter need to be on the same page (good thing it's the same people!). I think the way to go here is for the aspnetcore listener to set it once on the end event (only if it hasn't been changed from ..HttpRequestIn), and the azmon exporter shouldn't have to change it. My 2cents.

[johncrim]

RE Tim's comment:

Unfortunately, I found some cases where the value in DisplayName was not sufficient so we won't be making any changes here.

I don't think this PR should be taken "as is", it was mainly to show the problem more thoroughly and propose a fix. Also, this is the only place I can implement a test demonstrating the issue by validating the exporter output, since all the exporter types are internal.

This and https://github.com/open-telemetry/opentelemetry-dotnet-contrib/issues/1948 should be fixed at the same time, and if the otel-aspnetcore bug is fixed well this bug should have a trivial fix.

Other points I'm trying to make with this PR:

1. The WebServerTelemetryCustomizationTests should be included - and in general more e2e or integration tests like this seem like a good idea.
2. There are a number of unit tests that enforce that the span name/operation name are always set by the exporter - these should be removed. These tests are enforcing duplicate logic with the aspnetcore instrumentation library. So you get good code coverage in your exporter tests, but that's a false metric. What matters more is the behavior when integrated with the aspnetcore instrumentation library, since they will always be used together.

Feel free to take any changes that are useful, or take over the PR, or close the PR if you have a better fix. Or I'm happy to amend it once https://github.com/open-telemetry/opentelemetry-dotnet-contrib/issues/1948 is resolved.

[github-actions[bot]]

Hi @johncrim. Thank you for your interest in helping to improve the Azure SDK experience and for your contribution. We've noticed that there hasn't been recent engagement on this pull request. If this is still an active work stream, please let us know by pushing some changes or leaving a comment. Otherwise, we'll close this out in 7 days.

[johncrim]

Closing this since I don't think it's going anywhere. The fixes here should be handled by a fix for #46021