Skip to content

Commit 2f84d07

Browse files
rwynnrwynn
committed
fix(valkey): Make error checking configurable
All tracer.WithError calls first run through a configurable error check function. Apply same treatment to rueidis to maintain consistency.
1 parent e459938 commit 2f84d07

File tree

6 files changed

+150
-26
lines changed

6 files changed

+150
-26
lines changed

Diff for: ‎contrib/redis/rueidis/option.go

+13
Original file line numberDiff line numberDiff line change
@@ -6,13 +6,15 @@
66
package rueidis
77

88
import (
9+
"github.com/redis/rueidis"
910
"gopkg.in/DataDog/dd-trace-go.v1/internal"
1011
"gopkg.in/DataDog/dd-trace-go.v1/internal/namingschema"
1112
)
1213

1314
type config struct {
1415
rawCommand bool
1516
serviceName string
17+
errCheck func(err error) bool
1618
}
1719

1820
// Option represents an option that can be used to create or wrap a client.
@@ -23,6 +25,9 @@ func defaultConfig() *config {
2325
// Do not include the raw command by default since it could contain sensitive data.
2426
rawCommand: internal.BoolEnv("DD_TRACE_REDIS_RAW_COMMAND", false),
2527
serviceName: namingschema.ServiceName(defaultServiceName),
28+
errCheck: func(err error) bool {
29+
return err != nil && !rueidis.IsRedisNil(err)
30+
},
2631
}
2732
}
2833

@@ -39,3 +44,11 @@ func WithServiceName(name string) Option {
3944
cfg.serviceName = name
4045
}
4146
}
47+
48+
// WithErrorCheck specifies a function fn which determines whether the passed
49+
// error should be marked as an error.
50+
func WithErrorCheck(fn func(err error) bool) Option {
51+
return func(cfg *config) {
52+
cfg.errCheck = fn
53+
}
54+
}

Diff for: ‎contrib/redis/rueidis/rueidis.go

