Improve Activity API usability and OpenTelemetry integration (Part 2) #39087

tarekgh · 2020-07-10T17:17:36Z

Dotnet-GitSync-Bot · 2020-07-10T17:17:40Z

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-07-10T17:17:42Z

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

ghost · 2020-07-10T17:18:20Z

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

tarekgh · 2020-07-10T17:19:00Z

dotnet/designs#134

tarekgh · 2020-07-10T17:20:42Z

CC @cijothomas @reyang

noahfalk

LGTM!

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

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

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

…ityApisPart2

...braries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivityTagsCollection.cs

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

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

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

cijothomas

Looks good - added small comments/questions. Would love to see response before marking approval.

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

...braries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivityTagsCollection.cs

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

...braries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivityTagsCollection.cs

cijothomas · 2020-07-14T17:46:30Z

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

@@ -47,6 +47,11 @@ public ActivityListener()
        /// </summary>
        public GetRequestedData<ActivityContext>? GetRequestedDataUsingContext { get; set; }

+        /// <summary>
+        /// Determine if the listener automatically generates a new trace Id before sampling when there is no parent context.


Could you make it explicit that this traceid will be used by the Activity, should we chose to create it?

tarekgh · 2020-07-14T19:03:00Z

runtime (Libraries Build Windows_NT allConfigurations x64 Debug) failure is not related and @joperezr is working to take care with this one.

@Anipik is this the one you are fixing in https://github.com/dotnet/runtime/pull/39298/files?

safern · 2020-07-14T21:31:59Z

Yeah, the PR that fixes the all configurations failure in master is: #39291

tarekgh · 2020-07-14T23:16:57Z

@dotnet/dnceng I am seeing some weird things here. could you please double check if there is anything wrong? basically the PR showing the legs which not completed as "in progress" but when I view it in AzDo I am seeing them queued and not started. I am experiencing some problems with these legs and I had to restart them hoping behave nicely. Thanks for your help.

MattGal · 2020-07-14T23:22:45Z

@dotnet/dnceng I am seeing some weird things here. could you please double check if there is anything wrong? basically the PR showing the legs which not completed as "in progress" but when I view it in AzDo I am seeing them queued and not started. I am experiencing some problems with these legs and I had to restart them hoping behave nicely. Thanks for your help.

Sure, I'll take a look :)

MattGal · 2020-07-14T23:34:28Z

@tarekgh nothing's in that state now, but I think I know what you mean.

Basically what you're seeing is that Github / Azure Devops have called our pool provider which said "sure! I can do this work!" but behind the scenes it's sticking work items into a private Helix queue. So, when you see messages in the build that it's queued but not started, that build queue is just a bit backed up (and/or waiting for the helix agent to clean up its workspace, reboot, and take more work).

