feat: first iteration on datadog tracing with spans #374

pboros · 2022-07-20T11:51:15Z

No description provided.

codecov-commenter · 2022-07-20T12:03:15Z

Codecov Report

Merging #374 (2cb3c25) into main (45a7e5d) will decrease coverage by 0.59%.
The diff coverage is 13.53%.

@@            Coverage Diff             @@
##             main     #374      +/-   ##
==========================================
- Coverage   28.83%   28.24%   -0.60%     
==========================================
  Files          66       68       +2     
  Lines        7001     7144     +143     
==========================================
- Hits         2019     2018       -1     
- Misses       4754     4899     +145     
+ Partials      228      227       -1

Impacted Files	Coverage Δ
server/metrics/tenant.go	`62.50% <ø> (ø)`
server/metrics/tracing.go	`0.00% <0.00%> (ø)`
server/midddleware/middleware.go	`0.00% <0.00%> (ø)`
server/midddleware/tracing.go	`0.00% <0.00%> (ø)`
server/transaction/manager.go	`1.69% <0.00%> (-1.13%)`	⬇️
server/midddleware/metrics.go	`16.12% <30.00%> (-13.42%)`	⬇️
server/metrics/requests.go	`63.09% <51.21%> (+3.09%)`	⬆️
store/kv/kv.go	`18.10% <100.00%> (+0.89%)`	⬆️

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 45a7e5d...2cb3c25. Read the comment docs.

store/kv/kv.go

store/search/ts.go

server/main.go

server/config/options.go

rbarabas · 2022-07-26T20:02:42Z

server/midddleware/middleware.go

