Added distributed tracing support: activity and diagnostic listener. #6853

Ilchert · 2020-12-09T10:55:39Z

Added distributed tracing support: added grain filters for activity propagation.
#4992

src/Orleans.Core/Core/ClientBuilderExtensions.cs

src/Orleans.Core/Diagnostics/ActivityPropagationGrainCallFilter.cs

alexvaluyskiy · 2020-12-10T18:11:39Z

Have you looked at ActivitySource? It is the new recommended approach for distributed tracing for Microsoft.
It uses the same Activity class, but the Activity should be started via ActivitySource, instead of DiagnosticSource
You could look at this example.
https://github.com/open-telemetry/opentelemetry-dotnet/blob/master/src/OpenTelemetry.Instrumentation.StackExchangeRedis/Implementation/RedisProfilerEntryToActivityConverter.cs

Ilchert · 2020-12-10T18:49:34Z

@alexvaluyskiy, I have seen Activity source, but it is not available in netcore 3. Also, if I understand correctly, activity source just simplify working with activities and diagnostic source and my current code do the same.

alexvaluyskiy · 2020-12-10T22:33:41Z

It works in NetCore 3.1, you just should a reference to the latest version of ‘ System.Diagnostics.DiagnosticSource’

alexvaluyskiy · 2020-12-10T22:34:17Z

ActivitySource allows you to add a tracing to OpenTelemetry without any additional code

Ilchert · 2020-12-11T06:31:50Z

@alexvaluyskiy, It is interesting, according to msdn it is available since net5, but also there is a nuget package. But Orleans uses version 4.7 of the DiagnosticSource NuGet package and I can't increase the version, because it depends on other 5.0 libs (System.Runtime.CompilerServices.Unsafe in full dotnet). So we have to upgrade all references to actual and then update the pull request using ActivitySource.
But eventually, this PR does the same as ActivitySource - creates activity and pushes info into DiagnosticSource.

Ilchert · 2020-12-11T06:35:11Z

Also, the main goal of this PR for me is propagating trace id over Orleans transport to support trace headers inside HttpClient.

ReubenBond · 2020-12-16T19:59:52Z

Thank you, this looks good to me and we can accept it with the change mentioned (DiagnosticSource/DiagnosticListener subclass).

We can upgrade all references to .NET 5 libs as long as they are still compatible with .NET Standard 2.0. It will take some work (must edit Directory.Build.props in the root directory to upgrade the packages).

This could also be a separate package, on OrleansContrib, for example.

Ilchert · 2020-12-16T20:40:19Z

Thank you, this looks good to me and we can accept it with the change mentioned (DiagnosticSource/DiagnosticListener subclass).

@ReubenBond please confirm, that you prefer subclass to a static field.

ReubenBond · 2020-12-16T20:44:29Z

I'm fine with a static field if the value will never need to participate in DI. A subclass may be cleaner.

Ilchert · 2020-12-16T20:48:47Z

Thanks, I will come back in a few days.

Ilchert · 2020-12-17T09:06:46Z

@ReubenBond done.

alexvaluyskiy · 2020-12-28T10:49:01Z

@cijothomas could you please look at this implementation?

cijothomas · 2020-12-28T17:32:51Z

@cijothomas could you please look at this implementation?

Ack. I'm on vacation now, will be back by Jan 1st week and take a look.

cijothomas · 2021-01-07T19:26:34Z

It is strongly advised to use ActivitySource API to start the Activity, instead of doing new Activity(). ActivitySource was introduced to make Activity more aligned with OpenTelemetry efforts, and has capabilities like built-in Sampling etc.
Its available as nuget for all .NET versions, and being a System.* package, is fully backward compatible.

cijothomas · 2021-01-07T19:29:00Z

