From 61232bcaca3248541e1d50bc8c3c3919a8619f5c Mon Sep 17 00:00:00 2001 From: Gabriel Aszalos Date: Tue, 14 Dec 2021 12:58:38 +0200 Subject: [PATCH] contrib/go-redis/redis.v8: add WithSkipRawCommand option and fix resource This change adds a new option called `WithSkippedRawCommand` which determines the instrumentation code to skip adding the `redis.raw_command` tag to spans. This is useful in cases when the Datadog Agent Redis obfuscation is disabled (which it is, by default) and commands may contain sensitive information. Additionally, duplicate entries and an incosistency was found when setting the resource name for Redis spans, which was fixed as part of this PR. Closes #1022 --- contrib/go-redis/redis.v8/option.go | 10 +++++ contrib/go-redis/redis.v8/redis.go | 13 ++++--- contrib/go-redis/redis.v8/redis_test.go | 49 ++++++++++++++++++++++++- 3 files changed, 65 insertions(+), 7 deletions(-) diff --git a/contrib/go-redis/redis.v8/option.go b/contrib/go-redis/redis.v8/option.go index 86f90d8b59..400eb9b241 100644 --- a/contrib/go-redis/redis.v8/option.go +++ b/contrib/go-redis/redis.v8/option.go @@ -14,6 +14,7 @@ import ( type clientConfig struct { serviceName string analyticsRate float64 + skipRaw bool } // ClientOption represents an option that can be used to create or wrap a client. @@ -29,6 +30,15 @@ func defaults(cfg *clientConfig) { } } +// WithSkipRawCommand reports whether to skip setting the "redis.raw_command" tag +// on instrumenation spans. This may be useful if the Datadog Agent is not +// set up to obfuscate this value and it could contain sensitive information. +func WithSkipRawCommand(skip bool) ClientOption { + return func(cfg *clientConfig) { + cfg.skipRaw = skip + } +} + // WithServiceName sets the given service name for the client. func WithServiceName(name string) ClientOption { return func(cfg *clientConfig) { diff --git a/contrib/go-redis/redis.v8/redis.go b/contrib/go-redis/redis.v8/redis.go index afa9728e20..e03f5ad799 100644 --- a/contrib/go-redis/redis.v8/redis.go +++ b/contrib/go-redis/redis.v8/redis.go @@ -102,14 +102,16 @@ func (ddh *datadogHook) BeforeProcess(ctx context.Context, cmd redis.Cmder) (con raw := cmd.String() length := strings.Count(raw, " ") p := ddh.params - opts := make([]ddtrace.StartSpanOption, 0, 5+len(ddh.additionalTags)+1) // 5 options below + for additionalTags + for analyticsRate + opts := make([]ddtrace.StartSpanOption, 0, 4+1+len(ddh.additionalTags)+1) // 4 options below + redis.raw_command + ddh.additionalTags + analyticsRate opts = append(opts, tracer.SpanType(ext.SpanTypeRedis), tracer.ServiceName(p.config.serviceName), tracer.ResourceName(raw[:strings.IndexByte(raw, ' ')]), - tracer.Tag("redis.raw_command", raw), tracer.Tag("redis.args_length", strconv.Itoa(length)), ) + if !p.config.skipRaw { + opts = append(opts, tracer.Tag("redis.raw_command", raw)) + } opts = append(opts, ddh.additionalTags...) if !math.IsNaN(p.config.analyticsRate) { opts = append(opts, tracer.Tag(ext.EventSampleRate, p.config.analyticsRate)) @@ -134,16 +136,17 @@ func (ddh *datadogHook) BeforeProcessPipeline(ctx context.Context, cmds []redis. raw := commandsToString(cmds) length := strings.Count(raw, " ") p := ddh.params - opts := make([]ddtrace.StartSpanOption, 0, 7+len(ddh.additionalTags)+1) // 7 options below + for additionalTags + for analyticsRate + opts := make([]ddtrace.StartSpanOption, 0, 5+1+len(ddh.additionalTags)+1) // 5 options below + redis.raw_command + ddh.additionalTags + analyticsRate opts = append(opts, tracer.SpanType(ext.SpanTypeRedis), tracer.ServiceName(p.config.serviceName), tracer.ResourceName(raw[:strings.IndexByte(raw, ' ')]), - tracer.Tag("redis.raw_command", raw), tracer.Tag("redis.args_length", strconv.Itoa(length)), - tracer.Tag(ext.ResourceName, raw), tracer.Tag("redis.pipeline_length", strconv.Itoa(len(cmds))), ) + if !p.config.skipRaw { + opts = append(opts, tracer.Tag("redis.raw_command", raw)) + } opts = append(opts, ddh.additionalTags...) if !math.IsNaN(p.config.analyticsRate) { opts = append(opts, tracer.Tag(ext.EventSampleRate, p.config.analyticsRate)) diff --git a/contrib/go-redis/redis.v8/redis_test.go b/contrib/go-redis/redis.v8/redis_test.go index bd68d4bdf0..270a30334f 100644 --- a/contrib/go-redis/redis.v8/redis_test.go +++ b/contrib/go-redis/redis.v8/redis_test.go @@ -36,6 +36,51 @@ func TestMain(m *testing.M) { os.Exit(m.Run()) } +func TestSkipRaw(t *testing.T) { + runCmds := func(t *testing.T, opts ...ClientOption) []mocktracer.Span { + mt := mocktracer.Start() + defer mt.Stop() + ctx := context.Background() + client := NewClient(&redis.Options{Addr: "127.0.0.1:6379"}, opts...) + client.Set(ctx, "test_key", "test_value", 0) + pipeline := client.Pipeline() + pipeline.Expire(ctx, "pipeline_counter", time.Hour) + pipeline.Exec(ctx) + spans := mt.FinishedSpans() + assert.Len(t, spans, 2) + return spans + } + + t.Run("true", func(t *testing.T) { + spans := runCmds(t, WithSkipRawCommand(true)) + for _, span := range spans { + raw, ok := span.Tags()["redis.raw_command"] + assert.False(t, ok) + assert.Empty(t, raw) + } + }) + + t.Run("default", func(t *testing.T) { + spans := runCmds(t) + raw, ok := spans[0].Tags()["redis.raw_command"] + assert.True(t, ok) + assert.Equal(t, "set test_key test_value: ", raw) + raw, ok = spans[1].Tags()["redis.raw_command"] + assert.True(t, ok) + assert.Equal(t, "expire pipeline_counter 3600: false\n", raw) + }) + + t.Run("false", func(t *testing.T) { + spans := runCmds(t, WithSkipRawCommand(false)) + raw, ok := spans[0].Tags()["redis.raw_command"] + assert.True(t, ok) + assert.Equal(t, "set test_key test_value: ", raw) + raw, ok = spans[1].Tags()["redis.raw_command"] + assert.True(t, ok) + assert.Equal(t, "expire pipeline_counter 3600: false\n", raw) + }) +} + func TestClientEvalSha(t *testing.T) { ctx := context.Background() opts := &redis.Options{Addr: "127.0.0.1:6379"} @@ -225,7 +270,7 @@ func TestPipeline(t *testing.T) { assert.Equal("redis.command", span.OperationName()) assert.Equal(ext.SpanTypeRedis, span.Tag(ext.SpanType)) assert.Equal("my-redis", span.Tag(ext.ServiceName)) - assert.Equal("expire pipeline_counter 3600: false\n", span.Tag(ext.ResourceName)) + assert.Equal("expire", span.Tag(ext.ResourceName)) assert.Equal("127.0.0.1", span.Tag(ext.TargetHost)) assert.Equal("6379", span.Tag(ext.TargetPort)) assert.Equal("1", span.Tag("redis.pipeline_length")) @@ -244,7 +289,7 @@ func TestPipeline(t *testing.T) { assert.Equal("redis.command", span.OperationName()) assert.Equal(ext.SpanTypeRedis, span.Tag(ext.SpanType)) assert.Equal("my-redis", span.Tag(ext.ServiceName)) - assert.Equal("expire pipeline_counter 3600: false\nexpire pipeline_counter_1 60: false\n", span.Tag(ext.ResourceName)) + assert.Equal("expire", span.Tag(ext.ResourceName)) assert.Equal("2", span.Tag("redis.pipeline_length")) }