-		validatorStreamServerInterceptor(),
+	}
+
+	if config.DatadogTrace.Enabled {


Should we put this logic into the Interceptor and just pass out required components (Logger etc. Config is global so no need to pass that down)? Am I missing the benefit of having these interceptors exposed at this level?

Initializing the interceptor has a very visible cost according to the profiler. I think it's better only to initialize them when they are needed. What do you think?

rbarabas · 2022-07-26T20:03:57Z

server/midddleware/middleware.go

+	}
+
+	if config.DatadogTrace.Enabled {
+		streamInterceptors = append(streamInterceptors, ddTraceStream())
 	}

 	if config.Metrics.Grpc.Enabled && config.Metrics.Grpc.ResponseTime {


What's our goal with the "basic" GRPC interceptor? Do we still need it? Or is this in place to make the transition smoother?

rbarabas · 2022-07-26T20:05:30Z

server/midddleware/middleware.go

+	}
+
+	if config.DatadogTrace.Enabled {
+		unaryInterceptors = append(unaryInterceptors, ddTraceUnary())
 	}

 	if config.Metrics.Grpc.Enabled && config.Metrics.Grpc.ResponseTime {


I wonder if we should put these Enabled flags under a single config structure and just case switch on them. WDYT?

rbarabas · 2022-07-26T20:06:45Z

server/midddleware/middleware.go

@@ -118,6 +120,10 @@ func Get(config *config.Config, tenantMgr *metadata.TenantManager, txMgr *transa
 	}

 	unaryInterceptors = append(unaryInterceptors, []grpc.UnaryServerInterceptor{
+		//grpctrace.UnaryServerInterceptor(grpctrace.WithServiceName(util.Service)),
+		grpc_logging.UnaryServerInterceptor(grpc_zerolog.InterceptorLogger(sampledTaggedLogger)),


Shall we add a TODO to hook this up to the span IDs? Perhaps perform the logging in the code that manages the high level request?

This is not something I changed, I just moved it in the initialization order. This and logs attached to spans are different.

server/midddleware/ddtrace.go

server/midddleware/metrics.go

store/kv/kv.go

store/search/ts.go

rbarabas · 2022-07-27T13:23:10Z

store/kv/kv.go

@@ -102,6 +103,17 @@ func measureLow(ctx context.Context, name string, f func() error) {
 		metrics.FdbRequests.Tagged(tags).Histogram("histogram", tally.DefaultBuckets)
 		defer metrics.FdbRequests.Tagged(tags).Histogram("histogram", tally.DefaultBuckets).Start().Stop()
 	}
+	if config.DefaultConfig.Tracing.Enabled {


Would the following work?

closer, err := setTrace(ctx, name) defer closer()

Above assumes no defer needs to be passed up but if you think you need the span later after L117 we can bring it back for Finish() or other things.

The config will be available as it's global so no need to pass that.

func setTrace(ctx context.Context, name string) (func f(), error) { if !config.DefaultConfig.Tracing.Enabled { return f(){}, nil } parentSpan, exists := tracer.SpenFromContext(ctx) if exists { ... } return f(){}, nil }

Btw I don't see tags being used but if they are being used elsewhere we could pass those down also.

rbarabas · 2022-07-27T13:26:04Z

store/search/ts.go

+		metrics.SearchRequests.Tagged(tags).Histogram("histogram", tally.DefaultBuckets)
+		defer metrics.SearchRequests.Tagged(tags).Histogram("histogram", tally.DefaultBuckets).Start().Stop()
+	}
+	if config.DefaultConfig.Tracing.Enabled {


Perhaps we can do the same here as in kv:

span, err := setTrace(ctx, name)

Maybe tags will need to be added to the signature of setTrace() or whatever the name of the helper function would be.

tigrisdata-argocd-bot · 2022-07-28T22:17:57Z

🎉 This PR is included in version 1.0.0-alpha.25 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

tigrisdata-argocd-bot · 2022-09-20T20:44:29Z

🎉 This PR is included in version 1.0.0-beta.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

pboros added the debug image This PR will generate a debug image label Jul 20, 2022

pboros force-pushed the pboros/datadog-spans branch from f682df6 to c53bf7f Compare July 20, 2022 11:56

pboros removed the debug image This PR will generate a debug image label Jul 20, 2022

pboros added the debug image This PR will generate a debug image label Jul 20, 2022

pboros force-pushed the pboros/datadog-spans branch from c53bf7f to 2233ae5 Compare July 20, 2022 13:40

pboros added debug image This PR will generate a debug image and removed debug image This PR will generate a debug image labels Jul 20, 2022

pboros force-pushed the pboros/datadog-spans branch from 2233ae5 to b09eac4 Compare July 20, 2022 14:32

pboros added debug image This PR will generate a debug image and removed debug image This PR will generate a debug image labels Jul 20, 2022

pboros force-pushed the pboros/datadog-spans branch from b09eac4 to 82eab19 Compare July 21, 2022 15:45

pboros added debug image This PR will generate a debug image and removed debug image This PR will generate a debug image labels Jul 21, 2022

pboros force-pushed the pboros/datadog-spans branch from 82eab19 to badc2c7 Compare July 25, 2022 12:26

pboros removed the debug image This PR will generate a debug image label Jul 25, 2022

pboros force-pushed the pboros/datadog-spans branch 3 times, most recently from 6ce0919 to b4dca20 Compare July 25, 2022 15:50

pboros added the debug image This PR will generate a debug image label Jul 25, 2022

pboros force-pushed the pboros/datadog-spans branch from b4dca20 to f404eef Compare July 25, 2022 17:57

pboros added debug image This PR will generate a debug image and removed debug image This PR will generate a debug image labels Jul 25, 2022

pboros marked this pull request as ready for review July 25, 2022 18:22

pboros requested review from rbarabas, himank and efirs and removed request for rbarabas and himank July 25, 2022 18:22

himank reviewed Jul 26, 2022

View reviewed changes

store/kv/kv.go Outdated Show resolved Hide resolved

himank reviewed Jul 26, 2022

View reviewed changes

store/search/ts.go Outdated Show resolved Hide resolved