search

dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
14.56k stars 4.54k forks source link

Question around DiagnosticSource & DiagnosticListener & execution context #104427

Open dxynnez opened 3 days ago

dxynnez commented 3 days ago

I always think when using DiagnosticSource to write an event, it would not wait for the Listener to complete processing the event. But recently I found code like this: https://github.com/open-telemetry/opentelemetry-dotnet-contrib/blob/38cd9638966bc7d97470d30bde9e232faa5e6f3f/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs#L63C1-L64C1 and this: https://github.com/Azure/azure-cosmos-dotnet-v3/blob/5287ffb4ba8f1271c38e3b42365fa5fcb8ba6cc0/Microsoft.Azure.Cosmos.Samples/Usage/CustomDiagnosticAndEventListener/CustomDiagnosticAndEventListener.cs#L68C1-L69C1

They both seem to assume that the execution will wait for the listener to finish the processing, and hence it's safe to rely on the Activity.Current to retrieve other contextual information that's not provided in the event payload.

  1. I want to understand if it's true that when using DiagnosticSource to write an event, it will wait for the listener to complete the processing and hence the Activity.Current will always be in the same state when the event is written.

  2. Another thing is, is the callback on the listener guaranteed to be executed in the same execution context as where the event is written? I asked as the Activity.Current is, essentially an AyncLocal. If the callback might get invoked in a different execution context then we will not see the same AsyncLocal value.

Last thing is, for code: https://github.com/open-telemetry/opentelemetry-dotnet-contrib/blob/38cd9638966bc7d97470d30bde9e232faa5e6f3f/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs#L95

3) The piece of code subscribes to the HttpHandlerDiagnosticListener and mutate the HttpRequestMessage in the event payload - it expects the HttpRequestMessage instance it mutated will be the one getting sent out later by the DiagnosticsHandler. But to my understanding, that is an implementation details of the runtime DiagnosticsHandler and there is no contract for that. Should I consider such approach fragile and avoid it?

Any pointers would be greatly appreciated!

dotnet-policy-service[bot] commented 3 days ago

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

tarekgh commented 3 days ago

@noahfalk could you please help with the mentioned questions? Thanks!