If this does end up taking > 90 minutes it will fail (https://github.com/dotnet/core-eng/issues/10136 tracks bringing this up to 120). Mostly we have ~2000 fewer cores than before the pandemic and builds are a place where the pain is commonly felt.

tarekgh · 2020-07-14T23:37:00Z

Thanks @MattGal.

CodeBlanch · 2020-07-15T17:01:36Z

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

-        public System.Diagnostics.Activity? StartActivity(string name, System.Diagnostics.ActivityKind kind, System.Diagnostics.ActivityContext parentContext, System.Collections.Generic.IEnumerable<System.Collections.Generic.KeyValuePair<string, string?>>? tags = null, System.Collections.Generic.IEnumerable<System.Diagnostics.ActivityLink>? links = null, System.DateTimeOffset startTime = default) { throw null; }
-        public System.Diagnostics.Activity? StartActivity(string name, System.Diagnostics.ActivityKind kind, string parentId, System.Collections.Generic.IEnumerable<System.Collections.Generic.KeyValuePair<string, string?>>? tags = null, System.Collections.Generic.IEnumerable<System.Diagnostics.ActivityLink>? links = null, System.DateTimeOffset startTime = default) { throw null; }
+        public System.Diagnostics.Activity? StartActivity(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, System.DateTimeOffset startTime = default) { throw null; }
+        public System.Diagnostics.Activity? StartActivity(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, System.DateTimeOffset startTime = default) { throw null; }


@tarekgh Hey I just noticed working on some code in OTel that you can't specify events in the StartActivity call. You can specify pretty much everything else. Should we add events too?

Also there is no AddLink method. Links you have to specify at creation?

I just noticed working on some code in OTel that you can't specify events in the StartActivity call. You can specify pretty much everything else. Should we add events too?

StartActivity arguments mainly the things needed for sampling decision to decide if want to create the activity or just avoid that and sample it out. If you look at the OT sampling specs, you'll find events are not needed there https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/sdk.md#sampling. But anyway, you can add events after creating the activity.

Also there is no AddLink method. Links you have to specify at creation?

This was proposed before and then got removed because there was a claim that shouldn't allow adding links after creating and starting the activity. OT specs saying During the Span creation user MUST have the ability to record links to other Spans. Linked Spans can be from the same or a different trace.. so, the spec is explicit mentioning the links should be provided during the Span creation. But anyway, later if we find any scenario need to add links after starting the activity, we can consider it.

CC @cijothomas @reyang

What Tarek said is my understanding as well.

That all makes sense. What I'm doing right now is working on a unit test. It needs the Activity created very specifically. So it's not a normal use case. I think I can make it work with reflection. The links are a bit of a challenge. I'll mention you guys on the change once I have it working so you can see what I'm up against, and then we can decide if any changes are warranted.

Improve Activity API usability and OpenTelemetry integration (Part 2)

8ab1677

Dotnet-GitSync-Bot added area-System.Globalization new-api-needs-documentation labels Jul 10, 2020

tarekgh requested a review from noahfalk July 10, 2020 17:17

tarekgh added area-System.Diagnostics.Tracing and removed area-System.Globalization labels Jul 10, 2020

tarekgh added this to the 5.0.0 milestone Jul 10, 2020

Fix NetFx break

5b19cd0

noahfalk approved these changes Jul 13, 2020

View reviewed changes

src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs Show resolved Hide resolved

src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivityListener.cs Outdated Show resolved Hide resolved

cijothomas reviewed Jul 13, 2020

View reviewed changes

src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivityContext.cs Show resolved Hide resolved

tarekgh added 2 commits July 13, 2020 16:47

Merge branch 'master' of https://github.com/dotnet/runtime into Activ…

4ddfeb3

…ityApisPart2

Address the feedback

07642e9

cijothomas reviewed Jul 14, 2020

View reviewed changes

...braries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivityTagsCollection.cs Outdated Show resolved Hide resolved

cijothomas reviewed Jul 14, 2020

View reviewed changes

...braries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivityTagsCollection.cs Outdated Show resolved Hide resolved

cijothomas reviewed Jul 14, 2020

View reviewed changes

...braries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivityTagsCollection.cs Show resolved Hide resolved

cijothomas reviewed Jul 14, 2020

View reviewed changes

src/libraries/System.Diagnostics.DiagnosticSource/tests/ActivitySourceTests.cs Outdated Show resolved Hide resolved

cijothomas reviewed Jul 14, 2020

View reviewed changes

src/libraries/System.Diagnostics.DiagnosticSource/tests/ActivityTests.cs Show resolved Hide resolved

cijothomas reviewed Jul 14, 2020

View reviewed changes

src/libraries/System.Diagnostics.DiagnosticSource/tests/ActivitySourceTests.cs Show resolved Hide resolved

cijothomas suggested changes Jul 14, 2020

View reviewed changes

CodeBlanch reviewed Jul 14, 2020

View reviewed changes

src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs Outdated Show resolved Hide resolved

CodeBlanch reviewed Jul 14, 2020

View reviewed changes

...braries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivityTagsCollection.cs Outdated Show resolved Hide resolved

CodeBlanch reviewed Jul 14, 2020

View reviewed changes

...braries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivityTagsCollection.cs Outdated Show resolved Hide resolved

CodeBlanch reviewed Jul 14, 2020

View reviewed changes

...aries/System.Diagnostics.DiagnosticSource/ref/System.Diagnostics.DiagnosticSourceActivity.cs Show resolved Hide resolved

CodeBlanch reviewed Jul 14, 2020

View reviewed changes

...braries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivityTagsCollection.cs Show resolved Hide resolved

Address the feedback

ba7f50f

Fix NetFX build and add teh requested test

dcd7768

cijothomas reviewed Jul 14, 2020

View reviewed changes

cijothomas approved these changes Jul 14, 2020

View reviewed changes

tarekgh merged commit b895666 into dotnet:master Jul 15, 2020

tarekgh deleted the ActivityApisPart2 branch July 15, 2020 05:07

CodeBlanch reviewed Jul 15, 2020

View reviewed changes

CodeBlanch mentioned this pull request Jul 16, 2020

Cleaning Jaeger Exporter open-telemetry/opentelemetry-dotnet#832

Merged

2 tasks

tarekgh mentioned this pull request Aug 3, 2020

System.Diagnostics.ActivitySource API Addition #40046

Closed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve Activity API usability and OpenTelemetry integration (Part 2) #39087

Improve Activity API usability and OpenTelemetry integration (Part 2) #39087

tarekgh commented Jul 10, 2020

Dotnet-GitSync-Bot commented Jul 10, 2020

ghost commented Jul 10, 2020

ghost commented Jul 10, 2020

tarekgh commented Jul 10, 2020

tarekgh commented Jul 10, 2020

noahfalk left a comment

cijothomas left a comment

cijothomas Jul 14, 2020

tarekgh commented Jul 14, 2020 •

edited

Loading

safern commented Jul 14, 2020

tarekgh commented Jul 14, 2020

MattGal commented Jul 14, 2020

MattGal commented Jul 14, 2020

tarekgh commented Jul 14, 2020

CodeBlanch Jul 15, 2020

CodeBlanch Jul 15, 2020

tarekgh Jul 15, 2020 •

edited

Loading

cijothomas Jul 15, 2020

CodeBlanch Jul 15, 2020

Improve Activity API usability and OpenTelemetry integration (Part 2) #39087

Improve Activity API usability and OpenTelemetry integration (Part 2) #39087

Conversation

tarekgh commented Jul 10, 2020

Dotnet-GitSync-Bot commented Jul 10, 2020

ghost commented Jul 10, 2020

ghost commented Jul 10, 2020

tarekgh commented Jul 10, 2020

tarekgh commented Jul 10, 2020

noahfalk left a comment

Choose a reason for hiding this comment

cijothomas left a comment

Choose a reason for hiding this comment

cijothomas Jul 14, 2020

Choose a reason for hiding this comment

tarekgh commented Jul 14, 2020 • edited Loading

safern commented Jul 14, 2020

tarekgh commented Jul 14, 2020

MattGal commented Jul 14, 2020

MattGal commented Jul 14, 2020

tarekgh commented Jul 14, 2020

CodeBlanch Jul 15, 2020

Choose a reason for hiding this comment

CodeBlanch Jul 15, 2020

Choose a reason for hiding this comment

tarekgh Jul 15, 2020 • edited Loading

Choose a reason for hiding this comment

cijothomas Jul 15, 2020

Choose a reason for hiding this comment

CodeBlanch Jul 15, 2020

Choose a reason for hiding this comment

tarekgh commented Jul 14, 2020 •

edited

Loading

tarekgh Jul 15, 2020 •

edited

Loading