Additionally, would recommend to use OpenTelemetry concept of Propagators to propagate context, instead of propagating manually.
(but this would require adding a dependency to OpenTelemetry library. This is something we hope to solve in .NET 6 timeline where .NET DiagnosticSource would expose a propagator API (dotnet/runtime#46054))

Ilchert · 2021-01-29T07:52:32Z

@ReubenBond could you please summarize, should I cancel this PR and wait 5.0 libs, reference OpenTelemetry or it is OK for current orleans version?

ReubenBond · 2021-01-29T16:02:37Z

@Ilchert I was under the impression that @cijothomas' comment needed addressing.
On the other hand, I also believe this can be turned into an OrleansContrib project, since it doesn't have any requirements on changing the core.

oising · 2021-02-03T00:19:27Z

hey @Ilchert -- this might help you kickstart the modifications to use what @cijothomas is suggesting:

https://rehansaeed.com/deep-dive-into-open-telemetry-for-net/
https://rehansaeed.com/open-telemetry-for-asp-net-core/

Ilchert · 2021-02-03T09:13:12Z

@ReubenBond thanks, I will update this PR when orleans updates dependencies to 5.x version. I believe that framework for distributed application must support distributed tracing :)

cijothomas · 2021-02-03T17:12:31Z

This is the link to official instrumentation doc: https://github.com/open-telemetry/opentelemetry-dotnet/tree/main/src/OpenTelemetry.Api#instrumenting-a-libraryapplication-with-net-activity-api
Open issues in the OpenTelemetry-dotnet repo, if you have questions/concerns.

Ilchert · 2021-02-03T17:14:13Z

@cijothomas thanks!

ReubenBond · 2021-02-03T19:37:40Z

I believe that framework for distributed application must support distributed tracing :)

I agree, it's very important

oising · 2021-02-17T15:49:28Z

@Ilchert @cijothomas -- I would love to use this functionality with Orleans 3.4.x -- if I understand the conversation above, if I reference the OpenTelemetry nuget package(s), I should be able to get this working with minimal changes, right?

Ilchert · 2021-02-17T16:28:15Z

@oising, you can just upgrade Diagnostic source package to 5.0. Open telemetry does not have stable package.

cijothomas · 2021-02-17T16:37:28Z

@oising, you can just upgrade Diagnostic source package to 5.0. Open telemetry does not have stable package.

It does have stable now. (Released last week) https://www.nuget.org/packages/opentelemetry.api

cijothomas · 2021-02-17T16:39:17Z

@Ilchert @cijothomas -- I would love to use this functionality with Orleans 3.4.x -- if I understand the conversation above, if I reference the OpenTelemetry nuget package(s), I should be able to get this working with minimal changes, right?

Mostly you'd just need System.Diagnostics.DiagnosticSource reference only.
If you need to leverage the OpenTelemetry context propagation mechanism, then you'd need to add reference to Opentelemetry.Api nuget as well.

https://github.com/open-telemetry/opentelemetry-dotnet/tree/main/src/OpenTelemetry.Api#instrumenting-a-libraryapplication-with-net-activity-api

src/Orleans.Core/Diagnostics/ActivityPropagationGrainCallFilter.cs

amccool · 2021-06-11T15:10:48Z

src/Orleans.Runtime/Hosting/CoreHostingExtensions.cs

