Add ActivitySource.CreateActivity methods #48374

tarekgh · 2021-02-17T01:15:33Z

This PR should be addressing #43853 too.

dotnet-issue-labeler · 2021-02-17T01:15:37Z

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 · 2021-02-17T01:15:38Z

Tagging subscribers to this area: @tarekgh
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #42784

This PR should be addressing #43853 too.

Author:	tarekgh
Assignees:	-
Labels:	`area-System.Diagnostics.Activity`, `new-api-needs-documentation`
Milestone:	-

tarekgh · 2021-02-17T01:16:29Z

#47313

noahfalk · 2021-02-17T03:36:02Z

src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs

+                activity.StartTimeUtc = startTime.UtcDateTime;
+            }
+
+            if (startIt)


Does anything prevent us from doing?

Suggested change

if (startIt)

if (startIt)

{

Start();

}

It feels odd to be replicating all this start code.

I think it is possible. The only different case would be when someone calling StartActivity and passing ActivityContext with a default span Id. we used to keep Activity.Parent=null but if we call Activity.Start() it would set Activity.Parent to the Activity.Current. I am not sure though if this will be ok (or should be the right behavior). what you think?

In theory spanId = 0 is an invalid parent context. We should probably take a peak at the spec and chat with the OT folks to see if they have any input on what error handling behavior is desirable. I don't recall us ever defining the behavior very precisely but what I see the code currently do is give the bad id (in string or ActivityContext form) to the sampler as-is. Later in CreateAndStart we potentially ignore the id (if it was in string form) and create a new id based on IdFormat. This runs afoul of our sampler TraceId == Activity.TraceId invariant the same as above.

I don't care too much about the factoring on its own, but I am glad thinking about it sussed out issues like this one : )

noahfalk · 2021-02-17T03:46:32Z

...aries/System.Diagnostics.DiagnosticSource/ref/System.Diagnostics.DiagnosticSourceActivity.cs

@@ -146,6 +146,9 @@ public sealed class ActivitySource : IDisposable
        public string Name { get { throw null; } }
        public string? Version { get { throw null; } }
        public bool HasListeners() { throw null; }
+        public System.Diagnostics.Activity? CreateActivity(string name, System.Diagnostics.ActivityKind kind) { throw null; }
+        public System.Diagnostics.Activity? CreateActivity(string name, System.Diagnostics.ActivityKind kind, System.Diagnostics.ActivityContext parentContext, System.Collections.Generic.IEnumerable<System.Collections.Generic.KeyValuePair<string, object?>>? tags = null, System.Collections.Generic.IEnumerable<System.Diagnostics.ActivityLink>? links = null) { throw null; }
+        public System.Diagnostics.Activity? CreateActivity(string name, System.Diagnostics.ActivityKind kind, string parentId, System.Collections.Generic.IEnumerable<System.Collections.Generic.KeyValuePair<string, object?>>? tags = null, System.Collections.Generic.IEnumerable<System.Diagnostics.ActivityLink>? links = null) { throw null; }


Weren't we going to handle #43853 by adding an IdFormat parameter here?

No. After discussing this in the design review, @pakrym felt the other proposal is not needed and that can be handled by this CreateActivity proposal. After creating the activity, they can set the IdFormat on such created Activity before starting it.

Unforetunately that doesn't work. CreateActivity() calls the sampling callback, sampling callback has access to the ID, generating that ID has already fixed the id format. Using SetIdFormat() after CreateActivity() should probably throw an exception if it doesn't already.

public Activity SetIdFormat(ActivityIdFormat format) { if (_id != null || _spanId != null) { NotifyError(new InvalidOperationException(SR.SetFormatOnStartedActivity)); } else { IdFormat = format; } return this; }

If the Activity not started, _id and _spanId would still be nulls. otherwise Activity.Start() would fail too. no?

@noahfalk I have addressed all comments. The remaining point is this one which related to IdFormat. I don't think we should fix the Ids before starting the Activity. Please let me know if you think otherwise.

The caller is passing default context, there is no parent (Activity.Current), and the listeners didn't try to access the trace Id from the provided ActivityCreationOptions.

This case seems very real. Think of a worker app uploading a blob. There is no parent and all spans are exported so TraceId is not accessed.

This case seems very real. Think of a worker app uploading a blob. There is no parent and all spans are exported so TraceId is not accessed.

Yes, agree. But using CreateActivity with the current design will make it work. right? I mean users can use CreateActivity and SetIdFormat if they need to for this scenario.

@pakrym I had offline discussion with @noahfalk and we propose adding extra Boolean parameter to the CreateActivity called forceW3CId. The reason we didn't have the parameter of ActivityIdFormat type because we want to promote W3C in general and not allow users to try the hierarchal explicitly. Please let's know if you have any concern. I'll send it anyway to the fxdc and will CC you there.

