System.Diagnostics Tracing APIs (#35220) by tarekgh · Pull Request #35326 · dotnet/runtime

tarekgh · 2020-04-23T04:23:44Z

This change is porting the added System.Diagnostics APIs to preview 4. These APIs are very important to fill the gap between .NET Libraries and OpenTelemtry. There are some teams (application insight and Azure Monitoring) waiting to try the new APIs as early as we can provide it. It will be good if we can have these APIs included in preview 4 for early adaptors.

Original PR merged to master: #35220
Original issue tracking the work: #31373
The design issue for the APIs: dotnet/designs#98

CC @noahfalk

* System.Diagnostics Tracing APIs * address the feedback

Dotnet-GitSync-Bot · 2020-04-23T04:23:48Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

ghost · 2020-04-23T04:23:51Z

Tagging subscribers to this area: @tarekgh, @tommcdon
Notify danmosemsft if you want to be subscribed.

tarekgh · 2020-04-23T04:26:05Z

@ericstj @Anipik please let me know if there is anything else I should do to get this to preview 4. Thanks!

stephentoub · 2020-04-23T15:14:33Z

+        private static readonly IEnumerable<ActivityLink> s_emptyLinks = new ActivityLink[0];
+        private static readonly IEnumerable<ActivityEvent> s_emptyEvents = new ActivityEvent[0];
 #pragma warning restore CA1825
+        private static ActivitySource s_defaultSource = new ActivitySource(string.Empty);


Suggested change

private static ActivitySource s_defaultSource = new ActivitySource(string.Empty);

private static readonly ActivitySource s_defaultSource = new ActivitySource(string.Empty);

I'll do that.

stephentoub · 2020-04-23T15:15:27Z

+        private string? _displayName;
+
+        /// <summary>
+        /// Kind describes the relationship between the Activity, its parents, and its children in a Trace.


Suggested change

/// Kind describes the relationship between the Activity, its parents, and its children in a Trace.

/// Gets or sets the relationship between the Activity, its parents, and its children in a Trace.

I'll do that.

stephentoub · 2020-04-23T15:16:43Z

+        /// <summary>
+        /// DisplayName is name mainly intended to be used in the UI and not necessary has to be
+        /// same as OperationName.
+        /// </summary>


Suggested change

/// <summary>

/// DisplayName is name mainly intended to be used in the UI and not necessary has to be

/// same as OperationName.

/// </summary>

/// <summary>Gets or sets the display name of the Activity</summary>

/// <remarks>

/// DisplayName is intended to be used in a user interface and need not be the same as OperationName.

/// </remarks>

I'll do that.

stephentoub · 2020-04-23T15:17:51Z

+        public string DisplayName
+        {
+            get => _displayName ?? OperationName;
+            set => _displayName = value;


Is null expected / allowed? If you want to permit null, you should add [AllowNull] to the property. If you don't want to permit null, ideally this would throw, e.g. set => _displayName = value ?? throw new ArgumentNullException(nameof(value));

I'll change this to
set => _displayName = value ?? throw new ArgumentNullException(nameof(value));

stephentoub · 2020-04-23T15:19:09Z

+        /// <summary>
+        /// Get the ActivitySource object associated with this Activity.
+        /// All Activities created from public constructors will have a singleton source where the source name is empty string.
+        /// Otherwise, the source will be holding the object that created the Activity through ActivitySource.StartActivity.
+        /// </summary>


Suggested change

/// <summary>

/// Get the ActivitySource object associated with this Activity.

/// All Activities created from public constructors will have a singleton source where the source name is empty string.

/// Otherwise, the source will be holding the object that created the Activity through ActivitySource.StartActivity.

/// </summary>

/// <summary>Get the ActivitySource object associated with this Activity.</summary>

/// <remarks>

/// All Activities created from public constructors will have a singleton source where the source name is an empty string.

/// Otherwise, the source will hold the object that created the Activity through ActivitySource.StartActivity.

/// </remarks>

I'll do that

stephentoub · 2020-04-23T15:46:54Z

+        private static SynchronizedList<ActivityListener> s_allListeners = new SynchronizedList<ActivityListener>();
+        private SynchronizedList<ActivityListener>? _listeners;
+
+        private ActivitySource() { throw new InvalidOperationException(); }


Why is this needed?

I left it when I was doing some internal testing. It was not intended to stay. Thanks for the catch.

stephentoub · 2020-04-23T16:48:20Z

+    // Causion: We can have the action executed on the same item more than once which is ok in our scenarios.
+    internal class SynchronizedList<T>
+    {
+        private List<T> _list;


Suggested change

private List<T> _list;

private readonly List<T> _list;

will do it.

stephentoub · 2020-04-23T16:48:29Z

+    // SynchronizedList<T> is a helper collection which ensure thread safety on the collection
+    // and allow enumerating the collection items and execute some action on the enumerated item and can detect any change in the collection
+    // during the enumeration which force restarting the enumeration again.
+    // Causion: We can have the action executed on the same item more than once which is ok in our scenarios.


Suggested change

// Causion: We can have the action executed on the same item more than once which is ok in our scenarios.

// Caution: We can have the action executed on the same item more than once which is ok in our scenarios.

why English lack having Causion :-) interesting the editor spell checker didn't complain. of course, I'll fix it. thanks.

stephentoub · 2020-04-23T16:49:36Z

+        {
+            lock (_list)
+            {
+                if (!_list.Contains(item))


How big do we expect these lists to be? And more importantly, what's the worst case? N calls to AddIfNotExist results in O(N^2) work here.

We are expecting the lists to be short. For activity sources, we expect would be one per component if the component publishing any tracing data. For the listeners, usually, the list will be shorter as in main scenario cases we expect one listener for who is interested to get the Activity notification (e.g. OpenTelemetry for instance). usual apps/Libraries not going to create listeners.

stephentoub · 2020-04-23T16:51:34Z

+                    {
+                        version = _version;
+                        index = 0;
+                        continue;


So if there's a mutation we start over? Hence the warning about potentially invoking delegates multiple times? This means if one bad actor was repeatedly adding / removing / adding / removing / etc. while someone else was enumerating, that someone else might never end?

Yes, it is safe in the calling scenarios. We are calling the listeners to detect if we need to create the Activity. I am not seeing a problem calling the listener multiple times with the same parameters multiple parameters when someone else mutates the list.
The lists we have are the ActivtySource lists and listeners inside the activity source. If there is a bad actor you can do many more bad things creating a lot of Activity sources and listeners. or even in the listening can spend a long time in the execution.

tarekgh · 2020-04-23T16:58:37Z

@stephentoub could you please tell which of your comments should block getting this to preview 4? I am asking because I can address your comments on master instead and safely catch preview 4.

stephentoub · 2020-04-23T17:03:43Z

I didn't realize this was already merged to master.

It'd be good to at least reason about the last two comments.

danmoseley · 2020-04-23T17:32:50Z

Approved for P4, merging today would be ideal -- cc @Anipik

tarekgh · 2020-04-23T20:21:19Z

Talked to @stephentoub offline and learned none of the feedback comments is P4 blocker. So, I'll go ahead and merge this one and will address the feedback on the master. Thanks, @stephentoub for the feedback.

System.Diagnostics Tracing APIs (dotnet#35220)

fe6a756

* System.Diagnostics Tracing APIs * address the feedback

Dotnet-GitSync-Bot added area-System.Diagnostics.Tracing new-api-needs-documentation labels Apr 23, 2020

tarekgh added the Servicing-consider Issue for next servicing release review label Apr 23, 2020

tarekgh added this to the 5.0 milestone Apr 23, 2020

stephentoub reviewed Apr 23, 2020

View reviewed changes

danmoseley added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Apr 23, 2020

tarekgh merged commit 88c069b into dotnet:release/5.0-preview4 Apr 23, 2020

tarekgh mentioned this pull request Apr 28, 2020

Address the System.Diagnostics Feedback #35582

Merged

pjanotti mentioned this pull request May 6, 2020

Step 1 - Use new Activity to Replace OT Span open-telemetry/opentelemetry-dotnet#660

Merged

ghost locked as resolved and limited conversation to collaborators Dec 9, 2020

tarekgh deleted the release/5.0-preview4 branch March 15, 2021 19:41

	private static ActivitySource s_defaultSource = new ActivitySource(string.Empty);
	private static readonly ActivitySource s_defaultSource = new ActivitySource(string.Empty);

	/// Kind describes the relationship between the Activity, its parents, and its children in a Trace.
	/// Gets or sets the relationship between the Activity, its parents, and its children in a Trace.

	// Causion: We can have the action executed on the same item more than once which is ok in our scenarios.
	// Caution: We can have the action executed on the same item more than once which is ok in our scenarios.

Conversation

tarekgh commented Apr 23, 2020

Uh oh!

Dotnet-GitSync-Bot commented Apr 23, 2020

Uh oh!

ghost commented Apr 23, 2020

Uh oh!

tarekgh commented Apr 23, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tarekgh commented Apr 23, 2020

Uh oh!

stephentoub commented Apr 23, 2020

Uh oh!

danmoseley commented Apr 23, 2020

Uh oh!

tarekgh commented Apr 23, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants