MikeGoldsmith
commented
4 years ago Hi @kimbell.
The context data that you're referring to above is related to span context. I understand the negative feeling towards static state variables, particularly around unit testing. You can elect to not use static state management in unit tests by using local instances of the tracer / spans.
The static AsyncLocal context is useful in larger, more complex code systems where we do not want to burden APIs with having to pass the state objects, parent spans, etc around.
alanwest
This project looks very interesting for our environment. In my case I'm running .NET Core 3.1; I don't care that much about .NET Framework, so the question is very biased.
I did some exploring in the code. Several places I noticed static member variables. One is AsyncLocal, but the rest are plain static.
https://github.com/open-telemetry/opentelemetry-dotnet/blob/ace469d42b037244e409e9cda0e5acbeab461efe/src/OpenTelemetry.Api/Context/DistributedContext.cs#L28
https://github.com/open-telemetry/opentelemetry-dotnet/blob/ace469d42b037244e409e9cda0e5acbeab461efe/src/OpenTelemetry.Api/Context/CorrelationContext.cs#L28
https://github.com/open-telemetry/opentelemetry-dotnet/blob/ace469d42b037244e409e9cda0e5acbeab461efe/src/OpenTelemetry/Context/AsyncLocalDistributedContextCarrier.cs#L28
I use XUnit for running my tests asynchronously; several times over the years things have failed due to static variables remembering state between tests. When I see static member variables, it becomes a read flag for me; especially when they contain Context in the name.
There may be other aspects of the design that eliminates this as an issue that I haven't discovered yet. If this is not an issue, a code comment would be helpful to explain things for future code explorers.