Skip to content
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

Enhance distributed tracing support #7647

Merged
merged 1 commit into from
Apr 13, 2022

Conversation

ReubenBond
Copy link
Member

@ReubenBond ReubenBond commented Mar 24, 2022

Fixes #4992
Contributes to #7480

We have support for Open Telemetry already, via System.Diagnostics.Activity support (from #6853 and #7443).

This PR enhances that support to include more details and hopefully conform more to the Open Telemetry guidelines surrounding span names and tags for RPC.

One thing which is missing is net.peer.name/.ip/.port. That should be set to the address of the remote endpoint. That information isn't known at the time that the integration executes and the Activity isn't in context when the remote peer is resolved or the message is transported to the remote endpoint. I think it's fine to omit this information, despite what the spec says: the remote service will emit a matching trace (with the trace parent set), and if there's a failure, it will be made visible in the trace error.

IInvokable now includes an ActivityName string property, containing the interface & method names formatted in a way that's hopefully appropriate for Open Telemetry. This replaces the existing constants passed to StartActivity (Orleans.Runtime.GrainCall.In and Orleans.Runtime.GrainCall.Out). Examples of the new Activity.Name values:

  • IGrainManagementExtension/DeactivateOnIdle
  • IBlockingEchoTaskGrain/GetLastEcho
  • IHungryGrain<T>/EatWith<U>

The ActivitySouceName is now Microsoft.Orleans instead of orleans.runtime.graincall.

The activity now includes the source & target GrainId, so you could use this to create a call graph of which grains call which other grains.

I wonder if we should name the ISiloBuilder/IClientBuilder extension methods from AddActivityPropagation to AddDistributedTracing - thoughts?

⚠ Note that this PR relies on #7646, so that must be merged first

Microsoft Reviewers: Open in CodeFlow

@oising
Copy link
Contributor

oising commented Mar 24, 2022

I wonder if we should name the ISiloBuilder/IClientBuilder extension methods from AddActivityPropagation to AddDistributedTracing - thoughts?

The general pattern for adding otel instrumentation in dotnet looks like this:

using Microsoft.Extensions.DependencyInjection;
using OpenTelemetry.Trace;

public void ConfigureServices(IServiceCollection services)
{
    services.AddOpenTelemetryTracing((builder) => builder
        .AddAspNetCoreInstrumentation()
        .AddJaegerExporter()
    );
}

So -- for discoverability's sake -- I'm wondering if you should expose an AddOrleansInstrumentation extension that calls your AddDistributedTracing internally.

@ReubenBond
Copy link
Member Author

ReubenBond commented Mar 24, 2022

A general-purpose extension makes sense from a DX standpoint. If it was configured directly on OTel, then Orleans would still need to know about it so that we can configure the call filters. I wonder how that could work, especially with us not directly referencing any OTel packages (going by System.Diagnostics instead). We could have them always-enabled, but I much prefer this be pay-for-what-you-use.

EDIT: by the way, I consider any enhancement there to be out of scope of this PR, but it's a good discussion to have and this is a fine place to discuss it (at least until this PR is merged)

@ievgenii-shepeliuk
Copy link

Will all this native distributed tracing support be available only in 4.X releases ?

@benjaminpetit
Copy link
Member

It would be nice if we had the grain interface version, it could be useful.

Too bad that we lose the namespace though, but the no periods thing is really limiting us.

@ReubenBond
Copy link
Member Author

We can consider that for a subsequent PR if we determine a desire for those properties. We can also include the fully qualified interface name as a tag.

@ReubenBond
Copy link
Member Author

Will all this native distributed tracing support be available only in 4.X releases ?

Yes, this form of it will only be available in 4.x, especially since it relies on other features/behavior only available in 4.x

You can, however, backport most of this to 3.x and use it, especially in the form it existed in before this PR.

@ReubenBond ReubenBond force-pushed the feature/distributed-tracing branch 2 times, most recently from 8aba444 to 69fed09 Compare April 6, 2022 21:02
@ReubenBond
Copy link
Member Author

@benjaminpetit ready to merge

@benjaminpetit benjaminpetit merged commit d4cead8 into dotnet:main Apr 13, 2022
@RehanSaeed
Copy link

How do you link the parent span coming from a client? With an ASP.NET server, the client passes HTTP headers containing this information.

@github-actions github-actions bot locked and limited conversation to collaborators Dec 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Distributed tracing and Orleans
5 participants