Libraries like HttpClient use legacy activity creation, requiring hacks in OpenTelemetry SDK

[denisivan0v]

Problem Description

Distributed tracing can be supported by .NET applications by instrumenting them using System.Diagnostics.Activity API. In addition, there are two APIs to be used to produce and consume events generated by instrumented application:

• System.Diagnostics.DiagnosticSource/System.Diagnostics.DiagnosticListener
• System.Diagnostics.ActivitySource/System.Diagnostics.ActivityListener

The latter API were introduced recently and reflects OpenTelemetry specifications for tracing. By contrast the former is the legacy .NET-specific API were used to implement instrumentation for distributed tracing initially.

Currently, HttpClient still leverages System.Diagnostics.DiagnosticSource for distributed tracing instrumentation which requires OpenTelemetry SDK to use hacks and reflection. Going forward, this may also make it harder to stay up-to-date with progress done in OpenTelemetry specifications bringing even more complexity to OpenTelemetry SDK. For example, HTTP retries and redirects implementation require SpanLink objects (as per proposal here) that can only be supported by System.Diagnostics.ActivitySource.

Additional Information

• PR: Scope and roadmap for bringing the existing HTTP semantic conventions for tracing to an initial stable state.
• PR: Implementation of HTTP retries and redirects instrumentation in .NET OpenTelemetry SDK.

[ghost]

Tagging subscribers to this area: @tarekgh, @tommcdon, @pjanotti, @safern
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Problem Description

Distributed tracing can be supported by .NET applications by instrumenting them using System.Diagnostics.Activity API. In addition, there are two APIs to be used to produce and consume events generated by instrumented application:

• System.Diagnostics.DiagnosticSource/System.Diagnostics.DiagnosticListener
• System.Diagnostics.ActivitySource/System.Diagnostics.ActivityListener

The latter API were introduced recently and reflects OpenTelemetry specifications for tracing. By contrast the former is the legacy .NET-specific API were used to implement instrumentation for distributed tracing initially.

Currently, HttpClient still leverages System.Diagnostics.DiagnosticSource for distributed tracing instrumentation which requires OpenTelemetry SDK to use hacks and reflection. Going forward, this may also make it harder to stay up-to-date with progress done in OpenTelemetry specifications bringing even more complexity to OpenTelemetry SDK. For example, HTTP retries and redirects implementation require SpanLink objects (as per proposal here) that can only be supported by System.Diagnostics.ActivitySource.

Additional Information

• PR: Scope and roadmap for bringing the existing HTTP semantic conventions for tracing to an initial stable state.
• PR: Implementation of HTTP retries and redirects instrumentation in .NET OpenTelemetry SDK.

Author:	denisivan0v
Assignees:	-
Labels:	area-System.Diagnostics.Tracing, untriaged
Milestone:	-

[MihaZupan]

