Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

contrib/go-redis/redis.v8: add Option to Exclude Raw Command #1026

Closed
wants to merge 12 commits into from
13 changes: 11 additions & 2 deletions contrib/go-redis/redis.v8/option.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,9 @@ import (
)

type clientConfig struct {
serviceName string
analyticsRate float64
serviceName string
analyticsRate float64
excludeRawCommand bool
}

// ClientOption represents an option that can be used to create or wrap a client.
Expand Down Expand Up @@ -58,3 +59,11 @@ func WithAnalyticsRate(rate float64) ClientOption {
}
}
}

// WithExcludeRawCommand sets the option to exclude the raw command
// for when the command/cache might contain sensitive data
Comment on lines +63 to +64
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand this part. The Datadog Agent obfuscates this tag and hides all parameters. Why would the fact that it contains sensitive data be an issue?

Suggested change
// WithExcludeRawCommand sets the option to exclude the raw command
// for when the command/cache might contain sensitive data
// SkipRawCommand determines the integration to skip setting the raw command
// in the redis.raw_command tag of a span.

As for the doc, we should be a bit more specific on what it does.

func WithExcludeRawCommand(exclude bool) ClientOption {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
func WithExcludeRawCommand(exclude bool) ClientOption {
func SkipRawCommand(skip bool) ClientOption {

I think this would be better, unless it's bad that we don't have the With* prefix?

return func(cfg *clientConfig) {
cfg.excludeRawCommand = exclude
}
}
19 changes: 14 additions & 5 deletions contrib/go-redis/redis.v8/redis.go
Original file line number Diff line number Diff line change
Expand Up @@ -102,14 +102,18 @@ 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
var opts []ddtrace.StartSpanOption
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT: Generally I'm okay with not pre-allocating the arrays backing a slice unless it has a significant performance impact. I'm not sure if this analysis has been done here, but assuming it was, this change here could be a small performance regression.

Have you considered just keeping this line (and the one in BeforeProcessPipeline) as-is? As far as I can tell that should work just fine.

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.excludeRawCommand {
opts = append(opts, tracer.Tag("redis.raw_command", raw))
}

Comment on lines +112 to +116
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if !p.config.excludeRawCommand {
opts = append(opts, tracer.Tag("redis.raw_command", raw))
}
if !p.config.excludeRawCommand {
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))
Expand All @@ -134,16 +138,21 @@ 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
var opts []ddtrace.StartSpanOption
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.excludeRawCommand {
opts = append(opts,
tracer.Tag("redis.raw_command", raw),
tracer.Tag(ext.ResourceName, raw))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for missing this. The resource name is 100% mandatory for all spans. We can not leave it out.

}

Comment on lines +149 to +155
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if !p.config.excludeRawCommand {
opts = append(opts,
tracer.Tag("redis.raw_command", raw),
tracer.Tag(ext.ResourceName, raw))
}
if !p.config.excludeRawCommand {
opts = append(opts,
tracer.Tag("redis.raw_command", raw),
tracer.Tag(ext.ResourceName, raw))
}

opts = append(opts, ddh.additionalTags...)
if !math.IsNaN(p.config.analyticsRate) {
opts = append(opts, tracer.Tag(ext.EventSampleRate, p.config.analyticsRate))
Expand Down
50 changes: 50 additions & 0 deletions contrib/go-redis/redis.v8/redis_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -464,3 +464,53 @@ func TestWithContext(t *testing.T) {
assert.Equal(span1.SpanID(), setSpan.ParentID())
assert.Equal(span2.SpanID(), getSpan.ParentID())
}

func TestWithExcludeRawCommand(t *testing.T) {
execCommand := func(t *testing.T, mt mocktracer.Tracer, opts ...ClientOption) {
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)
}

t.Run("defaults", func(t *testing.T) {
mt := mocktracer.Start()
defer mt.Stop()

execCommand(t, mt)

spans := mt.FinishedSpans()
assert.Len(t, spans, 2)
for _, s := range spans {
assert.Equal(t, "SET test_key test_value", s.Tag("redis.raw_command"))
}
})

t.Run("with exclude true", func(t *testing.T) {
mt := mocktracer.Start()
defer mt.Stop()

execCommand(t, mt, WithExcludeRawCommand(true))

spans := mt.FinishedSpans()
assert.Len(t, spans, 2)
for _, s := range spans {
assert.Equal(t, nil, s.Tag("redis.raw_command"))
}
})

t.Run("with exclude false", func(t *testing.T) {
mt := mocktracer.Start()
defer mt.Stop()

execCommand(t, mt, WithExcludeRawCommand(false))

spans := mt.FinishedSpans()
assert.Len(t, spans, 2)
for _, s := range spans {
assert.Equal(t, "SET test_key test_value", s.Tag("redis.raw_command"))
}
})
}