Support Open Telemetry for metrics

[samsp-msft]

As part of being a good CNCF citizen, we need to make sure we interoperate well out of the box for Open Telemetry.

dotnet/runtime#44445

[Tratcher]

Here's a demo we can borrow and insert ourselves into:
https://github.com/shirhatti/trivia

Note: the way we bypass HttpClient and go directly to SocketHttpHandler bypasses the DiagnosticsHandler and may break some of the open telemetry integration. See #292

[samsp-msft]

Does @MihaZupan 's last comment on #292 imply that it should work?

[Tratcher]

I think we only ported a subset of the functionality, enough to add headers, but not enough to track the outbound request as a separate child activity.

[Kahbazi]

How the users are going to consume the open telemetry? Are we going to add the rest of DiagnosticHandler to the http request and let users use OpenTelemetry.Instrumentation.AspNetCore and OpenTelemetry.Instrumentation.Http or we are going to create a new Activity for YARP and create a OpenTelemetry.Instrumentation.YARP package?

[MihaZupan]

Distributed tracing works with YARP, but as Chris mentioned, we will appear as a single node (there is no separation for incoming and outgoing requests, it's seen as a single operation).

Do we know what is the use-case for YARP to have a separate activity for the outgoing request?

[Tratcher]

Having a separate activity would get you more granular timings, right? Or do you rely on the destination to start its own activity?

I'd also expect a separate activity to be useful in retry scenarios.

@Kahbazi I would expect to re-use the OpenTelemetry.Instrumentation.AspNetCore package. I don't know if we'll be able to re-use the http package or not. I'd prefer to re-use it if possible.

[MihaZupan]

Having a separate activity would get you more granular timings, right?

Yes, but only slightly - in YARP the outgoing request starts at about the same time as the incoming one (there are no network delays etc. in between). Retries would be a good scenario tho.

do you rely on the destination to start its own activity

Yes, every part of the system contributes its information.

I would expect to re-use the OpenTelemetry.Instrumentation.AspNetCore package. I don't know if we'll be able to re-use the http package or not.

The AspNetCore part can be reused.

For Http, it could be reused if we mirror the DiagnosticListener events currently used by DiagnosticHandler (same names, same arguments).

[Kahbazi]

Having a separate activity would get you more granular timings, right?

Yes, but only slightly - in YARP the outgoing request starts at about the same time as the incoming one (there are no network delays etc. in between). Retries would be a good scenario tho.

There might be scenarios where there's delay between the incoming request and outgoing requests, like when there is authentication involved, or when there's a ConcurrencyMiddleware for routes.

[samsp-msft]

If the outbound request is queued, in theory there could also be a delay there while it waits for an available connection (eg H2 concurrency limit).

[MihaZupan]

ToDo: Distributed tracing & logging

• Update the ActivityPropagationHandler to create its own child Activity (so that outgoing requests are separated from the incoming request) and log events matching those of Runtime's DiagnosticsHandler
• Test YARP behavior with OpenTelemetry & App insights on 5.0 and 3.1
• Test compatibility of above HTTP instrumentations with a DiagnosticListener/ActivitySource implementation of our handler
• Test ILogger integration behavior with the OpenTelemetry SDK
  • Do we need extra config to get ILogger to capture Span IDs
  • Are logs exported and correlated
• Create samples for wiring up telemetry egress. There shouldn't be any difference here to any other AspNetCore app so pointing to existing docs might be sufficient

Runtime work (dotnet/runtime#31261):

• Updating DiagnosticsHandler to use ActivitySource
• Exposing the type?

Future: Metrics

• Specs yet to finalize
• .NET 6.0 APIs yet to become available
• Do we need different instrumentation / flowing EventCounter data (dimensions?)

[Kahbazi]

Update the ActivityPropagationHandler to create its own child Activity (so that outgoing requests are separated from the incoming request) and log events matching those of Runtime's DiagnosticsHandler

YARP could be used as part of an ASP.NET Core application, and although having matching events as DiagnosticsHandler is necessary, I think having the exact same events but with different name is useful in order to monitor only YARP requests.

[karelz]

Triage: Let's scope it to "just" Metrics (will be in .NET 6.0, hence Backlog here for YARP) -- @MihaZupan can you please file a new issue with leftovers for Logging and Telemetry we should address in YARP 1.0?

[karelz]

Triage: We believe that work done in 8.0 Runtime and in Kestrel covers everything that might be needed for OpenTelemetry to work well.
If we add more, then we would be mirroring those in Runtime/Kestrel.

Closing

@davidfowl did you miss anything else which you would find valuable in YARP?