Add ActivitySource Tags #102678
Add ActivitySource Tags #102678
Conversation
|
Note regarding the |
|
@noahfalk could you please have a look. |
| if (tags is not null) | ||
| { | ||
| var tagList = new List<KeyValuePair<string, object?>>(tags); | ||
| tagList.Sort((left, right) => string.Compare(left.Key, right.Key, StringComparison.Ordinal)); |
There was a problem hiding this comment.
what is the usecase for the sorting here?
Also, should this do de-duplication, given Activity Tags are deduped when set via SetTag?
There was a problem hiding this comment.
what is the usecase for the sorting here?
We apply the same approach for Meter. The reason is to avoid creating duplicate meters with the same parameters and tags in the cached objects used in DI. While ActivitySource is not currently used in DI, implementing this now ensures consistent behavior and eliminates the need for changes if we decide to support ActivitySource/tracing configuration in the future.
should this do de-duplication, given Activity Tags are deduped when set via SetTag?
Any reason? Should this be the responsibility of the callers? We didn't do that with the Meter tags.
There was a problem hiding this comment.
should this do de-duplication, given Activity Tags are deduped when set via SetTag?
Any reason? Should this be the responsibility of the callers? We didn't do that with the Meter tags.
Because Activity.SetTag already does de-duplication, setting some precedence!
For metrics, BCL does not do any deduping of tags!
There was a problem hiding this comment.
I am fine with this if you see it as important. However, I want to understand the scenario better. Who is going to create a new ActivitySource and potentially have duplicate tags? I don't see this as a likely scenario since the creators of the ActivitySource will know exactly which tags they want to use. SetTag scenario is different as different parties can call it on the same activity object in different time. This is not the case for ActivitySource creation.
| public ActivitySource(string name, string? version = "") { throw null; } | ||
| public ActivitySource(string name, string? version = "", System.Collections.Generic.IEnumerable<System.Collections.Generic.KeyValuePair<string, object?>>? tags = default) { throw null; } |
There was a problem hiding this comment.
would we need to do the [System.ComponentModel.EditorBrowsable(System.ComponentModel.EditorBrowsableState.Never)] on this one too when we support schemaUrl as a parameter? (#63651)
There was a problem hiding this comment.
Yes, when adding a new constructor with extra optional parameter, we can add the [System.ComponentModel.EditorBrowsable(System.ComponentModel.EditorBrowsableState.Never)] to the existing one.
I was thinking in the future if we add more options, we may start having ActivitySourceOptions and a constructor take this options object.
|
DiagnosticSourceEventSource has ActivityStart and ActivityStop events. Currently the expectation is that all tags would be located in Activity tags, but now if some instrumentation will using ActivitySource scoped tags instead we need to add those tags to the events to ensure that out-of-process consumers still have all the data available. This is pretty much the same issue as #93767 but for ActivitySource tags instead of Meter tags. Aside from that, LGTM 👍 |
|
That commit added Meter tags to Begin/End instrument reporting events, but it did not add the tags to the value publishing events. So for some events it remains ambiguous and for other events it is not ambiguous any longer. Fixing #93767 I assume is going to add Meter tags (and other data) to the value published events so that they are also not ambiguous. The ActivityStart/Stop events for distributed tracing are analogous to the value published events for metrics. |
|
I think tags are not enough to disambiguate. Yes, we should propagate the tags with the events, but ambiguity can still happen. like creating two meters/instruments with same name, version, scope, and tags. I don't know if we need to care much about that though. I recall it is reported that this happened in a real scenario. Anyway, for the sake of this PR, I'll add the tags to the Start/Stop activity events. |
Its fine if multiple ActivitySource objects with properties set to the same values are indistinguishable via out-of-proc events. That part is a non-goal :) The case I think we need to care about is given some Activity occurred, we should be able to enumerate all the tags, both the Activity-scoped ones and the ActivitySource scoped ones. |
@noahfalk Some random thoughts here. I've been working on a dotnet-monitor prototype to listen out of proc and send that data using OTel SDK. There's other stuff on ActivitySource for example version which I can't get at using the events. In metrics we have instrument published events currently. Could we have something in tracing for activity source published which would fire off the static details when the first activity is started for some source? If we add some unique id/count to the meter/source that could be included on the individual starts/stop/measurement events to tie back to the static data so we wouldn't need to duplicate it every time (expensive). |
|
@CodeBlanch we can discuss this more, but I want to say this is out of scope for this PR. We can use #93767 to discuss this more and look at your scenarios. Or you can log a new issue with your requests, and we can triage it accordingly. |
|
Tarek and I confirmed that the DiagnosticSourceEventSource DSL can be used to define a TRANSFORM_SPEC that evaluates Activity.Source.Tags without needing any explicit changes to the event definitions. |
@CodeBlanch I think you can use the same idea mentioned by @noahfalk in the comment #102678 (comment) to get the ActivitySource version. |
* Add ActivitySource Tags * Fix failing test
Fixes #93795.