Skip to content

Commit 271307d

Browse files
rwynnkakkoyundarccio
committed
fix(valkey): Make error checking configurable and fix inconsistency (#3297)
Co-authored-by: Kemal Akkoyun <[email protected]> Co-authored-by: Dario Castañé <[email protected]>
1 parent d652d88 commit 271307d

File tree

6 files changed

+150
-8
lines changed

6 files changed

+150
-8
lines changed

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

+13
Original file line numberDiff line numberDiff line change
@@ -8,11 +8,13 @@ package rueidis
88
import (
99
"github.com/DataDog/dd-trace-go/v2/instrumentation"
1010
"github.com/DataDog/dd-trace-go/v2/instrumentation/options"
11+
"github.com/redis/rueidis"
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.
@@ -22,6 +24,9 @@ func defaultConfig() *config {
2224
return &config{
2325
rawCommand: options.GetBoolEnv("DD_TRACE_REDIS_RAW_COMMAND", false),
2426
serviceName: instr.ServiceName(instrumentation.ComponentDefault, nil),
27+
errCheck: func(err error) bool {
28+
return err != nil && !rueidis.IsRedisNil(err)
29+
},
2530
}
2631
}
2732

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

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

+13-4
Original file line numberDiff line numberDiff line change
@@ -100,12 +100,21 @@ func (c *client) startSpan(ctx context.Context, cmd command) (*tracer.Span, cont
100100

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

109+
func (c *client) firstError(s []rueidis.RedisResult) error {
110+
for _, result := range s {
111+
if err := result.Error(); c.cfg.errCheck(err) {
112+
return err
113+
}
114+
}
115+
return nil
116+
}
117+
109118
func (c *client) B() rueidis.Builder {
110119
return c.client.B()
111120
}
@@ -114,14 +123,14 @@ func (c *client) Do(ctx context.Context, cmd rueidis.Completed) rueidis.RedisRes
114123
span, ctx := c.startSpan(ctx, processCommand(&cmd))
115124
resp := c.client.Do(ctx, cmd)
116125
setClientCacheTags(span, resp)
117-
span.Finish(tracer.WithError(resp.Error()))
126+
c.finishSpan(span, resp.Error())
118127
return resp
119128
}
120129

121130
func (c *client) DoMulti(ctx context.Context, multi ...rueidis.Completed) []rueidis.RedisResult {
122131
span, ctx := c.startSpan(ctx, processCommandMulti(multi))
123132
resp := c.client.DoMulti(ctx, multi...)
124-
c.finishSpan(span, firstError(resp))
133+
c.finishSpan(span, c.firstError(resp))
125134
return resp
126135
}
127136

@@ -147,7 +156,7 @@ func (c *client) DoCache(ctx context.Context, cmd rueidis.Cacheable, ttl time.Du
147156
func (c *client) DoMultiCache(ctx context.Context, multi ...rueidis.CacheableTTL) []rueidis.RedisResult {
148157
span, ctx := c.startSpan(ctx, processCommandMultiCache(multi))
149158
resp := c.client.DoMultiCache(ctx, multi...)
150-
c.finishSpan(span, firstError(resp))
159+
c.finishSpan(span, c.firstError(resp))
151160
return resp
152161
}
153162

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

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

+13
Original file line numberDiff line numberDiff line change
@@ -8,11 +8,13 @@ package valkey
88
import (
99
"github.com/DataDog/dd-trace-go/v2/instrumentation"
1010
"github.com/DataDog/dd-trace-go/v2/instrumentation/options"
11+
"github.com/valkey-io/valkey-go"
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: options.GetBoolEnv("DD_TRACE_VALKEY_RAW_COMMAND", false),
2527
serviceName: instr.ServiceName(instrumentation.ComponentClient, nil),
28+
errCheck: func(err error) bool {
29+
return err != nil && !valkey.IsValkeyNil(err)
30+
},
2631
}
2732
}
2833

@@ -40,3 +45,11 @@ func WithService(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-io/valkey-go/valkey.go

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

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

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

@@ -203,12 +203,21 @@ func (c *client) startSpan(ctx context.Context, cmd command) (*tracer.Span, cont
203203

204204
func (c *client) finishSpan(span *tracer.Span, err error) {
205205
var opts []tracer.FinishOption
206-
if err != nil && !valkey.IsValkeyNil(err) {
206+
if c.cfg.errCheck(err) {
207207
opts = append(opts, tracer.WithError(err))
208208
}
209209
span.Finish(opts...)
210210
}
211211

212+
func (c *client) firstError(s []valkey.ValkeyResult) error {
213+
for _, result := range s {
214+
if err := result.Error(); c.cfg.errCheck(err) {
215+
return err
216+
}
217+
}
218+
return nil
219+
}
220+
212221
type commander interface {
213222
Commands() []string
214223
}

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

0 commit comments

Comments
 (0)