We have tried adding ActivitySource support in .NET 6.0 (#54437), but the PR was reverted as it caused networking test failures (#55006, #54778) - the why is currently unclear.

Allowing more functionality that is only available to ActivitySource is a good reason to add it.

HttpClient still leverages System.Diagnostics.DiagnosticSource for distributed tracing instrumentation which requires OpenTelemetry SDK to use hacks and reflection

My understanding is that instrumentations like OpenTelemetry and AppInsights would still rely on DiagnosticSource and such hacks even if ActivitySource support were added.
They currently rely on inspecting the HttpRequestMessage object that comes with DiagnosticSource events, and as far as I am aware, there is currently no way for ActivitySource to expose such context to its listeners. Is that not the case?

cc: @dotnet/ncl as areas overlap here

[ghost]

Tagging subscribers to this area: @dotnet/area-system-diagnostics-activity
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Problem Description

Distributed tracing can be supported by .NET applications by instrumenting them using System.Diagnostics.Activity API. In addition, there are two APIs to be used to produce and consume events generated by instrumented application:

• System.Diagnostics.DiagnosticSource/System.Diagnostics.DiagnosticListener
• System.Diagnostics.ActivitySource/System.Diagnostics.ActivityListener

The latter API were introduced recently and reflects OpenTelemetry specifications for tracing. By contrast the former is the legacy .NET-specific API were used to implement instrumentation for distributed tracing initially.

Currently, HttpClient still leverages System.Diagnostics.DiagnosticSource for distributed tracing instrumentation which requires OpenTelemetry SDK to use hacks and reflection. Going forward, this may also make it harder to stay up-to-date with progress done in OpenTelemetry specifications bringing even more complexity to OpenTelemetry SDK. For example, HTTP retries and redirects implementation require SpanLink objects (as per proposal here) that can only be supported by System.Diagnostics.ActivitySource.

Additional Information

• PR: Scope and roadmap for bringing the existing HTTP semantic conventions for tracing to an initial stable state.
• PR: Implementation of HTTP retries and redirects instrumentation in .NET OpenTelemetry SDK.

Author:	denisivan0v
Assignees:	-
Labels:	untriaged, area-System.Diagnostics.Activity
Milestone:	-

[ManickaP]

Follow up from the meeting:

1. @ManickaP: bring back Add ActivitySource support to DiagnosticsHandler #54437 to the 7.0 (main) branch
2. @denisivan0v: follow this change in OT implementation and see how it can be useful
3. After that, we can see what other API changes we'd need to support Activity creation with links (for the redirects scenario)
   a. Changes to ActivitySource to be able to access HttpRequestMessage associated with the activity - @MihaZupan did I get this right? Is this what is missing and why we still need DiagnosticSource?
   b. Changes to be able to access HttpResponseMessage associated with the request - @denisivan0v what scenarios need the response object, do we have examples?
   c. Potentially: changes to SocketsHttpHandler/DistributedContextPropagator to be able to create Activity manually and override default DiagnosticsHandler behavior, i.e. fully customize Activity creation.

[ghost]

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Problem Description

Distributed tracing can be supported by .NET applications by instrumenting them using System.Diagnostics.Activity API. In addition, there are two APIs to be used to produce and consume events generated by instrumented application:

• System.Diagnostics.DiagnosticSource/System.Diagnostics.DiagnosticListener
• System.Diagnostics.ActivitySource/System.Diagnostics.ActivityListener

The latter API were introduced recently and reflects OpenTelemetry specifications for tracing. By contrast the former is the legacy .NET-specific API were used to implement instrumentation for distributed tracing initially.

Currently, HttpClient still leverages System.Diagnostics.DiagnosticSource for distributed tracing instrumentation which requires OpenTelemetry SDK to use hacks and reflection. Going forward, this may also make it harder to stay up-to-date with progress done in OpenTelemetry specifications bringing even more complexity to OpenTelemetry SDK. For example, HTTP retries and redirects implementation require SpanLink objects (as per proposal here) that can only be supported by System.Diagnostics.ActivitySource.

Additional Information

• PR: Scope and roadmap for bringing the existing HTTP semantic conventions for tracing to an initial stable state.
• PR: Implementation of HTTP retries and redirects instrumentation in .NET OpenTelemetry SDK.

Author:	denisivan0v
Assignees:	-
Labels:	area-System.Net.Http, untriaged
Milestone:	-

[MihaZupan]

a. Changes to ActivitySource to be able to access HttpRequestMessage associated with the activity - @MihaZupan did I get this right? Is this what is missing and why we still need DiagnosticSource?
b. Changes to be able to access HttpResponseMessage associated with the request - @denisivan0v what scenarios need the response object, do we have examples?

There are a few steps here:

1. Activity creation
   • This is where ActivitySource would fit in.
   • To be more useful, it would need access to the HttpRequestMessage and have full control over the Activity creation. Currently it does not have access to the request - that is only available to DiagnosticSource events.Activity has already been created.
2. Injecting headers into the request
   • This is done by either the call to DistributedContextPropagator.Inject or as part of creating the Activity in step 1.
   • At this point the propagator has access to the HttpRequestMessage, but is by definition too late to influence Activity creation.
3. Reporting the result based on the response
   • DiagnosticSource is currently the only mechanism providing a hook/callback when the response is received. It also gives access to the HttpResponseMessage and Exception.

So with the current API shape, DiagnosticSource is still required for:

1. Providing access to the HttpRequestMessage, HttpResponseMessage and Exception.
2. Providing a hook before the request is sent & after the response is received.

Just by adding ActivitySource, we don't eliminate any of the above.

---

All of this context is available to a custom DelegatingHandler.
The reason instrumentations currently prefer using events instead is that injecting a handler into the HttpClient pipeline requires user interaction (on top of just calling 1 method to register in DI).

If the existing events are not sufficient, should we instead look at providing a callback that allows instrumentations to inject a custom handler?
That would decouple instrumentations from internal implementation details of DiagnosticsHandler, allowing them to simplify the logic and remove all reflection hacks. They would also have as much context as DiagnosticsHandler can have when it comes to creating the activity.

[cijothomas]

So with the current API shape, DiagnosticSource is still required for:

Providing access to the HttpRequestMessage, HttpResponseMessage and Exception.
Providing a hook before the request is sent & after the response is received.
Just by adding ActivitySource, we don't eliminate any of the above.

DiagnosticSource callbacks are still required for OpenTelemetry (and for other scenarios as well)

Shifting from creating activity using legacy way (i.e new Activity("name")), to the ActivitySource way (i.e activitySource.startActivity(Kind.Client,..)) is the only ask. This would allow instrumentations in OpenTelemetry to avoid current hacks its doing to:

1. Set ActivityKind from internal to Client using reflection. (https://github.com/open-telemetry/opentelemetry-dotnet/blob/main/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs#L131)
2. Set ActivitySource from empty to proper source https://github.com/open-telemetry/opentelemetry-dotnet/blob/main/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs#L130
3. Legacy activity creation does not automatically run samplers. OpenTelemetry does a hack to run samplers : https://github.com/open-telemetry/opentelemetry-dotnet/blob/main/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs#L130
   This hack means Samplers don't always get the right info it is supposed to get (Instrumentations based on legacy activity creation provide default Kind to sampler open-telemetry/opentelemetry-dotnet#1947)

[tarekgh]

@denisivan0v I think we can close this now as it is addressed by @ManickaP PR?

[tarekgh]

@denisivan0v can we close this issue? or are we waiting for something more?

[ManickaP]

I'll close this since the PR is in. We can reopen and revisit if something turns out to be missing.