+13-13
Original file line numberDiff line numberDiff line change
@@ -103,12 +103,21 @@ func (c *client) startSpan(ctx context.Context, cmd command) (tracer.Span, conte
103103

104104
func (c *client) finishSpan(span tracer.Span, err error) {
105105
var opts []tracer.FinishOption
106-
if err != nil && !rueidis.IsRedisNil(err) {
106+
if c.cfg.errCheck(err) {
107107
opts = append(opts, tracer.WithError(err))
108108
}
109109
span.Finish(opts...)
110110
}
111111

112+
func (c *client) firstError(s []rueidis.RedisResult) error {
113+
for _, result := range s {
114+
if err := result.Error(); c.cfg.errCheck(err) {
115+
return err
116+
}
117+
}
118+
return nil
119+
}
120+
112121
func (c *client) B() rueidis.Builder {
113122
return c.client.B()
114123
}
@@ -117,14 +126,14 @@ func (c *client) Do(ctx context.Context, cmd rueidis.Completed) rueidis.RedisRes
117126
span, ctx := c.startSpan(ctx, processCommand(&cmd))
118127
resp := c.client.Do(ctx, cmd)
119128
setClientCacheTags(span, resp)
120-
span.Finish(tracer.WithError(resp.Error()))
129+
c.finishSpan(span, resp.Error())
121130
return resp
122131
}
123132

124133
func (c *client) DoMulti(ctx context.Context, multi ...rueidis.Completed) []rueidis.RedisResult {
125134
span, ctx := c.startSpan(ctx, processCommandMulti(multi))
126135
resp := c.client.DoMulti(ctx, multi...)
127-
c.finishSpan(span, firstError(resp))
136+
c.finishSpan(span, c.firstError(resp))
128137
return resp
129138
}
130139

@@ -150,7 +159,7 @@ func (c *client) DoCache(ctx context.Context, cmd rueidis.Cacheable, ttl time.Du
150159
func (c *client) DoMultiCache(ctx context.Context, multi ...rueidis.CacheableTTL) []rueidis.RedisResult {
151160
span, ctx := c.startSpan(ctx, processCommandMultiCache(multi))
152161
resp := c.client.DoMultiCache(ctx, multi...)
153-
c.finishSpan(span, firstError(resp))
162+
c.finishSpan(span, c.firstError(resp))
154163
return resp
155164
}
156165

@@ -264,15 +273,6 @@ func multiCommand(cmds []command) command {
264273
}
265274
}
266275

267-
func firstError(s []rueidis.RedisResult) error {
268-
for _, result := range s {
269-
if err := result.Error(); err != nil && !rueidis.IsRedisNil(err) {
270-
return err
271-
}
272-
}
273-
return nil
274-
}
275-
276276
func setClientCacheTags(s tracer.Span, result rueidis.RedisResult) {
277277
s.SetTag(ext.RedisClientCacheHit, result.IsCacheHit())
278278
s.SetTag(ext.RedisClientCacheTTL, result.CacheTTL())

Diff for: ‎contrib/redis/rueidis/rueidis_test.go

+49
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ package rueidis
66

77
import (
88
"context"
9+
"errors"
910
"fmt"
1011
"os"
1112
"testing"
@@ -253,6 +254,54 @@ func TestNewClient(t *testing.T) {
253254
},
254255
wantServiceName: "global-service",
255256
},
257+
{
258+
name: "Test SET command with canceled context and custom error check",
259+
opts: []Option{
260+
WithErrorCheck(func(err error) bool {
261+
return err != nil && !rueidis.IsRedisNil(err) && !errors.Is(err, context.Canceled)
262+
}),
263+
},
264+
runTest: func(t *testing.T, ctx context.Context, client rueidis.Client) {
265+
ctx, cancel := context.WithCancel(ctx)
266+
cancel()
267+
require.Error(t, client.Do(ctx, client.B().Set().Key("test_key").Value("test_value").Build()).Error())
268+
},
269+
assertSpans: func(t *testing.T, spans []mocktracer.Span) {
270+
require.Len(t, spans, 1)
271+
272+
span := spans[0]
273+
assert.Equal(t, "SET", span.Tag(ext.ResourceName))
274+
assert.Nil(t, span.Tag(ext.RedisRawCommand))
275+
assert.Equal(t, false, span.Tag(ext.RedisClientCacheHit))
276+
assert.Less(t, span.Tag(ext.RedisClientCacheTTL), int64(0))
277+
assert.Less(t, span.Tag(ext.RedisClientCachePXAT), int64(0))
278+
assert.Less(t, span.Tag(ext.RedisClientCachePTTL), int64(0))
279+
assert.Nil(t, span.Tag(ext.Error))
280+
},
281+
wantServiceName: "global-service",
282+
},
283+
{
284+
name: "Test redis nil not attached to span",
285+
opts: []Option{
286+
WithRawCommand(true),
287+
},
288+
runTest: func(t *testing.T, ctx context.Context, client rueidis.Client) {
289+
require.Error(t, client.Do(ctx, client.B().Get().Key("404").Build()).Error())
290+
},
291+
assertSpans: func(t *testing.T, spans []mocktracer.Span) {
292+
require.Len(t, spans, 1)
293+
294+
span := spans[0]
295+
assert.Equal(t, "GET", span.Tag(ext.ResourceName))
296+
assert.Equal(t, "GET 404", span.Tag(ext.RedisRawCommand))
297+
assert.Equal(t, false, span.Tag(ext.RedisClientCacheHit))
298+
assert.Less(t, span.Tag(ext.RedisClientCacheTTL), int64(0))
299+
assert.Less(t, span.Tag(ext.RedisClientCachePXAT), int64(0))
300+
assert.Less(t, span.Tag(ext.RedisClientCachePTTL), int64(0))
301+
assert.Nil(t, span.Tag(ext.Error))
302+
},
303+
wantServiceName: "global-service",
304+
},
256305
}
257306
for _, tt := range tests {
258307
t.Run(tt.name, func(t *testing.T) {

Diff for: ‎contrib/valkey-go/option.go

+13
Original file line numberDiff line numberDiff line change
@@ -6,13 +6,15 @@
66
package valkey
77

88
import (
9+
"github.com/valkey-io/valkey-go"
910
"gopkg.in/DataDog/dd-trace-go.v1/internal"
1011
"gopkg.in/DataDog/dd-trace-go.v1/internal/namingschema"
1112
)
1213

1314
type config struct {
1415
rawCommand bool
1516
serviceName string
17+
errCheck func(err error) bool
1618
}
1719

1820
// Option represents an option that can be used to create or wrap a client.
@@ -23,6 +25,9 @@ func defaultConfig() *config {
2325
// Do not include the raw command by default since it could contain sensitive data.
2426
rawCommand: internal.BoolEnv("DD_TRACE_VALKEY_RAW_COMMAND", false),
2527
serviceName: namingschema.ServiceName(defaultServiceName),
28+
errCheck: func(err error) bool {
29+
return err != nil && !valkey.IsValkeyNil(err)
30+
},
2631
}
2732
}
2833

@@ -40,3 +45,11 @@ func WithServiceName(name string) Option {
4045
cfg.serviceName = name
4146
}
4247
}
48+
49+
// WithErrorCheck specifies a function fn which determines whether the passed
50+
// error should be marked as an error.
51+
func WithErrorCheck(fn func(err error) bool) Option {
52+
return func(cfg *config) {
53+
cfg.errCheck = fn
54+
}
55+
}

Diff for: ‎contrib/valkey-go/valkey.go

+13-13
Original file line numberDiff line numberDiff line change
@@ -80,14 +80,14 @@ func (c *client) Do(ctx context.Context, cmd valkey.Completed) valkey.ValkeyResu
8080
span, ctx := c.startSpan(ctx, processCommand(&cmd))
8181
resp := c.client.Do(ctx, cmd)
8282
setClientCacheTags(span, resp)
83-
span.Finish(tracer.WithError(resp.Error()))
83+
c.finishSpan(span, resp.Error())
8484
return resp
8585
}
8686

8787
func (c *client) DoMulti(ctx context.Context, multi ...valkey.Completed) []valkey.ValkeyResult {
8888
span, ctx := c.startSpan(ctx, processCommandMulti(multi))
8989
resp := c.client.DoMulti(ctx, multi...)
90-
c.finishSpan(span, firstError(resp))
90+
c.finishSpan(span, c.firstError(resp))
9191
return resp
9292
}
9393

@@ -109,7 +109,7 @@ func (c *client) DoCache(ctx context.Context, cmd valkey.Cacheable, ttl time.Dur
109109
func (c *client) DoMultiCache(ctx context.Context, multi ...valkey.CacheableTTL) []valkey.ValkeyResult {
110110
span, ctx := c.startSpan(ctx, processCommandMultiCache(multi))
111111
resp := c.client.DoMultiCache(ctx, multi...)
112-
c.finishSpan(span, firstError(resp))
112+
c.finishSpan(span, c.firstError(resp))
113113
return resp
114114
}
115115

@@ -207,12 +207,21 @@ func (c *client) startSpan(ctx context.Context, cmd command) (tracer.Span, conte
207207

208208
func (c *client) finishSpan(span tracer.Span, err error) {
209209
var opts []tracer.FinishOption
210-
if err != nil && !valkey.IsValkeyNil(err) {
210+
if c.cfg.errCheck(err) {
211211
opts = append(opts, tracer.WithError(err))
212212
}
213213
span.Finish(opts...)
214214
}
215215

216+
func (c *client) firstError(s []valkey.ValkeyResult) error {
217+
for _, result := range s {
218+
if err := result.Error(); c.cfg.errCheck(err) {
219+
return err
220+
}
221+
}
222+
return nil
223+
}
224+
216225
type commander interface {
217226
Commands() []string
218227
}
@@ -267,15 +276,6 @@ func multiCommand(cmds []command) command {
267276
}
268277
}
269278

270-
func firstError(s []valkey.ValkeyResult) error {
271-
for _, result := range s {
272-
if err := result.Error(); err != nil && !valkey.IsValkeyNil(err) {
273-
return err
274-
}
275-
}
276-
return nil
277-
}
278-
279279
func setClientCacheTags(s tracer.Span, result valkey.ValkeyResult) {
280280
s.SetTag(ext.ValkeyClientCacheHit, result.IsCacheHit())
281281
s.SetTag(ext.ValkeyClientCacheTTL, result.CacheTTL())

Diff for: ‎contrib/valkey-go/valkey_test.go

+49
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ package valkey
66

77
import (
88
"context"
9+
"errors"
910
"fmt"
1011
"os"
1112
"testing"
@@ -260,6 +261,54 @@ func TestNewClient(t *testing.T) {
260261
},
261262
wantServiceName: "global-service",
262263
},
264+
{
265+
name: "Test SET command with canceled context and custom error check",
266+
opts: []Option{
267+
WithErrorCheck(func(err error) bool {
268+
return err != nil && !valkey.IsValkeyNil(err) && !errors.Is(err, context.Canceled)
269+
}),
270+
},
271+
runTest: func(t *testing.T, ctx context.Context, client valkey.Client) {
272+
ctx, cancel := context.WithCancel(ctx)
273+
cancel()
274+
require.Error(t, client.Do(ctx, client.B().Set().Key("test_key").Value("test_value").Build()).Error())
275+
},
276+
assertSpans: func(t *testing.T, spans []mocktracer.Span) {
277+
require.Len(t, spans, 1)
278+
279+
span := spans[0]
280+
assert.Equal(t, "SET", span.Tag(ext.ResourceName))
281+
assert.Nil(t, span.Tag(ext.ValkeyRawCommand))
282+
assert.Equal(t, false, span.Tag(ext.ValkeyClientCacheHit))
283+
assert.Less(t, span.Tag(ext.ValkeyClientCacheTTL), int64(0))
284+
assert.Less(t, span.Tag(ext.ValkeyClientCachePXAT), int64(0))
285+
assert.Less(t, span.Tag(ext.ValkeyClientCachePTTL), int64(0))
286+
assert.Nil(t, span.Tag(ext.Error))
287+
},
288+
wantServiceName: "global-service",
289+
},
290+
{
291+
name: "Test valkey nil not attached to span",
292+
opts: []Option{
293+
WithRawCommand(true),
294+
},
295+
runTest: func(t *testing.T, ctx context.Context, client valkey.Client) {
296+
require.Error(t, client.Do(ctx, client.B().Get().Key("404").Build()).Error())
297+
},
298+
assertSpans: func(t *testing.T, spans []mocktracer.Span) {
299+
require.Len(t, spans, 1)
300+
301+
span := spans[0]
302+
assert.Equal(t, "GET", span.Tag(ext.ResourceName))
303+
assert.Equal(t, "GET 404", span.Tag(ext.ValkeyRawCommand))
304+
assert.Equal(t, false, span.Tag(ext.ValkeyClientCacheHit))
305+
assert.Less(t, span.Tag(ext.ValkeyClientCacheTTL), int64(0))
306+
assert.Less(t, span.Tag(ext.ValkeyClientCachePXAT), int64(0))
307+
assert.Less(t, span.Tag(ext.ValkeyClientCachePTTL), int64(0))
308+
assert.Nil(t, span.Tag(ext.Error))
309+
},
310+
wantServiceName: "global-service",
311+
},
263312
}
264313
for _, tt := range tests {
265314
t.Run(tt.name, func(t *testing.T) {

0 commit comments

Comments
 (0)