Make request span name settable (via Activity.DisplayName) #44974

johncrim · 2024-07-12T03:21:50Z

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: open-telemetry/opentelemetry-dotnet-contrib#1948
but this problem is much worse - that issue could be left as is, but this one must be fixed IMO.

fixes: #44971

…Name fixes: Azure#44971

Previously some of these tests were enforcing that Activity.DisplayName be ignored, which I think is clearly incorrect.

github-actions · 2024-07-12T03:22:03Z

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

johncrim · 2024-07-12T03:29:28Z

sdk/monitor/Azure.Monitor.OpenTelemetry.Exporter/src/Customizations/Models/TelemetryItem.cs

+                    // Also, it's important than any user-set DisplayName be used #44971
+                    Tags[ContextTagKeys.AiOperationName.ToString()] = displayName.Truncate(SchemaConstants.Tags_AiOperationName_MaxLength);
+                }
+                else if (activityTagsProcessor.activityType.HasFlag(OperationType.V2))
                {
                    Tags[ContextTagKeys.AiOperationName.ToString()] = TraceHelper.GetOperationNameV2(activity, ref activityTagsProcessor.MappedTags).Truncate(SchemaConstants.Tags_AiOperationName_MaxLength);


Note that the logic in TraceHelper.GetOperationNameV2 (and TraceHelper.GetOperationNameV2) sets the same value that Activity.DisplayName was already set to by the AspNetCore instrumentation. So unless this exporter is used without OpenTelemetry.Instrumentation.AspNetCore or .AddAspNetCoreInstrumentation() (which currently isn't possible) lines 28-37 (in the old code) are redundant with the instrumentation library.

For this version of the PR I left them in, because there are tests that enforce this logic, but I think they should be removed.

This comments should be simplified and code deleted before the PR is merged

johncrim · 2024-07-12T04:11:08Z

sdk/monitor/Azure.Monitor.OpenTelemetry.Exporter/src/Customizations/Models/TelemetryItem.cs

-                if (activityTagsProcessor.activityType.HasFlag(OperationType.V2))
+                var displayName = activity.DisplayName;
+                if (!string.IsNullOrEmpty(displayName)
+                    && !displayName.StartsWith(activity.Source.Name))


This check is intended to handle detecting whether the DisplayName has been set to a value different than the value it started with. We know from the source code that the ASP.NET core request activity starts with the name Microsoft.AspNetCore.Hosting.HttpRequestIn and comes from activity source Microsoft.AspNetCore, and there's a bunch of other code that depends on that.

We know from the source code that the ASP.NET core request activity starts with the name Microsoft.AspNetCore.Hosting.HttpRequestIn

This may be tricky. Asp.Net Core is not the only instrumentation possible, and this exporter shouldn't bake in any assumptions about instrumentations.

I agree with you @cijothomas . I think the "right" fix for this is just to use the activity.DisplayName, and skip all the other conditions. As I tried to explain, they are unnecessary b/c the same values are set by the AspNetCore instrumentation. But if I'd made that fix I would have broken a number of tests that enforce that (redundant) logic.

Also, I was trying to come up with a way to determine whether any other instrumentation library has set the value. This does work for that problem. My thinking is: If the developer or any other instrumentation library has set the DisplayName value, it should not be overwritten by the AspNetCore instrumentation (so this is a possible way to detect changes to fix open-telemetry/opentelemetry-dotnet-contrib#1948 ).

johncrim · 2024-07-12T04:12:23Z

sdk/monitor/Azure.Monitor.OpenTelemetry.Exporter/src/Customizations/Models/TelemetryItem.cs

-                if (activityTagsProcessor.activityType.HasFlag(OperationType.V2))
+                // All of this block should be changed to:
+                // Tags[ContextTagKeys.AiOperationName.ToString()] = activity.DisplayName.Truncate(SchemaConstants.Tags_AiOperationName_MaxLength);
+                // 


All of this block should be changed to this one line, but I'm leaving the long version in for review and discussion.

Also, the comments are more extensive than they should be; they are there for the review and should be simplified before the PR is merged.

johncrim · 2024-07-12T04:14:39Z

...penTelemetry.Exporter/tests/Azure.Monitor.OpenTelemetry.Exporter.Tests/TelemetryItemTests.cs

@@ -20,6 +20,9 @@ public class TelemetryItemTests
        private const string ActivitySourceName = "TelemetryItemTests";
        private const string ActivityName = "TestActivity";

+        private const string RequestActivitySourceName = "Microsoft.AspNetCore";
+        private const string RequestActivityName = "Microsoft.AspNetCore.Hosting.HttpRequestIn";


This could be removed if tests are removed that enforce the TraceHelper.GetOperationNameV2() and TraceHelper.GetOperationName() logic above.

johncrim · 2024-07-12T16:27:40Z

...penTelemetry.Exporter/tests/Azure.Monitor.OpenTelemetry.Exporter.Tests/TelemetryItemTests.cs

                ActivityKind.Server,
                null,
                startTime: DateTime.UtcNow);

            Assert.NotNull(activity);
-            activity.DisplayName = "/getaction";


This test (and other tests below) are enforcing that the developer set DisplayName is overwritten by the method + route. If this line is removed then the test is now enforcing that the exporter sets a default name.

Still, this "default DisplayName" logic is in the AspNetCore instrumentation, so should not also be in the exporter.

TimothyMothra · 2024-08-15T22:56:06Z

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: OTel@microsoft.com

cc: @mattmccleary

johncrim · 2024-08-16T04:11:48Z

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 · 2024-08-16T15:24:56Z

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 open-telemetry/opentelemetry-dotnet-contrib#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:

The WebServerTelemetryCustomizationTests should be included - and in general more e2e or integration tests like this seem like a good idea.
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 open-telemetry/opentelemetry-dotnet-contrib#1948 is resolved.

github-actions · 2024-10-18T05:05:43Z

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 · 2024-10-22T18:50:46Z

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

johncrim added 2 commits July 11, 2024 19:42

fix(request telemetry): Make span name settable from Activity.Display…

07eb6b9

…Name fixes: Azure#44971

fix(tests): Fix tests related to Activity.DisplayName being ignored

e65dcac

Previously some of these tests were enforcing that Activity.DisplayName be ignored, which I think is clearly incorrect.

johncrim requested review from cijothomas, reyang, rajkumar-rangaraj, TimothyMothra and vishweshbankwar as code owners July 12, 2024 03:21

github-actions bot added Community Contribution Community members are working on the issue customer-reported Issues that are reported by GitHub users external to the Azure organization. Monitor - Exporter Monitor OpenTelemetry Exporter labels Jul 12, 2024

johncrim commented Jul 12, 2024

View reviewed changes

improve(comments): PR-only comments

e7f1194

This comments should be simplified and code deleted before the PR is merged

johncrim commented Jul 12, 2024

View reviewed changes

github-actions bot added the no-recent-activity There has been no recent activity on this issue. label Oct 18, 2024

johncrim closed this Oct 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make request span name settable (via Activity.DisplayName) #44974

Make request span name settable (via Activity.DisplayName) #44974

johncrim commented Jul 12, 2024

github-actions bot commented Jul 12, 2024

johncrim Jul 12, 2024 •

edited

Loading

johncrim Jul 12, 2024

cijothomas Jul 12, 2024

johncrim Jul 12, 2024

johncrim Jul 12, 2024 •

edited

Loading

johncrim Jul 12, 2024

johncrim Jul 12, 2024 •

edited

Loading

TimothyMothra commented Aug 15, 2024

johncrim commented Aug 16, 2024

johncrim commented Aug 16, 2024 •

edited

Loading

github-actions bot commented Oct 18, 2024

johncrim commented Oct 22, 2024

Make request span name settable (via Activity.DisplayName) #44974

Make request span name settable (via Activity.DisplayName) #44974

Conversation

johncrim commented Jul 12, 2024

github-actions bot commented Jul 12, 2024

johncrim Jul 12, 2024 • edited Loading

Choose a reason for hiding this comment

johncrim Jul 12, 2024

Choose a reason for hiding this comment

cijothomas Jul 12, 2024

Choose a reason for hiding this comment

johncrim Jul 12, 2024

Choose a reason for hiding this comment

johncrim Jul 12, 2024 • edited Loading

Choose a reason for hiding this comment

johncrim Jul 12, 2024

Choose a reason for hiding this comment

johncrim Jul 12, 2024 • edited Loading

Choose a reason for hiding this comment

TimothyMothra commented Aug 15, 2024

johncrim commented Aug 16, 2024

johncrim commented Aug 16, 2024 • edited Loading

github-actions bot commented Oct 18, 2024

johncrim commented Oct 22, 2024

johncrim Jul 12, 2024 •

edited

Loading

johncrim Jul 12, 2024 •

edited

Loading

johncrim Jul 12, 2024 •

edited

Loading

johncrim commented Aug 16, 2024 •

edited

Loading