Configurable defaults for ObservabilityOptions in http middleware

[andrueastman]

The default telemetry configuration results in creating and an ActivitySource and Activity for each request, and then disposing those objects after a response is returned. ActivitySource.Dispose calls SynchronizedList.Remove (https://github.com/dotnet/runtime/blob/main/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivitySource.cs#L299) as shown in the Azure Profiler output where the lock is present (https://github.com/dotnet/runtime/blob/main/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivitySource.cs#L402).

As we are doing this in each of the client middleware irrespective of the whether the telemetry is being listened to (e.g.

kiota-http-dotnet/src/Middleware/CompressionHandler.cs

Line 38 in 7048ed2

activitySource = new ActivitySource(obsOptions.TracerInstrumentationName);

) , the use of the SynchronizedList in the underlying telemetry implementation can be perfomance bottleneck.

A temporary workaround is to add middleware to remove the default ObservabilityOptions created from the request. But the action below should probably be configurable and "opt-in"

kiota-http-dotnet/src/HttpClientRequestAdapter.cs

Line 547 in 7048ed2

message.Options.Set(new HttpRequestOptionsKey<IRequestOption>(typeof(ObservabilityOptions).FullName!), obsOptions);

cc @baywet

[baywet]

Thanks for starting the conversation here. I'm not clear how removing the default is a workaround? Is it because it indirectly will disable any telemetry unless the consumer passes an option for the request or sets different defaults?

From what I understand the philosophy of OpenTelemetry for library producers is to trace by default, and then if no exporters are configured for the application, it'll all be piped to null. It's designed this way so developers building applications don't have to turn on a thousand knobs in each library of the stack. Disabling tracing by default would be going against the design here IMHO.

What we could do instead is rely on the registry design pattern where we'd have a singleton registry of <sourceName,source>, this would be lazily initialized by the first handler needing it, and then others would just reuse the same object (when referring to the same key). The only challenge here is that we won't be disposing of the activity sources, but since this is all going to rely on a singleton, we can probably rely on the application lifecycle. Thoughts?

Also we should check what we're doing in abstractions and authentication azure, I'm pretty sure we're creating new activity sources and disposing them for each request as well.

[andrueastman]

I'm not clear how removing the default is a workaround? Is it because it indirectly will disable any telemetry unless the consumer passes an option for the request or sets different defaults?

Yes. This would prevent the creation of the ActivitySource instances on each call to SendAsync call which cause the lock contention when disposed. So, there will be no initialization without a ObserverbilityOptions request option.

kiota-http-dotnet/src/Middleware/CompressionHandler.cs

Line 36 in 7048ed2

if(request.GetRequestOption<ObservabilityOptions>() is ObservabilityOptions obsOptions)

Also we should check what we're doing in abstractions and authentication azure, I'm pretty sure we're creating new activity sources and disposing them for each request as well.

It looks like the pattern is different in the abstractions/auth libs as the ActivitySource instance are initiliazed in the ctor or static variable so they wouldn't go through the dispose cycle on each method call(only the Activity instances would).

Authentication lib - https://github.com/microsoft/kiota-authentication-azure-dotnet/blob/6a5ad5e623be61d7a206b198aeed9cbed04b4bbf/src/AzureIdentityAccessTokenProvider.cs#L48
Abstractions - https://github.com/microsoft/kiota-abstractions-dotnet/blob/bbdfa1f5a7f63fdd804485a5a62b7f4fd1312353/src/authentication/ApiKeyAuthenticationProvider.cs#L44
- https://github.com/microsoft/kiota-abstractions-dotnet/blob/bbdfa1f5a7f63fdd804485a5a62b7f4fd1312353/src/RequestInformation.cs#L249

Thinking about this, we could probably resolve this by simply using a similar pattern by moving the initialization of the ActivitySource instances (to the constructor or static class variable) as they are the cause of the lock contention. What do you think?

[baywet]

yeah I think that a static registry should do the trick here, want to send a PR for that change so we have a basis for discussion?

[andrueastman]

yeah I think that a static registry should do the trick here, want to send a PR for that change so we have a basis for discussion?

No worries will look into this.