gRPC client instrumentation #687

alanwest · 2020-05-19T20:09:03Z

This PR partially addresses issue #482 adding client-side instrumentation for grpc-dotnet.

pjanotti

Hi @alanwest - just an initial quick pass, it will be interesting to put this together with some gRPC client and tests (I assume this comes when it is not draft anymore).

src/OpenTelemetry.Adapter.Dependencies/Implementation/GrpcClientDiagnosticListener.cs

pjanotti · 2020-05-19T20:54:02Z

src/OpenTelemetry.Adapter.Dependencies/Implementation/GrpcClientDiagnosticListener.cs

+                var rx = new Regex(@"(?<package>\w+).(?<service>\w+)/(?<method>\w+)", RegexOptions.Compiled);
+                var rpcService = rx.Match(path).Groups["service"].Value;
+
+                span.SetAttribute("rpc.service", rpcService);


Nice!

@cijothomas we probably should create a package with the OTel semantic tags.

src/OpenTelemetry.Adapter.Dependencies/Implementation/GrpcClientDiagnosticListener.cs

src/OpenTelemetry.Adapter.Dependencies/GrpcClientAdapter.cs

pjanotti · 2020-05-19T21:01:37Z

BTW @alanwest you will need to sign the CLA :)

alanwest · 2020-05-19T21:07:46Z

BTW @alanwest you will need to sign the CLA :)

Done.

it will be interesting to put this together with some gRPC client and tests (I assume this comes when it is not draft anymore).

Yes, I will plan to include something. I've been testing locally with some of the example applications from the grpc-dotnet repository. There's an example setup to work with Open Telemetry: https://github.com/grpc/grpc-dotnet/tree/master/examples#aggregator.

src/OpenTelemetry.Adapter.Dependencies/GrpcClientAdapter.cs

cijothomas · 2020-05-19T22:22:11Z

src/OpenTelemetry.Adapter.Dependencies/Implementation/GrpcClientDiagnosticListener.cs

+                span.SetAttribute("net.peer.port", request.RequestUri.Port);
+            }
+
+            // Do we want to propagate trace context here? GrpcClient is built on top of HttpClient.


I'd like to understand this better. Do we end up with 2 spans, one from httpclient, and another from grpcclient, both essentially representing the same thing?

Correct, two spans are generated. First the one from the grpcclient and then one from httpclient as a child.

I think it makes sense to only produce the span from the grpcclient. HttpClient instrumentation checks for the traceparent header which seems to suggest this kind of scenario was anticipated? Though traceparent assumes W3C format, so it seems like there needs to be a more general check here if we want to prevent both spans from being generated.

The actual order of events would be like Grpc.Start, HttpClient.Start, HttpClient.Stop, Grpc.Stop right? The httpclient span would be the child of grpc one.

This is essentially same as a user making a httpclient call inside a using(span=startactivespan()) block, which result in 2 span, 1 from customer created, and other from httpclient. In this case, we'd want to see both spans. customer span as parent, httpclient span as child.

Applying this principle to Grpc->HttpClient, it looks okay to see both spans. But if both are essentially same information then its redundant. Maybe let user decide if they want to suppress the child.?

Instead of looking at traceparent presence, we can check the activity.Parent inside httpclient, which would be that of Grpc.

(Just my initial thoughts, we need to discuss this more)

The actual order of events would be like Grpc.Start, HttpClient.Start, HttpClient.Stop, Grpc.Stop right? The httpclient span would be the child of grpc one.

Correct.

My initial impression was that the information was redundant, though I can understand why it may be desirable to show both since it is an accurate representation of what's really happening.

I've seen similar cases from some database drivers that use HttpClient or HttpWebRequest behind the scenes. I've found it intuitive to show only a span for the database operation but not the underlying http request.

I'll share some screenshots of some traces in the gitter channel tomorrow, so we can discuss further.

Okay.
Adding @shirhatti to this discussion :)

The traces I've generated seem to suggest that the HttpClient activity ends with the request headers being received. I can dig into the code to be sure of this. Though here's a trace I've captured:

I agree and I think having both HTTP and gRPC spans is a little untidy, however I do think it's important to indicate the gPRC call was made over HTTP (it is theoretically possible to use other transports than HTTP).

It may also help identify an issue with the HTTP client when trying to help diagnose an issue by highlighting extra latency / errors.

it is theoretically possible to use other transports than HTTP