+        /// <returns>The builder.</returns>
+        public static ISiloBuilder AddActivityPropagation(this ISiloBuilder builder)
+        {
+            if (Activity.DefaultIdFormat != ActivityIdFormat.W3C)


I am running into systems that have DefaultIdFormat set to hierarchical, which causes this to throw. It appears that https://devblogs.microsoft.com/aspnet/improvements-in-net-core-3-0-for-troubleshooting-and-monitoring-distributed-apps/ and other posts such as https://jimmybogard.com/building-end-to-end-diagnostics-and-tracing-a-primer-trace-context/ are recommending
Activity.DefaultIdFormat = ActivityIdFormat.W3C;

Obviously the ClientBuilderExtension would need to change as well.

The systems in question are server2016 with dotnet3.1 runtime. dotnet5.0 is not installed.

Hello, @amccool, we have this check because internal implementation of context uses some w3c specific features, like scope, tags, etc. So, to use this extension you must change default format into W3C.

Please reconsider limiting to only W3C. Many systems rely on hierarchical IDs and it's set for entire app. Hierarchical IDs help with logs stored in DBs, since prefix matches use index. For example: startswith "|f728de91a402224da40046d1234e83c5.3b48459462d8e54e."

The default DistributedContextPropagator will handle both hierarchical and W3C ActivityId formats.

oising · 2021-06-29T14:49:38Z

Hmm, I've been playing with this extensively the last couple of days and I realized that a grain calling other grains are not shown to be dependent. That is to say, all dependent grain calls fall under the root call in the request, rather than as a child to the span of the calling grain(s). I don't think this is expected behavior.

Example - here I'm using the out of box OTEL instrumentation for aspnetcore and httpclient, and fronting a graphql server that talks to our cluster. You can see that the dependencies chain correct for the the OOTB calls - to the first grain call - but once you go beyond the initial hop into orleans, all calls are direct dependents of the root:

~~"getgateway" is a span representing the graphql query execution.~~

~~@ReubenBond - I would hold off on this, personally.~~

~~(ignore the slow startup times -- it does late connecting to services, and is running on my overloaded laptop lol~~

UPDATE Never mind... it turns out the use of GraphQL dataloaders and the way field resolvers are lazily evaluated is causing the chain to be broken

SebastianStehle · 2021-08-28T20:48:46Z

I really like this and it is a super cool feature. But I guess to also support stuff like serializers and other filters you would have to implement this deeper into the system.

But can it not be merged and then improved later?

davidfowl · 2021-08-28T20:57:37Z

cc @shirhatti

ReubenBond · 2021-08-30T15:25:11Z

But can it not be merged and then improved later?

Yes, it can. I think this PR is taking the right approach, too.
Many thanks to everyone who contributed to this either with code or with comments.

shirhatti · 2021-08-30T17:22:14Z

src/Orleans.Core/Diagnostics/ActivityPropagationGrainCallFilter.cs

+            // Create activity from context directly
+            var traceParent = RequestContext.Get(TraceParentHeaderName) as string;
+            var traceState = RequestContext.Get(TraceStateHeaderName) as string;
+            var parentContext = new ActivityContext();
+
+            if (traceParent is not null)
+            {
+                parentContext = ActivityContext.Parse(traceParent, traceState);
+            }


I would replace this logic and rely on the configured DistributedContextPropagator instead. I'm not familiar with Orleans, but in ASP.NET Core we use the propagator registered in DI and if not present, we fall back to the globally configured DistributedContextPropagator.Current

Also, was it intentional not to propagate Baggage? Or was it an omission?

DistributedContextPropagator is NET 6.0 only. I'm not sure Orleans 4.0 should be constrained to that, unless there's plans to backport it to 5.0

You may try to reference the latest Diagnostic Source package https://www.nuget.org/packages/System.Diagnostics.DiagnosticSource/6.0.0-preview.7.21377.19 which should be supporting the propagators and should work in .NET 5.0 too.

@tarekgh we had the same discussion about v5 packages. I can't just upgrade packages especially to preview versions. I think we should refactor this code after orleans upgrades all nuget references to 6.0.

@tarekgh Oh, that would be good if it works :)

shirhatti · 2021-08-30T17:24:34Z

src/Orleans.Core/Diagnostics/ActivityPropagationGrainCallFilter.cs

+            if (currentActivity is not null &&
+                currentActivity.IdFormat == ActivityIdFormat.W3C)
+            {
+                RequestContext.Set(TraceParentHeaderName, currentActivity.Id);
+                if (currentActivity.TraceStateString is not null)
+                    RequestContext.Set(TraceStateHeaderName, currentActivity.TraceStateString);
+            }


As I mentioned on the incoming filter, the outgoing filter should also use a propagator.

shirhatti · 2021-08-30T17:28:46Z

👀 @tarekgh

ReubenBond · 2021-12-18T20:54:27Z

We have a PR open to implement this, courtesy of @suraciii - comments welcome

amccool · 2021-12-19T04:22:44Z

would be good to be able to have the option to filter out system grains, similar to the dashboard.

suraciii · 2021-12-19T08:34:34Z

@amccool I agree, wonder if it can be implemented by a sampler
Also an option to make it always start new root traces for certain grain methods would be helpful, especially for IRemindable.ReceiveReminder, otherwise periodic calls of long-running grains may produce super long traces

suraciii · 2021-12-21T09:33:00Z

The PR to add DistributedContextPropagator: #7443

There's another choice is to manage activities along with src/Orleans.Core/Diagnostics/MessagingTrace.cs, just like what aspnetcore did in https://github.com/dotnet/aspnetcore/blob/release/6.0/src/Hosting/Hosting/src/Internal/HostingApplicationDiagnostics.cs, so that activities, diagnostic events and metrics can be managed in one location

ReubenBond · 2022-03-24T17:07:49Z

I've opened a follow-up PR which changes some of the details here. Please take a look if you're using this and/or are interested: #7647

Ilchert commented Dec 9, 2020

View reviewed changes

src/Orleans.Core/Core/ClientBuilderExtensions.cs Outdated Show resolved Hide resolved

Ilchert commented Dec 9, 2020

View reviewed changes

src/Orleans.Core/Diagnostics/ActivityPropagationGrainCallFilter.cs Outdated Show resolved Hide resolved

Ilchert closed this Feb 3, 2021

Ilchert reopened this Feb 16, 2021

ReubenBond added this to the 4.0.0 milestone Apr 28, 2021

rikbosch reviewed Apr 28, 2021

View reviewed changes

src/Orleans.Core/Diagnostics/ActivityPropagationGrainCallFilter.cs Outdated Show resolved Hide resolved

rikbosch reviewed Apr 29, 2021

View reviewed changes

src/Orleans.Core/Diagnostics/ActivityPropagationGrainCallFilter.cs Outdated Show resolved Hide resolved

amccool reviewed Jun 11, 2021

View reviewed changes

Ilchert added 6 commits July 6, 2021 16:01

Added distributed tracing support: activity and diagnostic listener.

ce3d24a

Make Diagnostic listener static.

def7d4d

rewritten to trace source.

9fbdc92

Fixed notes from PR

9f0c98b

Set status.

aab58c8

merge with master

c041e61

Ilchert force-pushed the feature/distributed-tracing branch from 4744702 to c041e61 Compare July 6, 2021 13:25

SebastianStehle mentioned this pull request Aug 28, 2021

[Feature Request] Support Activities (Open Telemetry) #7248

Closed

ReubenBond approved these changes Aug 30, 2021

View reviewed changes

ReubenBond merged commit 001be5b into dotnet:main Aug 30, 2021

shirhatti reviewed Aug 30, 2021

View reviewed changes

shirhatti mentioned this pull request Aug 30, 2021

Distributed tracing support should use DistributedContextPropagator #7251

Closed

suraciii mentioned this pull request Dec 17, 2021

Use DistributedContextPropagator to propagate Activities #7443

Merged

ReubenBond mentioned this pull request Mar 24, 2022

Enhance distributed tracing support #7647

Merged

github-actions bot locked and limited conversation to collaborators Dec 2, 2023

Added distributed tracing support: activity and diagnostic listener. #6853

Added distributed tracing support: activity and diagnostic listener. #6853

Conversation

Ilchert commented Dec 9, 2020

alexvaluyskiy commented Dec 10, 2020

Ilchert commented Dec 10, 2020 • edited Loading

alexvaluyskiy commented Dec 10, 2020

alexvaluyskiy commented Dec 10, 2020

Ilchert commented Dec 11, 2020

Ilchert commented Dec 11, 2020 • edited Loading

ReubenBond commented Dec 16, 2020 • edited Loading

Ilchert commented Dec 16, 2020

ReubenBond commented Dec 16, 2020

Ilchert commented Dec 16, 2020

Ilchert commented Dec 17, 2020

alexvaluyskiy commented Dec 28, 2020

cijothomas commented Dec 28, 2020

cijothomas commented Jan 7, 2021

cijothomas commented Jan 7, 2021

Ilchert commented Jan 29, 2021 • edited Loading

ReubenBond commented Jan 29, 2021

oising commented Feb 3, 2021

Ilchert commented Feb 3, 2021

cijothomas commented Feb 3, 2021

Ilchert commented Feb 3, 2021

ReubenBond commented Feb 3, 2021

oising commented Feb 17, 2021

Ilchert commented Feb 17, 2021

cijothomas commented Feb 17, 2021

cijothomas commented Feb 17, 2021

amccool Jun 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

magole Jul 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oising commented Jun 29, 2021 • edited Loading

SebastianStehle commented Aug 28, 2021

davidfowl commented Aug 28, 2021

ReubenBond commented Aug 30, 2021 • edited Loading

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

Ilchert Aug 31, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shirhatti commented Aug 30, 2021

ReubenBond commented Dec 18, 2021

amccool commented Dec 19, 2021

suraciii commented Dec 19, 2021 • edited Loading

suraciii commented Dec 21, 2021

ReubenBond commented Mar 24, 2022

Ilchert commented Dec 10, 2020 •

edited

Loading

Ilchert commented Dec 11, 2020 •

edited

Loading

ReubenBond commented Dec 16, 2020 •

edited

Loading

Ilchert commented Jan 29, 2021 •

edited

Loading

amccool Jun 11, 2021 •

edited

Loading

magole Jul 23, 2021 •

edited

Loading

oising commented Jun 29, 2021 •

edited

Loading

ReubenBond commented Aug 30, 2021 •

edited

Loading

Ilchert Aug 31, 2021 •

edited

Loading

suraciii commented Dec 19, 2021 •

edited

Loading