From 6c5c797ec979de0b938cf88982c1ddceb51647c5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dario=20Casta=C3=B1=C3=A9?= Date: Tue, 23 Jul 2024 16:34:19 +0200 Subject: [PATCH 1/5] ddtrace/tracer: dereference pointers to supported types in span.SetTag --- .gitlab-ci.yml | 2 +- ddtrace/tracer/span.go | 3 ++ ddtrace/tracer/span_test.go | 27 ++++++++++++++--- ddtrace/tracer/util.go | 52 +++++++++++++++++++++++++++++++++ ddtrace/tracer/util_test.go | 58 +++++++++++++++++++++++++++++++++++++ 5 files changed, 137 insertions(+), 5 deletions(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 00f8b74e72..16c4c75d68 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -9,7 +9,7 @@ variables: INDEX_FILE: index.txt KUBERNETES_SERVICE_ACCOUNT_OVERWRITE: dd-trace-go FF_USE_LEGACY_KUBERNETES_EXECUTION_STRATEGY: "true" - BENCHMARK_TARGETS: "BenchmarkStartRequestSpan|BenchmarkHttpServeTrace|BenchmarkTracerAddSpans|BenchmarkStartSpan|BenchmarkSingleSpanRetention|BenchmarkOTelApiWithCustomTags|BenchmarkInjectW3C|BenchmarkExtractW3C|BenchmarkPartialFlushing|BenchmarkGraphQL|BenchmarkSampleWAFContext|BenchmarkCaptureStackTrace" + BENCHMARK_TARGETS: "BenchmarkStartRequestSpan|BenchmarkHttpServeTrace|BenchmarkTracerAddSpans|BenchmarkStartSpan|BenchmarkSingleSpanRetention|BenchmarkOTelApiWithCustomTags|BenchmarkInjectW3C|BenchmarkExtractW3C|BenchmarkPartialFlushing|BenchmarkGraphQL|BenchmarkSampleWAFContext|BenchmarkCaptureStackTrace|BenchmarkSetTagString|BenchmarkSetTagStringPtr|BenchmarkSetTagMetric" include: - ".gitlab/benchmarks.yml" diff --git a/ddtrace/tracer/span.go b/ddtrace/tracer/span.go index 14f816f9f1..323003e23a 100644 --- a/ddtrace/tracer/span.go +++ b/ddtrace/tracer/span.go @@ -111,6 +111,9 @@ func (s *span) BaggageItem(key string) string { // SetTag adds a set of key/value metadata to the span. func (s *span) SetTag(key string, value interface{}) { + // To avoid dumping the memory address in case value is a pointer, we dereference it. + // Any pointer value that is a pointer to a pointer will be dumped as a string. + value = dereference(value) s.Lock() defer s.Unlock() // We don't lock spans when flushing, so we could have a data race when diff --git a/ddtrace/tracer/span_test.go b/ddtrace/tracer/span_test.go index 19e8edb8a4..e918acf45a 100644 --- a/ddtrace/tracer/span_test.go +++ b/ddtrace/tracer/span_test.go @@ -369,6 +369,13 @@ func TestSpanSetTag(t *testing.T) { span.SetTag("struct", sharedinternal.MetaStructValue{Value: testValue}) require.Equal(t, testValue, span.MetaStruct["struct"]) + s := "string" + span.SetTag("str_ptr", &s) + assert.Equal(s, span.Meta["str_ptr"]) + + span.SetTag("nil_str_ptr", (*string)(nil)) + assert.Equal("", span.Meta["nil_str_ptr"]) + assert.Panics(func() { span.SetTag("panicStringer", &panicStringer{}) }) @@ -1024,18 +1031,18 @@ func TestSetUserPropagatedUserID(t *testing.T) { func BenchmarkSetTagMetric(b *testing.B) { span := newBasicSpan("bench.span") - keys := "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ" + keys := strings.Split("abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ", "") b.ResetTimer() for i := 0; i < b.N; i++ { - k := string(keys[i%len(keys)]) + k := keys[i%len(keys)] span.SetTag(k, float64(12.34)) } } func BenchmarkSetTagString(b *testing.B) { span := newBasicSpan("bench.span") - keys := "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ" + keys := strings.Split("abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ", "") b.ResetTimer() for i := 0; i < b.N; i++ { @@ -1044,9 +1051,21 @@ func BenchmarkSetTagString(b *testing.B) { } } +func BenchmarkSetTagStringPtr(b *testing.B) { + span := newBasicSpan("bench.span") + keys := strings.Split("abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ", "") + v := makePointer("some text") + + b.ResetTimer() + for i := 0; i < b.N; i++ { + k := keys[i%len(keys)] + span.SetTag(k, v) + } +} + func BenchmarkSetTagStringer(b *testing.B) { span := newBasicSpan("bench.span") - keys := "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ" + keys := strings.Split("abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ", "") value := &stringer{} b.ResetTimer() for i := 0; i < b.N; i++ { diff --git a/ddtrace/tracer/util.go b/ddtrace/tracer/util.go index 67ee16149f..9a2da9fb0d 100644 --- a/ddtrace/tracer/util.go +++ b/ddtrace/tracer/util.go @@ -20,6 +20,8 @@ import ( func toFloat64(value interface{}) (f float64, ok bool) { const max = (int64(1) << 53) - 1 const min = -max + // If any other type is added here, remember to add it to the type switch in + // the `span.SetTag` function to handle pointers to these supported types. switch i := value.(type) { case byte: return float64(i), true @@ -122,3 +124,53 @@ func parsePropagatableTraceTags(s string) (map[string]string, error) { tags[key] = s[start:] return tags, nil } + +func dereference(value any) any { + // Falling into one of the cases will dereference the pointer and return the + // value of the pointer. It adds one allocation due to casting. + switch value.(type) { + case *bool: + return dereferenceGeneric(value.(*bool)) + case *string: + return dereferenceGeneric(value.(*string)) + // Supported type by toFloat64 + case *byte: + return dereferenceGeneric(value.(*byte)) + case *float32: + return dereferenceGeneric(value.(*float32)) + case *float64: + return dereferenceGeneric(value.(*float64)) + case *int: + return dereferenceGeneric(value.(*int)) + case *int8: + return dereferenceGeneric(value.(*int8)) + case *int16: + return dereferenceGeneric(value.(*int16)) + case *int32: + return dereferenceGeneric(value.(*int32)) + case *int64: + return dereferenceGeneric(value.(*int64)) + case *uint: + return dereferenceGeneric(value.(*uint)) + case *uint16: + return dereferenceGeneric(value.(*uint16)) + case *uint32: + return dereferenceGeneric(value.(*uint32)) + case *uint64: + return dereferenceGeneric(value.(*uint64)) + case *samplernames.SamplerName: + v := value.(*samplernames.SamplerName) + if v == nil { + return samplernames.Unknown + } + return *v + } + return value +} + +func dereferenceGeneric[T any](value *T) T { + if value == nil { + return *new(T) + } + return *value +} diff --git a/ddtrace/tracer/util_test.go b/ddtrace/tracer/util_test.go index 87e4d9ef19..8c40681ff4 100644 --- a/ddtrace/tracer/util_test.go +++ b/ddtrace/tracer/util_test.go @@ -11,6 +11,7 @@ import ( "testing" "github.com/stretchr/testify/assert" + "gopkg.in/DataDog/dd-trace-go.v1/internal/samplernames" ) func TestToFloat64(t *testing.T) { @@ -115,3 +116,60 @@ func TestParsePropagatableTraceTags(t *testing.T) { }) } } + +func TestDereference(t *testing.T) { + for i, tt := range []struct { + value interface{} + expected interface{} + }{ + {makePointer(1), 1}, + {makePointer(byte(1)), byte(1)}, + {makePointer(int16(1)), int16(1)}, + {makePointer(int32(1)), int32(1)}, + {makePointer(int64(1)), int64(1)}, + {makePointer(uint(1)), uint(1)}, + {makePointer(uint16(1)), uint16(1)}, + {makePointer(uint32(1)), uint32(1)}, + {makePointer(uint64(1)), uint64(1)}, + {makePointer("a"), "a"}, + {makePointer(float32(1.25)), float32(1.25)}, + {makePointer(float64(1.25)), float64(1.25)}, + {makePointer(true), true}, + {makePointer(false), false}, + {makePointer(samplernames.SingleSpan), samplernames.SingleSpan}, + {(*int)(nil), 0}, + {(*byte)(nil), byte(0)}, + {(*int16)(nil), int16(0)}, + {(*int32)(nil), int32(0)}, + {(*int64)(nil), int64(0)}, + {(*uint)(nil), uint(0)}, + {(*uint16)(nil), uint16(0)}, + {(*uint32)(nil), uint32(0)}, + {(*uint64)(nil), uint64(0)}, + {(*string)(nil), ""}, + {(*float32)(nil), float32(0)}, + {(*float64)(nil), float64(0)}, + {(*bool)(nil), false}, + {(*samplernames.SamplerName)(nil), samplernames.Unknown}, + {newSpan("test", "service", "resource", 1, 2, 0), "itself"}, + } { + t.Run(fmt.Sprintf("%d", i), func(t *testing.T) { + actual := dereference(tt.value) + // This is a special case where we want to compare the value itself + // because the dereference function returns the given value. + if tt.expected == "itself" { + if actual != tt.value { + t.Fatalf("expected: %#v, got: %#v", tt.value, actual) + } + return + } + if actual != tt.expected { + t.Fatalf("expected: %#v, got: %#v", tt.expected, actual) + } + }) + } +} + +func makePointer[T any](value T) *T { + return &value +} From a5eec188bd4f415e4344dc26df2629b64cc6ee59 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dario=20Casta=C3=B1=C3=A9?= Date: Thu, 25 Jul 2024 10:46:12 +0200 Subject: [PATCH 2/5] ddtrace/tracer: apply code review feedback --- ddtrace/tracer/util_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ddtrace/tracer/util_test.go b/ddtrace/tracer/util_test.go index 8c40681ff4..9690430ff2 100644 --- a/ddtrace/tracer/util_test.go +++ b/ddtrace/tracer/util_test.go @@ -151,7 +151,7 @@ func TestDereference(t *testing.T) { {(*float64)(nil), float64(0)}, {(*bool)(nil), false}, {(*samplernames.SamplerName)(nil), samplernames.Unknown}, - {newSpan("test", "service", "resource", 1, 2, 0), "itself"}, + {newSpan("test", "service", "resource", 1, 2, 0), "itself"}, // This test uses a value which type is not supported by dereference. } { t.Run(fmt.Sprintf("%d", i), func(t *testing.T) { actual := dereference(tt.value) From 22426bffe05f3b0d0b1efb118d37936537f43b54 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dario=20Casta=C3=B1=C3=A9?= Date: Thu, 25 Jul 2024 14:25:47 +0200 Subject: [PATCH 3/5] ddtrace/tracer: remove extra conversion to string in BenchmarkSetTagStringer --- ddtrace/tracer/span_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ddtrace/tracer/span_test.go b/ddtrace/tracer/span_test.go index e918acf45a..95b1460ac0 100644 --- a/ddtrace/tracer/span_test.go +++ b/ddtrace/tracer/span_test.go @@ -1069,7 +1069,7 @@ func BenchmarkSetTagStringer(b *testing.B) { value := &stringer{} b.ResetTimer() for i := 0; i < b.N; i++ { - k := string(keys[i%len(keys)]) + k := keys[i%len(keys)] span.SetTag(k, value) } } From 1e0c72798cf33af6ddff51782e7919ce986db9b9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dario=20Casta=C3=B1=C3=A9?= Date: Mon, 5 Aug 2024 11:04:43 +0200 Subject: [PATCH 4/5] Apply suggestions from code review Co-authored-by: Eliott Bouhana <47679741+eliottness@users.noreply.github.com> --- .gitlab-ci.yml | 2 +- ddtrace/tracer/util.go | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 16c4c75d68..c6d7819a1d 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -9,7 +9,7 @@ variables: INDEX_FILE: index.txt KUBERNETES_SERVICE_ACCOUNT_OVERWRITE: dd-trace-go FF_USE_LEGACY_KUBERNETES_EXECUTION_STRATEGY: "true" - BENCHMARK_TARGETS: "BenchmarkStartRequestSpan|BenchmarkHttpServeTrace|BenchmarkTracerAddSpans|BenchmarkStartSpan|BenchmarkSingleSpanRetention|BenchmarkOTelApiWithCustomTags|BenchmarkInjectW3C|BenchmarkExtractW3C|BenchmarkPartialFlushing|BenchmarkGraphQL|BenchmarkSampleWAFContext|BenchmarkCaptureStackTrace|BenchmarkSetTagString|BenchmarkSetTagStringPtr|BenchmarkSetTagMetric" + BENCHMARK_TARGETS: "BenchmarkStartRequestSpan|BenchmarkHttpServeTrace|BenchmarkTracerAddSpans|BenchmarkStartSpan|BenchmarkSingleSpanRetention|BenchmarkOTelApiWithCustomTags|BenchmarkInjectW3C|BenchmarkExtractW3C|BenchmarkPartialFlushing|BenchmarkGraphQL|BenchmarkSampleWAFContext|BenchmarkCaptureStackTrace|BenchmarkSetTagString|BenchmarkSetTagStringPtr|BenchmarkSetTagMetric|BenchmarkSetTagStringer" include: - ".gitlab/benchmarks.yml" diff --git a/ddtrace/tracer/util.go b/ddtrace/tracer/util.go index 9a2da9fb0d..476e696ecb 100644 --- a/ddtrace/tracer/util.go +++ b/ddtrace/tracer/util.go @@ -170,7 +170,8 @@ func dereference(value any) any { func dereferenceGeneric[T any](value *T) T { if value == nil { - return *new(T) + var T v + return v } return *value } From 497e1334ba7855acfcfca1f3f0da9a84173198e8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dario=20Casta=C3=B1=C3=A9?= Date: Mon, 5 Aug 2024 11:08:19 +0200 Subject: [PATCH 5/5] ddtrace/tracer: fix variable declaration in dereferenceGeneric --- ddtrace/tracer/util.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ddtrace/tracer/util.go b/ddtrace/tracer/util.go index 476e696ecb..bd3c03a845 100644 --- a/ddtrace/tracer/util.go +++ b/ddtrace/tracer/util.go @@ -170,7 +170,7 @@ func dereference(value any) any { func dereferenceGeneric[T any](value *T) T { if value == nil { - var T v + var v T return v } return *value