Azure / azure-sdk-for-net

This repository is for active development of the Azure SDK for .NET. For consumers of the SDK we recommend visiting our public developer docs at https://learn.microsoft.com/dotnet/azure/ or our versioned developer docs at https://azure.github.io/azure-sdk-for-net.
MIT License
5.47k stars 4.8k forks source link

[FEATURE REQ] Support for extending app insights Dependency.Type values #45112

Closed johncrim closed 1 month ago

johncrim commented 3 months ago

Library name

Azure.Monitor.OpenTelemetry.Exporter 1.3.0

Please describe the feature.

App insights classic supports customizing Telemetry via the ITelemetryInitializer interface; the Azure.Monitor.OpenTelemetry.Exporter only supports customizing telemetry via the otel extension points - the Exporter classes are all internal and there are no extension points for intercepting and modifying values before they are sent over the wire.

While adding such an extension point would be great (eg I could fix #45089 if I could modify the telemetry, and it could allow a way to validate telemetry in tests), the other option is to improve the mapping logic so it can be overridden using otel values. The most pressing of these is the Dependency.Type value. The current exporter logic for determining Dependency.Type is:

https://github.com/Azure/azure-sdk-for-net/blob/e1d67ba7056e66497a3696c39149b4d08fd4a097/sdk/monitor/Azure.Monitor.OpenTelemetry.Exporter/src/Customizations/Models/RemoteDependencyData.cs#L26C1-L40C14

https://github.com/Azure/azure-sdk-for-net/blob/e1d67ba7056e66497a3696c39149b4d08fd4a097/sdk/monitor/Azure.Monitor.OpenTelemetry.Exporter/src/Customizations/Models/RemoteDependencyData.cs#L50C1-L57C14

https://github.com/Azure/azure-sdk-for-net/blob/e1d67ba7056e66497a3696c39149b4d08fd4a097/sdk/monitor/Azure.Monitor.OpenTelemetry.Exporter/src/Internals/TraceHelper.cs#L450C2-L467C10

Since the exporter currently provides no reasonable way to provide a value for this field, all non-platform dependencies, and some platform dependencies like GRPC, all end up with "Type: Other". Which is not helpful as a top-level value relied upon by the App Insights UI:

image

What I'd like to see, in order from easiest to hardest:

  1. Define and document an Activity tag that, when set, is used for the Dependency.Type property value. Eg ai.dependency.type. When set, this value should override the existing logic.

  2. Work with the app insights web UI team to define some standard values and icons for this property. Eg grpc, websocket, db that are added the predefined platform values (like "HTTP" and "Microsoft.KeyVault") for dependency types. Then document those, and have the web UI team add support for showing the correct icon based on the Dependency.Type value.

  3. Ideally, define and document Activity tags for other top-level AI telemetry properties that can't currently be populated given the exporter logic; OR add an extension point for setting telemetry values, and make the telemetry classes non-internal. It's reasonable to create a separate feature request for this - I'd be happy to do so, but in this issue I just wanted to point out that there are other values that can't be set given the current exporter logic, and ideally they should all be addressed in the same way.

github-actions[bot] commented 3 months ago

Thanks for the feedback! We are routing this to the appropriate team for follow-up. cc @cijothomas @rajkumar-rangaraj @reyang @TimothyMothra @vishweshbankwar.

johncrim commented 3 months ago

I should also add - kind of related to this issue - there is currently no code path (set of input values) in the Exporter that results in OperationType.Rpc.

Here there is some helpful logic that results in emitting top-level telemetry properties if activityType is OperationType.Rpc:

https://github.com/Azure/azure-sdk-for-net/blob/09a650ffd29ed3d531224edb4d47cd51e8aeb9b5/sdk/monitor/Azure.Monitor.OpenTelemetry.Exporter/src/Customizations/Models/RemoteDependencyData.cs#L26C1-L41C1

            switch (activityTagsProcessor.activityType)
            {
                case OperationType.Http:
                    SetHttpDependencyPropertiesAndDependencyName(activity, ref activityTagsProcessor.MappedTags, isNewSchemaVersion, out dependencyName);
                    break;
                case OperationType.Db:
                    SetDbDependencyProperties(ref activityTagsProcessor.MappedTags);
                    break;
                case OperationType.Rpc:
                    SetRpcDependencyProperties(ref activityTagsProcessor.MappedTags);
                    break;
                case OperationType.Messaging:
                    SetMessagingDependencyProperties(activity, ref activityTagsProcessor.MappedTags);
                    break;
            }

https://github.com/Azure/azure-sdk-for-net/blob/09a650ffd29ed3d531224edb4d47cd51e8aeb9b5/sdk/monitor/Azure.Monitor.OpenTelemetry.Exporter/src/Customizations/Models/RemoteDependencyData.cs#L106C1-L112C10

        private void SetRpcDependencyProperties(ref AzMonList rpcTagObjects)
        {
            var rpcAttributeTagObjects = AzMonList.GetTagValues(ref rpcTagObjects, SemanticConventions.AttributeRpcService, SemanticConventions.AttributeRpcSystem, SemanticConventions.AttributeRpcStatus);
            Data = rpcAttributeTagObjects[0]?.ToString().Truncate(SchemaConstants.RemoteDependencyData_Data_MaxLength);
            Type = rpcAttributeTagObjects[1]?.ToString().Truncate(SchemaConstants.RemoteDependencyData_Type_MaxLength);
            ResultCode = rpcAttributeTagObjects[2]?.ToString().Truncate(SchemaConstants.RemoteDependencyData_ResultCode_MaxLength);
        }

but there is no set of Activity tags that results in activityType being set to OperationType.Rpc :

https://github.com/Azure/azure-sdk-for-net/blob/09a650ffd29ed3d531224edb4d47cd51e8aeb9b5/sdk/monitor/Azure.Monitor.OpenTelemetry.Exporter/src/Internals/ActivityTagsProcessor.cs#L96C1-L121C18

                if (s_semanticsSet.Contains(tag.Key))
                {
                    switch (tag.Key)
                    {
                        case SemanticConventions.AttributeHttpMethod:
                            activityType = OperationType.Http;
                            break;
                        case SemanticConventions.AttributeHttpRequestMethod:
                            activityType = OperationType.Http | OperationType.V2;
                            break;
                        case SemanticConventions.AttributeDbSystem:
                            activityType = OperationType.Db;
                            break;
                        case SemanticConventions.AttributeMessagingSystem:
                            activityType = OperationType.Messaging;
                            break;
                        case SemanticConventions.AttributeAzureNameSpace:
                            AzureNamespace = tag.Value.ToString();
                            break;
                        case SemanticConventions.AttributeEnduserId:
                            EndUserId = tag.Value.ToString();
                            continue;
                    }

                    AzMonList.Add(ref MappedTags, tag);
                }
johncrim commented 3 months ago

Stack overflow post related to this feature request (just to show I'm not the only one interested): https://stackoverflow.com/questions/42816122/application-insights-dependency-types

TimothyMothra commented 1 month ago

I'm consolidating all issues regarding overriding specific fields in the Application Insights schema into another issue to help with tracking. https://github.com/Azure/azure-sdk-for-net/issues/46021

johncrim commented 1 month ago

Hi @TimothyMothra - I want to point out that this issue is more than just "I should be able to set the Dependency.Type value". This part is also important (otherwise the type value is just rendered in App Insights as all caps text):

Work with the app insights web UI team to define some standard values and icons for this property. Eg grpc, websocket, db that are added the predefined platform values (like "HTTP" and "Microsoft.KeyVault") for dependency types. Then document those, and have the web UI team add support for showing the correct icon based on the Dependency.Type value.

I understand if that's outside the purview of this project, but there's no github repo for the App Insights UI or standardizing and documenting types and icons.

TimothyMothra commented 1 month ago

Hi @johncrim, I have to maintain this list of issues and ensure that they pertain to the SDK in this repo.

@mattmccleary what's the best way for customers to provide feedback to the UX?

mattmccleary commented 1 month ago

Hi @johncrim and @TimothyMothra, Feedback Hub is the official way to offer product feedback. https://feedback.azure.com/d365community/forum/3887dc70-2025-ec11-b6e6-000d3a4f09d0 Feel free to circle back with a link to you post to ensure it gets seen in a timely manner.

johncrim commented 1 month ago

But you guys are awesome, and feedback hub is where feedback goes to die. Honestly I think I've only seen one item in feedback hub make into a product.

OK, I'll try posting something there.

mattmccleary commented 1 month ago

Thanks John. Send me the item after you post it, I'll make sure it gets seen.

johncrim commented 1 month ago

I wrote this 6 months ago, it's the same idea; comes from the same requirement. I wrote it before we migrated from App Insights classic to otel.

https://feedback.azure.com/d365community/idea/9f258d7b-eae2-ee11-a73d-0022484c4e0d

I amended it from "custom Span" to "Dependency Type". Though I think it might be better design to have a new "CustomSpan" telemetry type, that might be too much effort for the incremental value. I've since found that internal spans are represented as dependencies in App Insights, so the more practical approach would be to allow customizing Dependency.type.

Note that we have internal operations in our browser app, and internal operations in our services, that are modelled as otel Spans; but in the App Insights world spans have to either be requests or dependencies or pageviews, and a request has a specific schema that really only works for http (and marginally works for other things like Web Sockets), and a pageview is clearly a specific thing, so everything that's not a request or pageview has to be called a "dependency".

johncrim commented 1 month ago

Got it @mattmccleary - go ahead and delete.