From 80b07796336fa7ee4225a026df77f327ed86a425 Mon Sep 17 00:00:00 2001 From: Evan Jones Date: Thu, 20 Jan 2022 17:45:28 -0500 Subject: [PATCH] ddtrace/tracer.StartSpan: Use setMeta instead of SetTag MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The SetTag function takes an interface{}, so every string argument must be converted using the convTString internal functions, which allocates a wrapper. Since this is an internal function, we can use setMeta directly. This seems to be what happens elsewhere in this function. These are the only two uses of SetTag. This removes up to 2 memory allocations on each call to this function. contrib/.../grpc: Add benchmark for env and version tags, and for the NoDebugStack() option. This should more accurately represent how the gRPC interceptor is typically configured. The benchstat output from my laptop for 10 runs shows this change saves about 1-2% CPU, and the expected 1 allocation for the env tag, because the version tag is not set on the gRPC interceptor spans. name old time/op new time/op delta UnaryServerInterceptor/ok_no_metadata-8 4.14µs ± 4% 4.11µs ±14% ~ (p=0.190 n=10+10) UnaryServerInterceptor/ok_with_metadata_no_parent-8 4.87µs ± 3% 4.77µs ± 3% -1.96% (p=0.011 n=9+9) UnaryServerInterceptor/ok_with_metadata_with_parent-8 4.29µs ± 3% 4.18µs ± 1% -2.50% (p=0.000 n=10+9) UnaryServerInterceptor/ok_no_metadata_with_analytics_rate-8 4.25µs ± 4% 4.13µs ± 1% -2.76% (p=0.002 n=10+9) UnaryServerInterceptor/error_no_metadata-8 10.6µs ± 3% 10.5µs ± 3% ~ (p=0.113 n=10+9) UnaryServerInterceptor/error_no_metadata_no_stack-8 5.24µs ± 2% 5.15µs ± 4% ~ (p=0.051 n=10+9) name old alloc/op new alloc/op delta UnaryServerInterceptor/ok_no_metadata-8 4.25kB ± 0% 4.23kB ± 0% -0.38% (p=0.001 n=10+10) UnaryServerInterceptor/ok_with_metadata_no_parent-8 4.86kB ± 0% 4.84kB ± 1% -0.42% (p=0.001 n=7+10) UnaryServerInterceptor/ok_with_metadata_with_parent-8 3.96kB ± 0% 3.95kB ± 0% -0.28% (p=0.017 n=10+10) UnaryServerInterceptor/ok_no_metadata_with_analytics_rate-8 4.45kB ± 0% 4.44kB ± 0% ~ (p=0.055 n=10+9) UnaryServerInterceptor/error_no_metadata-8 10.6kB ± 0% 10.6kB ± 0% -0.32% (p=0.001 n=10+9) UnaryServerInterceptor/error_no_metadata_no_stack-8 5.56kB ± 0% 5.54kB ± 1% -0.41% (p=0.007 n=10+10) name old allocs/op new allocs/op delta UnaryServerInterceptor/ok_no_metadata-8 43.0 ± 0% 42.0 ± 0% -2.33% (p=0.000 n=10+10) UnaryServerInterceptor/ok_with_metadata_no_parent-8 48.0 ± 0% 47.0 ± 0% -2.08% (p=0.000 n=10+10) UnaryServerInterceptor/ok_with_metadata_with_parent-8 39.0 ± 0% 38.0 ± 0% -2.56% (p=0.000 n=10+10) UnaryServerInterceptor/ok_no_metadata_with_analytics_rate-8 45.0 ± 0% 44.0 ± 0% -2.22% (p=0.000 n=10+10) UnaryServerInterceptor/error_no_metadata-8 60.0 ± 0% 59.0 ± 0% -1.67% (p=0.000 n=10+10) UnaryServerInterceptor/error_no_metadata_no_stack-8 47.0 ± 0% 46.0 ± 0% -2.13% (p=0.000 n=10+10) --- contrib/google.golang.org/grpc/grpc_test.go | 12 +++++++++++- ddtrace/tracer/tracer.go | 4 ++-- 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/contrib/google.golang.org/grpc/grpc_test.go b/contrib/google.golang.org/grpc/grpc_test.go index ff27a3d755..477a144029 100644 --- a/contrib/google.golang.org/grpc/grpc_test.go +++ b/contrib/google.golang.org/grpc/grpc_test.go @@ -799,7 +799,9 @@ func TestIgnoredMetadata(t *testing.T) { func BenchmarkUnaryServerInterceptor(b *testing.B) { // need to use the real tracer to get representative measurments - tracer.Start(tracer.WithLogger(log.DiscardLogger{})) + tracer.Start(tracer.WithLogger(log.DiscardLogger{}), + tracer.WithEnv("test"), + tracer.WithServiceVersion("0.1.2")) defer tracer.Stop() doNothingOKGRPCHandler := func(ctx context.Context, req interface{}) (interface{}, error) { @@ -870,4 +872,12 @@ func BenchmarkUnaryServerInterceptor(b *testing.B) { interceptor(ctx, "ignoredRequestValue", methodInfo, doNothingErrorGRPCHandler) } }) + interceptorNoStack := UnaryServerInterceptor(NoDebugStack()) + b.Run("error_no_metadata_no_stack", func(b *testing.B) { + b.ReportAllocs() + b.ResetTimer() + for i := 0; i < b.N; i++ { + interceptorNoStack(ctx, "ignoredRequestValue", methodInfo, doNothingErrorGRPCHandler) + } + }) } diff --git a/ddtrace/tracer/tracer.go b/ddtrace/tracer/tracer.go index c3855daf8e..54cea45970 100644 --- a/ddtrace/tracer/tracer.go +++ b/ddtrace/tracer/tracer.go @@ -400,10 +400,10 @@ func (t *tracer) StartSpan(operationName string, options ...ddtrace.StartSpanOpt delete(span.Metrics, keyMeasured) } if t.config.version != "" && span.Service == t.config.serviceName { - span.SetTag(ext.Version, t.config.version) + span.setMeta(ext.Version, t.config.version) } if t.config.env != "" { - span.SetTag(ext.Environment, t.config.env) + span.setMeta(ext.Environment, t.config.env) } if _, ok := span.context.samplingPriority(); !ok { // if not already sampled or a brand new trace, sample it