Inject trace context to grpc metadata #2870

lujiajing1126 · 2021-03-09T06:14:12Z

Signed-off-by: Megrez Lu [email protected]

Which problem is this PR solving?

Resolve #2869

Short description of the changes

Update go-plugin to 1.4.2
Add interceptors to support tracing propagation

codecov · 2021-03-09T06:45:33Z

Codecov Report

Merging #2870 (92733ab) into master (2687983) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2870      +/-   ##
==========================================
+ Coverage   96.00%   96.03%   +0.03%     
==========================================
  Files         229      229              
  Lines        9937     9941       +4     
==========================================
+ Hits         9540     9547       +7     
+ Misses        327      325       -2     
+ Partials       70       69       -1

Impacted Files	Coverage Δ
plugin/storage/grpc/shared/archive.go	`100.00% <100.00%> (ø)`
plugin/storage/grpc/shared/grpc_client.go	`87.06% <100.00%> (+0.46%)`	⬆️
cmd/collector/app/server/zipkin.go	`61.53% <0.00%> (-15.39%)`	⬇️
plugin/storage/integration/integration.go	`77.90% <0.00%> (+0.55%)`	⬆️
plugin/storage/badger/spanstore/reader.go	`96.08% <0.00%> (+0.71%)`	⬆️
...lugin/sampling/strategystore/adaptive/processor.go	`100.00% <0.00%> (+0.92%)`	⬆️
cmd/query/app/static_handler.go	`96.77% <0.00%> (+1.61%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2687983...92733ab. Read the comment docs.

jpkrohling · 2021-03-09T08:51:32Z

plugin/storage/grpc/shared/archive.go

@@ -44,7 +44,7 @@ type archiveWriter struct {

 // GetTrace takes a traceID and returns a Trace associated with that traceID from Archive Storage
 func (r *archiveReader) GetTrace(ctx context.Context, traceID model.TraceID) (*model.Trace, error) {
-	stream, err := r.client.GetArchiveTrace(upgradeContextWithBearerToken(ctx), &storage_v1.GetTraceRequest{
+	stream, err := r.client.GetArchiveTrace(upgradeWithTraceContext(upgradeContextWithBearerToken(ctx)), &storage_v1.GetTraceRequest{


I think it might be time to split the statement here, creating the context in one and using it in another.

I've used a functional type with a composition function to refactor this part. I suppose it is fine now.

jpkrohling · 2021-03-09T08:53:26Z

plugin/storage/grpc/shared/grpc_client.go

+		if !ok {
+			md = metadata.New(nil)
+		} else {
+			md = md.Copy()


Why do you need a copy?

Removed. I've checked it is unnecessary.

jpkrohling · 2021-03-09T08:56:35Z

plugin/storage/grpc/shared/metadata.go

@@ -0,0 +1,51 @@
+// Copyright (c) 2019 The Jaeger Authors.


Changed to 2021

jpkrohling · 2021-03-09T08:57:29Z

plugin/storage/grpc/shared/grpc_client.go

@@ -85,7 +121,7 @@ func (c *grpcClient) ArchiveSpanWriter() spanstore.Writer {

 // GetTrace takes a traceID and returns a Trace associated with that traceID
 func (c *grpcClient) GetTrace(ctx context.Context, traceID model.TraceID) (*model.Trace, error) {
-	stream, err := c.readerClient.GetTrace(upgradeContextWithBearerToken(ctx), &storage_v1.GetTraceRequest{
+	stream, err := c.readerClient.GetTrace(upgradeWithTraceContext(upgradeContextWithBearerToken(ctx)), &storage_v1.GetTraceRequest{


Given the number of places calling the two upgrade functions, it might be a good idea to combine both in another function, and use only this other function.

Refactored in the same way

lujiajing1126 · 2021-03-09T12:41:06Z

@jpkrohling Any idea why unit test failed? I don't think it is related to my latest modification

Update: Seems not stable

yurishkuro · 2021-03-09T14:48:58Z

plugin/storage/grpc/shared/grpc_client.go

+		return ctx
+	}
+
+	if span := opentracing.SpanFromContext(ctx); span != nil {


This is not now RPCs are typically instrumented, you need to create a child span for each outbound request.

Also, there is a common lib https://opentracing.io/registry/go-grpc/ for gRPC instrumentation, can we use that instead of doing manual instrumentation?

Yes. I agree with you that a child span should be created. But this is acutally a compromise here that I make use of the active span from the context.

The difficulty stems from the fact that go-plugin does not support customized DialOption which I have mentioned in the issue #2869.

The server-side code can use https://github.com/opentracing-contrib/go-grpc

Signed-off-by: Megrez Lu <[email protected]>

lujiajing1126 · 2021-06-04T02:23:31Z

@jpkrohling @yurishkuro I've updated to the latest go-plugin(v1.4.2) to enable client-side gRPC tracing. Could you kindly have a review again?

yurishkuro

I see you're only changing grpc-client. Are there similar changes needed for the server?
since there are no tests for tracing instrumentation of the plugin, could you try a manual test and post a screenshot of the trace?

yurishkuro · 2021-06-04T03:34:11Z

plugin/storage/grpc/config/config.go

@@ -16,11 +16,14 @@ package config

 import (
 	"fmt"
+	"github.com/opentracing/opentracing-go"


please move to the next group (I think make fmt will do this automatically)

Signed-off-by: Megrez Lu <[email protected]>

lujiajing1126 · 2021-06-04T04:18:23Z

I see you're only changing grpc-client. Are there similar changes needed for the server?

The plugin developer can enable tracing while starting the plugin server.

grpc.ServeWithGRPCServer(
				store.NewAliyunSLSStore(&appConfig, logger),
				func(options []googleGRPC.ServerOption) *googleGRPC.Server {
					return plugin.DefaultGRPCServer([]googleGRPC.ServerOption{
						googleGRPC.UnaryInterceptor(otgrpc.OpenTracingServerInterceptor(tracer)),
						googleGRPC.StreamInterceptor(otgrpc.OpenTracingStreamServerInterceptor(tracer)),
					})
				},
			)

Do we need to make it a default option?

since there are no tests for tracing instrumentation of the plugin, could you try a manual test and post a screenshot of the trace?

yurishkuro · 2021-06-04T04:38:42Z

I don't know if there's a place to make this a default option, at minimum it would make sense to mention this in the plugin/README, and perhaps add to examples/memstore-plugin/main.go ?

Signed-off-by: Megrez Lu <[email protected]>

lujiajing1126 · 2021-06-04T08:18:14Z

I don't know if there's a place to make this a default option, at minimum it would make sense to mention this in the plugin/README, and perhaps add to examples/memstore-plugin/main.go ?

I've added a tracing section in the document as well as code in the memstore example.

jpkrohling · 2021-06-04T10:48:08Z

Should 1.23 wait for this one?

lujiajing1126 requested a review from a team as a code owner March 9, 2021 06:14

lujiajing1126 requested a review from pavolloffay March 9, 2021 06:14

jpkrohling reviewed Mar 9, 2021

View reviewed changes

yurishkuro reviewed Mar 9, 2021

View reviewed changes

lujiajing1126 added 8 commits June 4, 2021 09:18

inject trace context to grpc metadata

5f668c3

Signed-off-by: Megrez Lu <[email protected]>

remove unused vars

1990930

Signed-off-by: Megrez Lu <[email protected]>

run make fmt

f3c1fd4

Signed-off-by: Megrez Lu <[email protected]>

fix year in header

83f4da5

Signed-off-by: Megrez Lu <[email protected]>

refactor with ContextUpgradeFunc

adca6ce

Signed-off-by: Megrez Lu <[email protected]>

refactor another usage in archive.go

f867e73

Signed-off-by: Megrez Lu <[email protected]>

eliminate metadata copy

f40c52d

Signed-off-by: Megrez Lu <[email protected]>

retry unit tests

4c600f6

Signed-off-by: Megrez Lu <[email protected]>

lujiajing1126 force-pushed the gprc-storage-inject-propagation-headers branch from 0bdde89 to 4c600f6 Compare June 4, 2021 01:18

lujiajing1126 added 6 commits June 4, 2021 10:18

update hashicorp/go-plugin to 1.4.2

575f097

Signed-off-by: Megrez Lu <[email protected]>

tidy

004a475

Signed-off-by: Megrez Lu <[email protected]>

remove context upgrade

4887f95

Signed-off-by: Megrez Lu <[email protected]>

add tracing interceptor

1ba4c3b

Signed-off-by: Megrez Lu <[email protected]>

remove test cases in grpc_plugin_test

b78d174

Signed-off-by: Megrez Lu <[email protected]>

directly depend on grpc-ot

a559ebc

Signed-off-by: Megrez Lu <[email protected]>

lujiajing1126 force-pushed the gprc-storage-inject-propagation-headers branch from 15da37b to a559ebc Compare June 4, 2021 02:19

yurishkuro reviewed Jun 4, 2021

View reviewed changes

run make fmt

a9ac83b

Signed-off-by: Megrez Lu <[email protected]>

lujiajing1126 added 2 commits June 4, 2021 14:45

add example

45f9ba5

Signed-off-by: Megrez Lu <[email protected]>

add docs

8982de5

Signed-off-by: Megrez Lu <[email protected]>

jpkrohling approved these changes Jun 4, 2021

View reviewed changes

jpkrohling added this to the Release 1.23.0 milestone Jun 4, 2021

yurishkuro approved these changes Jun 4, 2021

View reviewed changes

jpkrohling enabled auto-merge (squash) June 4, 2021 14:37

Merge branch 'master' into gprc-storage-inject-propagation-headers

92733ab

jpkrohling merged commit ff32436 into jaegertracing:master Jun 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Inject trace context to grpc metadata #2870

Inject trace context to grpc metadata #2870

lujiajing1126 commented Mar 9, 2021 •

edited

Loading

codecov bot commented Mar 9, 2021 •

edited

Loading

jpkrohling Mar 9, 2021

lujiajing1126 Mar 9, 2021

jpkrohling Mar 9, 2021

lujiajing1126 Mar 9, 2021

jpkrohling Mar 9, 2021

lujiajing1126 Mar 9, 2021

jpkrohling Mar 9, 2021

lujiajing1126 Mar 9, 2021

lujiajing1126 commented Mar 9, 2021 •

edited

Loading

yurishkuro Mar 9, 2021

lujiajing1126 Mar 9, 2021

lujiajing1126 commented Jun 4, 2021 •

edited

Loading

yurishkuro left a comment

yurishkuro Jun 4, 2021

lujiajing1126 Jun 4, 2021

lujiajing1126 commented Jun 4, 2021 •

edited

Loading

yurishkuro commented Jun 4, 2021

lujiajing1126 commented Jun 4, 2021

jpkrohling commented Jun 4, 2021

Inject trace context to grpc metadata #2870

Inject trace context to grpc metadata #2870

Conversation

lujiajing1126 commented Mar 9, 2021 • edited Loading

Which problem is this PR solving?

Short description of the changes

codecov bot commented Mar 9, 2021 • edited Loading

Codecov Report

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

lujiajing1126 commented Mar 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lujiajing1126 commented Jun 4, 2021 • edited Loading

yurishkuro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lujiajing1126 commented Jun 4, 2021 • edited Loading

yurishkuro commented Jun 4, 2021

lujiajing1126 commented Jun 4, 2021

jpkrohling commented Jun 4, 2021

lujiajing1126 commented Mar 9, 2021 •

edited

Loading

codecov bot commented Mar 9, 2021 •

edited

Loading

lujiajing1126 commented Mar 9, 2021 •

edited

Loading

lujiajing1126 commented Jun 4, 2021 •

edited

Loading

lujiajing1126 commented Jun 4, 2021 •

edited

Loading