Ah yes, interesting. I had not thought about this use case. Agreed it would be useful to note that the call was over HTTP in this case. As far as I know, the library currently only supports HTTP/2 at the moment (perhaps I'm wrong about this?)

@cijothomas and I spoke briefly regarding this issue. His thoughts:

my take would be this - lets not make any assumptions about what user wants - make it configurable to user. let user decide if they want to see grpc only or httpclient only or both. we can discuss the default behavior later.

I think this stance makes sense for now. My thought for this PR is to move forward without any attempt to suppress the spans generated from the HttpClient instrumentation. Then perhaps follow up as a separate effort with the idea of making this configurable and deciding on defaults.

I agree and think we should show everything as you are now. If we need to tighten up later, we can, maybe though configuration.

src/OpenTelemetry.Instrumentation.Dependencies/Implementation/GrpcClientDiagnosticListener.cs

alanwest · 2020-05-23T01:32:17Z

I signed it

src/OpenTelemetry.Api/Trace/Status.cs

This reverts commit be89687.

src/OpenTelemetry.Instrumentation.Dependencies/Implementation/GrpcClientDiagnosticListener.cs

…event.

funkeyfreak · 2020-06-16T18:17:23Z

@alanwest, do you need any help on this PR? My team is moving a ton of high-use protocols to GRPC, so this is very interesting to me :)

…ying HttpClient instrumentation

test/OpenTelemetry.Instrumentation.GrpcClient.Tests/GrpcClientTests.cs

JamesNK · 2020-07-02T21:30:04Z

...metry.Instrumentation.GrpcClient.Tests/OpenTelemetry.Instrumentation.GrpcClient.Tests.csproj

+    <DotNetCliToolReference Include="dotnet-xunit" Version="2.3.1" />
+
+    <PackageReference Include="Google.Protobuf" Version="3.12.3" />
+    <PackageReference Include="Grpc.AspNetCore" Version="2.25.0" />


I think you should update to 2.30.0. It resolves race conditions between the response task being completed and the listener being written to.

Otherwise looks good to go.

👍 Yep did the trick. I updated to 2.30.0-pre1 since it appears that 2.30.0 has not been published for all the dependencies yet. I can come back and update this again when they have been published.

Uh oh. Hehe, seems we're back to the same build error I was experiencing a few days back. You mentioned there may be other ways to resolve this?

I think, we can ignore the Azure pipeline. i think we will just use Github Actions for now...
@cijothomas , what do you think?

This reverts commit e7a7cbb.

...metry.Instrumentation.GrpcClient.Tests/OpenTelemetry.Instrumentation.GrpcClient.Tests.csproj

…nwest/opentelemetry-dotnet into alanwest/grpc-dotnet-instrumentation

JamesNK · 2020-07-07T00:04:35Z

🍰 🎉 🎈

Wire up Grpc.Net.Client instrumentation

2a96c46

pjanotti reviewed May 19, 2020

View reviewed changes

cijothomas reviewed May 19, 2020

View reviewed changes

src/OpenTelemetry.Adapter.Dependencies/GrpcClientAdapter.cs Outdated Show resolved Hide resolved

cijothomas reviewed May 19, 2020

View reviewed changes

src/OpenTelemetry.Adapter.Dependencies/GrpcClientAdapter.cs Outdated Show resolved Hide resolved

cijothomas reviewed May 19, 2020

View reviewed changes

Merge branch 'master' into alanwest/grpc-dotnet-instrumentation

0f45daa

alanwest force-pushed the alanwest/grpc-dotnet-instrumentation branch from 8d0f366 to 0f45daa Compare May 20, 2020 19:47

JamesNK reviewed May 21, 2020

View reviewed changes

src/OpenTelemetry.Instrumentation.Dependencies/Implementation/GrpcClientDiagnosticListener.cs Outdated Show resolved Hide resolved

src/OpenTelemetry.Instrumentation.Dependencies/Implementation/GrpcClientDiagnosticListener.cs Outdated Show resolved Hide resolved

alanwest added 6 commits May 22, 2020 07:09

Fix missed namespace change

3125fd0

Fix typos

aea088c

Make regex private static

e99fa29

diagnosticSourceSubscriber cannot be null

06d5974

Use grpc.method tag from activity for span name and service attribute

88fe165

Merge branch 'master' into alanwest/grpc-dotnet-instrumentation

34803c4

gRPC instrumentation set Span.Status

be89687

alanwest commented May 23, 2020

View reviewed changes

src/OpenTelemetry.Api/Trace/Status.cs Outdated Show resolved Hide resolved

alanwest added 2 commits May 24, 2020 08:18

Revert "gRPC instrumentation set Span.Status"

c0f4da8

This reverts commit be89687.

Set span.Status from grpc.status_code tag on activity

f3dc113

JamesNK mentioned this pull request May 29, 2020

HttpClient and Activity lifetime dotnet/runtime#37141

Open

MikeGoldsmith approved these changes May 29, 2020

View reviewed changes

JamesNK reviewed May 29, 2020

View reviewed changes

src/OpenTelemetry.Instrumentation.Dependencies/Implementation/GrpcClientDiagnosticListener.cs Outdated Show resolved Hide resolved

alanwest added 3 commits June 4, 2020 20:11

Merge branch 'master' into alanwest/grpc-dotnet-instrumentation

cc181e5

Remove code and comments regarding context propagation

b6deb6b

Remove OnException override. GrpcClient does not have an OnException …

f9f6728

…event.

