Fix issue of multiple instances of OpenTelemetry-Sdk EventSource being created #4586
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #4586 +/- ##
==========================================
- Coverage 85.14% 85.13% -0.01%
==========================================
Files 320 320
Lines 12679 12733 +54
==========================================
+ Hits 10795 10840 +45
- Misses 1884 1893 +9
|
| @@ -58,7 +60,7 @@ public bool TryTransformTag(KeyValuePair<string, object> tag, out T result, int? | |||
| // If an exception is thrown when calling ToString | |||
| // on any element of the array, then the entire array value | |||
| // is ignored. | |||
| OpenTelemetrySdkEventSource.Log.UnsupportedAttributeType(tag.Value.GetType().ToString(), tag.Key); | |||
| LogUnsupportedAttributeType?.Invoke(tag.Value.GetType().ToString(), tag.Key); | |||
There was a problem hiding this comment.
Maybe put "ToString()" under the null check (so we can skip the ToString() call if the callback is null).
There was a problem hiding this comment.
We wouldn't be calling ToString() if the callback is null as we use the conditional operator ? when calling Invoke: LogUnsupportedAttributeType?.Invoke()
| @@ -18,6 +18,8 @@ namespace OpenTelemetry.Internal; | |||
|
|
|||
| internal abstract class TagTransformer<T> | |||
| { | |||
| public static Action<string, string> LogUnsupportedAttributeType = null; | |||
There was a problem hiding this comment.
I feel this is becoming bit complicated - e.g. if some code accidentally update this part, it might cause undefined behavior? How do we prevent other OpenTelemetry components from updating this global state?
There was a problem hiding this comment.
The components that use TagTransformer would have their own instances of its implementation. There is no global state here.
For example, if you were to use ConsoleExporter and OtlpExporter together, each of those exporters would be operating on their instance of TagTransformer as they link this file into their respective dlls.
There was a problem hiding this comment.
Ideally, we should not use the Sdk project to host these utility files which are only used by other components and not the Sdk itself. We could work on moving such files out of the Sdk project as a follow-up PR.
There was a problem hiding this comment.
I think this PR is fine for now, but if we find we run into this general issue we might want to consider a different architecture for shared components.
For example, we have shared components (e.g., TagTransformer, envrironment variable parser, etc) which multiple SDK components consume. Today our architecture is that we effectively compile the shared components into each of the SDK components.
A different architecture might be something like a TagTransformerService which SDK components can get a handle to from the core SDK thereby eliminating the multiple compilations and weird gotchas like we have with EventSource.
But maybe we entertain something like this if we begin to see any recurring friction with the simple approach of this PR.
Fixes #4516
No other projects other than the SDK project should use
OpenTelemetry-SdkEventSource. Currently, we have multiple projects that link toOpenTelemetrySdkEventSourcewhich is causing users to run into the error:An instance of EventSource with Guid {guid} already exists.Moreover, it doesn't make sense for other components to log messages using SDK's EventSource. These components should use their own dedicated EventSource for that.Changes
OpenTelemetrySdkEventSourcefrom other projectsTagTransformerandConfigurationExtensionsto allow components to use their custom EventSource to log warningsMerge requirement checklist
CHANGELOG.mdfiles updated for non-trivial changes