fix(valkey): Make error checking configurable and fix inconsistency

[rwynn]

All tracer.WithError calls first run through a configurable error check function.

What does this PR do?

Fixes an inconsistency in the Do method where errors were not being checked for valkey nil.

Makes error checking configurable.

Motivation

Reviewer's Checklist

• Changed code has unit tests for its functionality at or near 100% coverage.
• System-Tests covering this feature have been added and enabled with the va.b.c-dev version tag.
• There is a benchmark for any new code, or changes to existing code.
• If this interacts with the agent in a new way, a system test has been added.
• Add an appropriate team label so this PR gets put in the right place for the release notes.
• Non-trivial go.mod changes, e.g. adding new modules, are reviewed by @DataDog/dd-trace-go-guild.
• For internal contributors, a matching PR should be created to the v2-dev branch and reviewed by @DataDog/apm-go.

Unsure? Have a question? Request a review!

[rwynn]

A valkey nil error will get attached to the span here currently.

[darccio]

@keisku Please, can you check this?

[rwynn]

Updated this to apply same treatment to rueidis to maintain consistency.

[keisku]

@rwynn

Thank you for fixing this. I wonder why changes other than the following are necessary.

func (c *client) Do(ctx context.Context, cmd valkey.Completed) valkey.ValkeyResult {
	span, ctx := c.startSpan(ctx, processCommand(&cmd))
	resp := c.client.Do(ctx, cmd)
	setClientCacheTags(span, resp)
- 	span.Finish(tracer.WithError(resp.Error()))
+ 	c.finishSpan(span, resp.Error())
	return resp
}

When do we need WithErrorCheck()? I think we need to separate concerns.

[rwynn]

@rwynn

Thank you for fixing this. I wonder why changes other than the following are necessary.

func (c *client) Do(ctx context.Context, cmd valkey.Completed) valkey.ValkeyResult {
	span, ctx := c.startSpan(ctx, processCommand(&cmd))
	resp := c.client.Do(ctx, cmd)
	setClientCacheTags(span, resp)
- 	span.Finish(tracer.WithError(resp.Error()))
+ 	c.finishSpan(span, resp.Error())
	return resp
}

When do we need WithErrorCheck()? I think we need to separate concerns.

Hi @keisku ,

Apologies, I didn't go into detail about the addition of WithErrorCheck in the description because we have been using it with other contrib plugins like aws and go-redis and I presumed that already justified it's existence.

dd-trace-go/contrib/aws/aws-sdk-go-v2/aws/option.go

Line 66 in 9da35e1

func WithErrorCheck(fn func(err error) bool) Option {

dd-trace-go/contrib/go-redis/redis.v8/option.go

Line 81 in 9da35e1

func WithErrorCheck(fn func(err error) bool) ClientOption {

We use WithErrorCheck to specify addition types of errors that we don't classify as errors that should not influence the error statistics reported when we check a service in Datadog. One example would be the use of WithErrorCheck included in the test I added. Often a http request into our services will first initiate a redis (or now valkey) get on a key. If the client quickly cancels this request before the get call completes we handle the resulting context.Canceled and return a http status 400 to the client. However, without our custom WithErrorCheck this trace is still marked as an error and negatively impacts the error rate as reported in Datadog. We generally like to see that error rate accurately report what we consider to be errors that need attention. The WithErrorCheck allows the user of the library to determine what those spurious errors might be.

I do see you point that this change address 2 separate concerns and could be 2 separate MRs. I can split into 2 if that is desirable.

[keisku]

Appreciate you explaining it!
For consistency of other redis integrations, adding WithErrorCheck makes sense to me.

dd-trace-go/contrib/redis/go-redis.v9/option.go

Lines 79 to 85 in a95495a

	// WithErrorCheck specifies a function fn which determines whether the passed
	// error should be marked as an error.
	func WithErrorCheck(fn func(err error) bool) ClientOption {
	return func(cfg *clientConfig) {
	cfg.errCheck = fn
	}
	}

Can we have the default errCheck like this? I think we can remove isTracedError().

func defaultConfig() *config {
	return &config{
		// Do not include the raw command by default since it could contain sensitive data.
		rawCommand:  internal.BoolEnv("DD_TRACE_VALKEY_RAW_COMMAND", false),
		serviceName: namingschema.ServiceName(defaultServiceName),
+ 		errCheck: func(err error) bool { return err != nil && !valkey.IsValkeyNil(err) },
	}
}

[rwynn]

thank you @keisku. I have updated the PR as you described and added an additional test.