Configuring a `User-Agent` header on `OtlpExporterOptions` may produce an exception

[stevejgordon]

Bug Report

Affected Package(s):

• OpenTelemetry 1.6.0

Runtime version:

• Any (experienced on net8.0)

Symptom

Including a User-Agent header in OtlpExporterOptions produces an exception with the HTTP exporter and an undesired behaviour for the GRPC exporter.

What is the expected behavior?

If consuming code provides a User-Agent header, this should be preferred over the value from OtlpExporterOptions.StandardHeaders.

What is the actual behavior?

When the agent is configured to use the http/protobuf protocol, an ArgumentException is thrown due to an attempt to add a duplicate key.

An item with the same key has already been added. Key: User-Agent

This exception was originally thrown at this call stack:
    System.ThrowHelper.ThrowAddingDuplicateWithKeyArgumentException<T>(T)
    System.Collections.Generic.Dictionary<TKey, TValue>.TryInsert(TKey, TValue, System.Collections.Generic.InsertionBehavior)
    System.Collections.Generic.Dictionary<TKey, TValue>.Add(TKey, TValue)
    OpenTelemetry.Exporter.OpenTelemetryProtocol.Implementation.ExportClient.BaseOtlpHttpExportClient<TRequest>..ctor.AnonymousMethod__0_0(System.Collections.Generic.Dictionary<string, string>, string, string) in BaseOtlpHttpExportClient.cs
    OpenTelemetry.Exporter.OtlpExporterOptionsExtensions.GetHeaders<THeaders>(OpenTelemetry.Exporter.OtlpExporterOptions, System.Action<THeaders, string, string>) in OtlpExporterOptionsExtensions.cs
    OpenTelemetry.Exporter.OpenTelemetryProtocol.Implementation.ExportClient.BaseOtlpHttpExportClient<TRequest>.BaseOtlpHttpExportClient(OpenTelemetry.Exporter.OtlpExporterOptions, System.Net.Http.HttpClient, string) in BaseOtlpHttpExportClient.cs
    OpenTelemetry.Exporter.OpenTelemetryProtocol.Implementation.ExportClient.OtlpHttpTraceExportClient.OtlpHttpTraceExportClient(OpenTelemetry.Exporter.OtlpExporterOptions, System.Net.Http.HttpClient) in OtlpHttpTraceExportClient.cs
    OpenTelemetry.Exporter.OtlpExporterOptionsExtensions.GetTraceExportClient(OpenTelemetry.Exporter.OtlpExporterOptions) in OtlpExporterOptionsExtensions.cs
    OpenTelemetry.Exporter.OtlpTraceExporter.OtlpTraceExporter(OpenTelemetry.Exporter.OtlpExporterOptions, OpenTelemetry.Exporter.OpenTelemetryProtocol.Implementation.SdkLimitOptions, OpenTelemetry.Exporter.OpenTelemetryProtocol.Implementation.ExportClient.IExportClient<OpenTelemetry.Proto.Collector.Trace.V1.ExportTraceServiceRequest>) in OtlpTraceExporter.cs
    OpenTelemetry.Trace.OtlpTraceExporterHelperExtensions.BuildOtlpExporterProcessor(OpenTelemetry.Exporter.OtlpExporterOptions, OpenTelemetry.Exporter.OpenTelemetryProtocol.Implementation.SdkLimitOptions, System.IServiceProvider, System.Func<OpenTelemetry.BaseExporter<System.Diagnostics.Activity>, OpenTelemetry.BaseExporter<System.Diagnostics.Activity>>) in OtlpTraceExporterHelperExtensions.cs
    ...
    [Call Stack Truncated]

When the agent is configured to use the grpc/protobuf protocol, no exception is thrown, however, the User-Agent header from OtlpExporterOptions.StandardHeaders will also be added to the metadata.

Reproduce

Add a custom User-Agent header when adding an OTLP exporter to a TracerProviderBuilder, as follows:

builder.AddOtlpExporter(options => options.Headers = "User-Agent=MY-CUSTOM-UA");

Run the application with at least one span being created and sent to the backend.

Additional Context

This specific scenario of setting the User-Agent header is likely rare. In our case, we are starting work on an Elastic OTel Agent Distro and would like to update the header to include our version information (in addition to the Otel SDK version info). Having reviewed the current code, this could also occur if any further OtlpExporterOptions.StandardHeaders are added in the future, which conflict with those set on the options by a consumer.

I would propose updating the implementation such that if any header from OtlpExporterOptions.StandardHeaders is already defined on the consumer-provided Headers, then the code should not attempt to add the standard header.

I'm opening this issue to discuss, as this is a blocker for our work. I would be happy to contribute a PR to fix this.

[martinjt]

Could you override this in the HttpClientFactory?

builder.Services.AddOpenTelemetry()
    .WithTracing(tpb => 
    {
        tpb.AddOtlpExporter(o => {
            o.HttpClientFactory = () => {
                return new HttpClient( ... );
            };
        });
    });

[stevejgordon]

@martinjt Thanks for the suggestion. I hadn't discovered that hook, and indeed, that does let me solve the issue when using http/protobuf. It's a bit involved, though, so I would still propose the logic be updated to accept this directly. Again, happy to contribute that via a PR unless there's a specific reason we don't want to do this.

I'll be honest; I'm not sure what happens in grpc/protobuf scenario where two entries with the same key are defined in the gRPC metadata. I don't know enough about the protocol or the .NET implementation to know what goes over the wire.

[martinjt]

I think that changing headers that have been added should require that extra work.

I do think that it should be handled better. So saying "you cannot amend existing headers" in that exception would be better. Like adding an Auth header for instance.

Also, maybe having an option that allows a delegate to the HeadersCollection might be an option?

Either way, I don't think this is an easy fix. We ended up using resource attributes for your usecase instead and extracting that anonymous metadata at ingest.

One other option would be allowing the OtlpExporter to be more extensible by exposing some of the inner elements to allow new derived types to be created. I'd be interested to see how other languages are handling it.

[stevejgordon]

@martinjt I'm not sure I agree.

The consumer provider headers are added first, and then the SDK adds its standard headers, just User-Agent right now. If a user has explicitly provided a header, then it seems reasonable to assume that is preferred over anything the SDK wants to add.

Either way, I don't think this is an easy fix.

Unless I'm missing something, the fix is that the actions passed in should check for an existing key before adding it. In the case of the BaseOtlpHttpExportClient, this is a small change to the ctor:

this.Headers = options.GetHeaders<Dictionary<string, string>>((d, k, v) => d.TryAdd(k, v));

[stevejgordon]

One other problem I foresee with resolving it via the handler approach is that it complicates scenarios for the distro where the consumer of that might want to provide their own HttpClient factory.

[vishweshbankwar]

@stevejgordon It would be good to get this clarification from spec itself. Spec does not outline how this conflict should be handled.

[stevejgordon]

@vishweshbankwar, Can you point me to the best place to raise this? I'd assumed it was more an implementation detail of the agent but I'm happy to raise it somewhere more central.