-
Notifications
You must be signed in to change notification settings - Fork 4.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add ActivitySource Tags #102678
Add ActivitySource Tags #102678
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,7 @@ | |
// The .NET Foundation licenses this file to you under the MIT license. | ||
|
||
using System.Collections.Generic; | ||
using System.ComponentModel; | ||
using System.Runtime.CompilerServices; | ||
using System.Threading; | ||
|
||
|
@@ -13,16 +14,40 @@ public sealed class ActivitySource : IDisposable | |
private static readonly SynchronizedList<ActivityListener> s_allListeners = new SynchronizedList<ActivityListener>(); | ||
private SynchronizedList<ActivityListener>? _listeners; | ||
|
||
/// <summary> | ||
/// Construct an ActivitySource object with the input name | ||
/// </summary> | ||
/// <param name="name">The name of the ActivitySource object</param> | ||
public ActivitySource(string name) : this(name, version: "", tags: null) {} | ||
|
||
/// <summary> | ||
/// Construct an ActivitySource object with the input name | ||
/// </summary> | ||
/// <param name="name">The name of the ActivitySource object</param> | ||
/// <param name="version">The version of the component publishing the tracing info.</param> | ||
public ActivitySource(string name, string? version = "") | ||
[EditorBrowsable(EditorBrowsableState.Never)] | ||
public ActivitySource(string name, string? version = "") : this(name, version, tags: null) {} | ||
|
||
/// <summary> | ||
/// Construct an ActivitySource object with the input name | ||
/// </summary> | ||
/// <param name="name">The name of the ActivitySource object</param> | ||
/// <param name="version">The version of the component publishing the tracing info.</param> | ||
/// <param name="tags">The optional ActivitySource tags.</param> | ||
public ActivitySource(string name, string? version = "", IEnumerable<KeyValuePair<string, object?>>? tags = default) | ||
{ | ||
Name = name ?? throw new ArgumentNullException(nameof(name)); | ||
Version = version; | ||
|
||
// Sorting the tags to make sure the tags are always in the same order. | ||
// Sorting can help in comparing the tags used for any scenario. | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what is the usecase for the sorting here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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.
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Because Activity.SetTag already does de-duplication, setting some precedence! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
Tags = tagList.AsReadOnly(); | ||
} | ||
|
||
s_activeSources.Add(this); | ||
|
||
if (s_allListeners.Count > 0) | ||
|
@@ -54,6 +79,11 @@ public ActivitySource(string name, string? version = "") | |
/// </summary> | ||
public string? Version { get; } | ||
|
||
/// <summary> | ||
/// Returns the tags associated with the ActivitySource. | ||
/// </summary> | ||
public IEnumerable<KeyValuePair<string, object?>>? Tags { get; } | ||
|
||
/// <summary> | ||
/// Check if there is any listeners for this ActivitySource. | ||
/// This property can be helpful to tell if there is no listener, then no need to create Activity object | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.