public Activity? CreateActivity(string name, ActivityKind kind); public Activity? CreateActivity( string name, ActivityKind kind, ActivityContext parentContext, IEnumerable<KeyValuePair<string, object?>>? tags = null, IEnumerable<ActivityLink>? links = null, bool forceW3CId = false); // added parameter public Activity? CreateActivity( string name, ActivityKind kind, string parentContext, bool forceW3CId = false, IEnumerable<KeyValuePair<string, object?>>? tags = null, IEnumerable<ActivityLink>? links = null, bool forceW3CId = false); // added parameter

LGTM. If we want to promote W3C can we default the parameter to true? 😄

If we want to promote W3C can we default the parameter to true? 😄

I discussed that with @noahfalk and he preferred not to default the value to W3C as he want to have consistent behavior with StartActivity when passing the exact same parameters.

src/libraries/System.Diagnostics.DiagnosticSource/tests/ActivitySourceTests.cs

…ormat logic

noahfalk

Mostly looked fine but I think we need to fix the closure allocation

noahfalk · 2021-02-22T00:30:01Z

src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivitySource.cs

+
+                // delegate to ensure updating the trace id in other ActivityCreationOption object when a new trace Id is generated.
+                // It is important so all listeners callbacks will always see the same trace Id which the activity will get created with
+                Action<ActivityTraceId, bool> updateTraceId = (traceId, isChangedFromContext) =>


This delegate needs a closure so it will do an allocation that we don't want. However we have a few ways out:

Previously the behavior for ActivityCreationOptions<string>.TraceId is that it always returns default. I suggest we preserve that behavior so there is no need to copy the TraceId across the different contexts.

Previously generating the TraceId was expensive because our random number generator was slow. Maybe now it is fast enough that there is little advantage in trying to generate it lazily? We'd need a benchmark measurement to know for sure.

Ok, I'll go with the #1 to make ActivityCreationOptions<string>.TraceId return default. I'll remove the delegate from the picture.

noahfalk · 2021-02-22T01:53:57Z

src/libraries/System.Diagnostics.DiagnosticSource/tests/ActivitySourceTests.cs

+        //  - If the parentId string is null and parent ActivityContext is default and we have Activity.Current != null, then use Activity.Current.IdFormat. Otherwise, goto next step.
+        //  - If we have non default parent ActivityContext, then use W3C. Otherwise, goto next step.
+        //  - if we have non null parent id string, try to parse it to W3C. If parsing succeeded then use W3C format. otherwise use Hierarchical format.
+        // --------------------------------------------------------------------------------------------------------------------------------------------------------------------------


There is a missing case in the algorithm where we should choose to use default format id:

IdFormat passed to ActivitySource.Create is Unknown (step 1 doesn't apply)

ForceDefaultFormat = false (step 2 doesn't apply)

Activity.Current = null (step 3 doesn't apply)

parent ActivityContext/id are default (steps 4 and 5 don't apply)

right. I handled this in the code :-) but didn't specify this step here. I'll add it here.

noahfalk · 2021-02-22T01:55:28Z

src/libraries/System.Diagnostics.DiagnosticSource/tests/ActivitySourceTests.cs

+        [InlineData($"Unknown,          Unknown,            W3C,                null,       true,                   false,              W3C,                    W3C")]
+        [InlineData($"Unknown,          Unknown,            Hierarchical,       null,       true,                   false,              Hierarchical,           Hierarchical")]
+
+        [InlineData($"Unknown,          Unknown,            Unknown,            NonWC3Id,   true,                   false,              Hierarchical,           Hierarchical")]


Nit - the constants were inconsistent in the column:

Suggested change

[InlineData($"Unknown, Unknown, Unknown, NonWC3Id, true, false, Hierarchical, Hierarchical")]

[InlineData($"Unknown, Unknown, Unknown, NonWC3, true, false, Hierarchical, Hierarchical")]

will fix that.

noahfalk · 2021-02-22T02:04:17Z

src/libraries/System.Diagnostics.DiagnosticSource/tests/ActivitySourceTests.cs

+        // --------------------------------------------------------------------------------------------------------------------------------------------------------------------------
+
+        //           -----------------------------------------------------------------------------------------------------------------------------------------------------------------
+        //           Default Id Format, Id fromat to force, Parent id format,   parent id,  default parent context, validate trace Id,  expected create format, expected start format


Suggested change

// Default Id Format, Id fromat to force, Parent id format, parent id, default parent context, validate trace Id, expected create format, expected start format

// Activity.DefaultIdFormat, Activity.Create(idFormat), Activity.Current.IdFormat, parent id, default parent context, validate trace Id, expected create format, expected start format

// (ForceIdFormat=true)

I got the wrong idea about what these columns meant until I reverse engineered it from the source. This might help clarify the intent.

noahfalk · 2021-02-22T02:13:20Z

src/libraries/System.Diagnostics.DiagnosticSource/tests/ActivitySourceTests.cs

+
+                using ActivityListener listener = new ActivityListener();
+                listener.ShouldListenTo = (activitySource) => object.ReferenceEquals(aSource, activitySource);
+                listener.SampleUsingParentId = (ref ActivityCreationOptions<string> activityOptions) =>


Lets add a 2nd listener that only implements the Sample callback. This will let us confirm that Sample is always called for W3C formatted activities and never called for Hierarchal ones.

noahfalk · 2021-02-22T02:19:46Z

src/libraries/System.Diagnostics.DiagnosticSource/tests/ActivitySourceTests.cs

+        [InlineData($"Unknown,          Hierarchical,       Unknown,            null,       false,                  false,              Hierarchical,           W3C")]
+        [InlineData($"Unknown,          W3C,                Unknown,            null,       false,                  false,              W3C,                    W3C")]
+
+        [InlineData($"Unknown,          W3C,                Unknown,            null,       true,                   true,               W3C,                    Default")]


We should have the test case where a non-W3C string id gets passed as the parentId, the child does get W3C format, and the ActivityCreationOptions<ActivityContext>.TraceId property does get viewed inside the Sample callback. That case wasn't previously calling Sample and we need to ensure that now it does work correctly.

You mean when the listener is not providing SampleUsingParentId. right?

noahfalk · 2021-02-22T02:21:59Z

src/libraries/System.Diagnostics.DiagnosticSource/tests/ActivitySourceTests.cs

+                listener.SampleUsingParentId = (ref ActivityCreationOptions<string> activityOptions) =>
+                {
+                    if (checkTraceId)
+                        traceId = activityOptions.TraceId;


Assuming we keep the previous behavior for SampleWithParentId, activityOptions.TraceId should always return default here.

To clarify, it will return default only in the case the parent Id string cannot parsed to W3C. right?

I was looking at the code more closely and may have misinterpretted the old behavior, trying to review more closely now : )

I did misinterpret and regardless I think the way you changed it will be fine. The behavior of ActivityCreationOptions<string>.TraceId is a bit weird, but this matches it's previous weird behavior and breaking change isn't necessarily any better : )

noahfalk · 2021-02-22T02:26:07Z

src/libraries/System.Diagnostics.DiagnosticSource/tests/ActivitySourceTests.cs

+        [InlineData($"Unknown,          Unknown,            Unknown,            null,       false,                  true,               W3C,                    W3C")]
+
+        [ConditionalTheory(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))]
+        public void TestIdFormats(string data)


Nice test : )