alanwest added 2 commits June 16, 2020 20:24

Merge branch 'master' into alanwest/grpc-dotnet-instrumentation

fce8818

Update GrpcClient instrumentation to use ActivitySource

33526a6

alanwest added 5 commits July 1, 2020 17:06

Simplify test scaffolding for Grpc tests

50c3905

Add test showing spans collected from Grpc instrumentation and underl…

0ae8020

…ying HttpClient instrumentation

Include package in rpc.service attribute

11756cd

Make GrpcClientInstrumentation internal

f5664b2

Merge branch 'master' into alanwest/grpc-dotnet-instrumentation

c51e214

cijothomas reviewed Jul 2, 2020

View reviewed changes

test/OpenTelemetry.Instrumentation.GrpcClient.Tests/GrpcClientTests.cs Show resolved Hide resolved

cijothomas reviewed Jul 2, 2020

View reviewed changes

test/OpenTelemetry.Instrumentation.GrpcClient.Tests/GrpcClientTests.cs Outdated Show resolved Hide resolved

cijothomas reviewed Jul 2, 2020

View reviewed changes

test/OpenTelemetry.Instrumentation.GrpcClient.Tests/GrpcClientTests.cs Show resolved Hide resolved

alanwest added 4 commits July 2, 2020 12:58

Validate that the HTTP span is a child of the gRPC span

0be7dc1

Set Activity.DefaultIdFormat in constructor

efc3f2e

After making call to gRPC stop the test host

e7a7cbb

Merge branch 'master' into alanwest/grpc-dotnet-instrumentation

1547234

eddynaka mentioned this pull request Jul 2, 2020

Support gRPC client integration #76

Closed

JamesNK approved these changes Jul 2, 2020

View reviewed changes

alanwest and others added 3 commits July 2, 2020 15:39

Revert "After making call to gRPC stop the test host"

c6bfad9

This reverts commit e7a7cbb.

Upgrading gRPC dependencies to 2.30.0-pre1 to resolve test flicker

46b8a2b

Merge branch 'master' into alanwest/grpc-dotnet-instrumentation

1b510be

JamesNK approved these changes Jul 5, 2020

View reviewed changes

...metry.Instrumentation.GrpcClient.Tests/OpenTelemetry.Instrumentation.GrpcClient.Tests.csproj Outdated Show resolved Hide resolved

alanwest and others added 5 commits July 5, 2020 10:56

Merge branch 'master' into alanwest/grpc-dotnet-instrumentation

fdf6cad

Use Grpc.Tools 2.25.0

3697939

Merge branch 'alanwest/grpc-dotnet-instrumentation' of github.com:ala…

eac3fab

…nwest/opentelemetry-dotnet into alanwest/grpc-dotnet-instrumentation

Replace Grpc.AspNetCore reference with Grpc.AspNetCore.Server

5b5a0fa

Merge branch 'master' into alanwest/grpc-dotnet-instrumentation

a52fea6

cijothomas approved these changes Jul 6, 2020

View reviewed changes

cijothomas merged commit 401737e into open-telemetry:master Jul 6, 2020

alanwest deleted the alanwest/grpc-dotnet-instrumentation branch July 7, 2020 19:22

alanwest mentioned this pull request Jul 30, 2020

Add gRPC attributes for ASP.NET Core #803

Merged

alanwest mentioned this pull request Aug 7, 2020

Create RPC spans for gRPC #482

Closed

alanwest mentioned this pull request Aug 15, 2023

Update Grpc.Net.Client to use ActivitySource grpc/grpc-dotnet#2244

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gRPC client instrumentation #687

gRPC client instrumentation #687

alanwest commented May 19, 2020 •

edited

pjanotti left a comment

pjanotti May 19, 2020

pjanotti commented May 19, 2020

alanwest commented May 19, 2020 •

edited

cijothomas May 19, 2020

alanwest May 19, 2020

cijothomas May 20, 2020

alanwest May 21, 2020

cijothomas May 21, 2020

alanwest May 23, 2020

MikeGoldsmith May 24, 2020

alanwest May 29, 2020

alanwest May 29, 2020

MikeGoldsmith May 29, 2020

alanwest commented May 23, 2020

funkeyfreak commented Jun 16, 2020 •

edited

JamesNK Jul 2, 2020

alanwest Jul 2, 2020

alanwest Jul 2, 2020

eddynaka Jul 2, 2020

JamesNK commented Jul 7, 2020

gRPC client instrumentation #687

gRPC client instrumentation #687

Conversation

alanwest commented May 19, 2020 • edited

pjanotti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjanotti commented May 19, 2020

alanwest commented May 19, 2020 • edited

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

alanwest commented May 23, 2020

funkeyfreak commented Jun 16, 2020 • edited

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

JamesNK commented Jul 7, 2020

alanwest commented May 19, 2020 •

edited

alanwest commented May 19, 2020 •

edited

funkeyfreak commented Jun 16, 2020 •

edited