tarekgh · 2021-02-22T20:27:38Z

@noahfalk I believe I have addressed all your feedback with the last commit. please let me know if you have any more comments or we good to go. Thanks!

noahfalk

LGTM, thanks Tarek!

Add ActivitySource.CreateActivity methods

e91b69a

dotnet-issue-labeler bot added area-System.Diagnostics.Activity new-api-needs-documentation labels Feb 17, 2021

tarekgh requested a review from noahfalk February 17, 2021 01:16

noahfalk reviewed Feb 17, 2021

View reviewed changes

tarekgh added 2 commits February 16, 2021 21:07

Address the feedback

92c5a64

Add IdFormat parameter to ActivitySource.Create and fix the whole IdF…

29fde6f

…ormat logic

noahfalk reviewed Feb 22, 2021

View reviewed changes

Address @noahfalk feedback

9471f9a

noahfalk approved these changes Feb 23, 2021

View reviewed changes

tarekgh merged commit 970d347 into dotnet:master Feb 23, 2021

tarekgh deleted the AddActivitySourceCreateMethods branch February 23, 2021 00:49

shirhatti mentioned this pull request Feb 23, 2021

Migrate to ActivitySource dotnet/aspnetcore#30089

Merged

ghost locked as resolved and limited conversation to collaborators Mar 25, 2021

	[InlineData($"Unknown, Unknown, Unknown, NonWC3Id, true, false, Hierarchical, Hierarchical")]
	[InlineData($"Unknown, Unknown, Unknown, NonWC3, true, false, Hierarchical, Hierarchical")]

	// Default Id Format, Id fromat to force, Parent id format, parent id, default parent context, validate trace Id, expected create format, expected start format
	// Activity.DefaultIdFormat, Activity.Create(idFormat), Activity.Current.IdFormat, parent id, default parent context, validate trace Id, expected create format, expected start format
	// (ForceIdFormat=true)

Add ActivitySource.CreateActivity methods #48374

Add ActivitySource.CreateActivity methods #48374

Conversation

tarekgh commented Feb 17, 2021

dotnet-issue-labeler bot commented Feb 17, 2021

ghost commented Feb 17, 2021

tarekgh commented Feb 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tarekgh Feb 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tarekgh Feb 17, 2021 • edited Loading

Choose a reason for hiding this comment

tarekgh Feb 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tarekgh Feb 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noahfalk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tarekgh commented Feb 22, 2021 • edited Loading

noahfalk left a comment

Choose a reason for hiding this comment

tarekgh Feb 17, 2021 •

edited

Loading

tarekgh Feb 17, 2021 •

edited

Loading

tarekgh Feb 17, 2021 •

edited

Loading

tarekgh Feb 18, 2021 •

edited

Loading

tarekgh commented Feb 22, 2021 •

edited

Loading