Support valkey-go tracing #3081

keisku · 2025-01-12T11:03:06Z

What does this PR do?

Support https://github.com/valkey-io/valkey-go

Example:

Motivation

#2920

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!

datadog-datadog-prod-us1 · 2025-01-12T11:09:50Z

Datadog Report

Branch report: keisku/valkey-go-support
Commit report: 3939a5a
Test service: dd-trace-go

❌ 8 Failed (0 Known Flaky), 5167 Passed, 70 Skipped, 2m 54.4s Total Time

❌ Failed Tests (8)

This report shows up to 5 failed tests.

TestNewClient - gopkg.in/DataDog/dd-trace-go.v1/contrib/valkey-go - Details
Expand for error
```
 Failed
 
 === RUN   TestNewClient
 --- FAIL: TestNewClient (0.11s)
```

TestNewClient/Test_GET_command_with_stream_with_env_vars - gopkg.in/DataDog/dd-trace-go.v1/contrib/valkey-go - Details

Expand for error

 Failed
 
 === RUN   TestNewClient/Test_GET_command_with_stream_with_env_vars
     valkey_test.go:287: 
         	Error Trace:	/home/runner/work/dd-trace-go/dd-trace-go/contrib/valkey-go/valkey_test.go:287
         	Error:      	Received unexpected error:
         	            	dial tcp 127.0.0.1:6380: connect: connection refused
         	Test:       	TestNewClient/Test_GET_command_with_stream_with_env_vars
         	Messages:   	Test GET command with stream with env vars
     --- FAIL: TestNewClient/Test_GET_command_with_stream_with_env_vars (0.02s)

TestNewClient/Test_HMGET_command_with_cache - gopkg.in/DataDog/dd-trace-go.v1/contrib/valkey-go - Details

Expand for error

 Failed
 
 === RUN   TestNewClient/Test_HMGET_command_with_cache
     valkey_test.go:287: 
         	Error Trace:	/home/runner/work/dd-trace-go/dd-trace-go/contrib/valkey-go/valkey_test.go:287
         	Error:      	Received unexpected error:
         	            	dial tcp 127.0.0.1:6380: connect: connection refused
         	Test:       	TestNewClient/Test_HMGET_command_with_cache
         	Messages:   	Test HMGET command with cache
     --- FAIL: TestNewClient/Test_HMGET_command_with_cache (0.01s)

TestNewClient/Test_invalid_password - gopkg.in/DataDog/dd-trace-go.v1/contrib/valkey-go - Details

Expand for error

 Failed
 
 === RUN   TestNewClient/Test_invalid_password
     valkey_test.go:67: 
         	Error Trace:	/home/runner/work/dd-trace-go/dd-trace-go/contrib/valkey-go/valkey_test.go:67
         	            				/home/runner/work/dd-trace-go/dd-trace-go/contrib/valkey-go/valkey_test.go:289
         	Error:      	Error message not equal:
         	            	expected: "WRONGPASS invalid username-password pair or user is disabled."
         	            	actual  : "dial tcp 127.0.0.1:6380: connect: connection refused"
         	Test:       	TestNewClient/Test_invalid_password
 ...

TestNewClient/Test_invalid_username - gopkg.in/DataDog/dd-trace-go.v1/contrib/valkey-go - Details

Expand for error

 Failed
 
 === RUN   TestNewClient/Test_invalid_username
     valkey_test.go:56: 
         	Error Trace:	/home/runner/work/dd-trace-go/dd-trace-go/contrib/valkey-go/valkey_test.go:56
         	            				/home/runner/work/dd-trace-go/dd-trace-go/contrib/valkey-go/valkey_test.go:289
         	Error:      	Error message not equal:
         	            	expected: "WRONGPASS invalid username-password pair or user is disabled."
         	            	actual  : "dial tcp 127.0.0.1:6380: connect: connection refused"
         	Test:       	TestNewClient/Test_invalid_username
 ...

pr-commenter · 2025-01-12T11:39:36Z

Benchmarks

Benchmark execution time: 2025-01-23 15:27:59

Comparing candidate commit 25088d1 in PR branch keisku/valkey-go-support with baseline commit 382c311 in branch main.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 59 metrics, 0 unstable metrics.

contrib/valkey-go/valkey.go

darccio · 2025-01-17T14:58:09Z

@keisku Thanks for contributing this integration! Please fix the conflicts and this should be good to go.

Also remember to create the same PR against v2-dev. Keep in mind that this branch has some major differences, including an instrumentation package for contribs. Check the same integration for Redis to learn more about how to integrate a library in our contribs in our upcoming v2 version.

contrib/valkey-go/example_test.go

contrib/valkey-go/valkey.go

Signed-off-by: keisku <keisuke.umegaki@datadoghq.com>

contrib/valkey-go/valkey.go

rarguelloF · 2025-01-20T17:16:11Z

contrib/valkey-go/valkey.go

+	skipRawCommand bool
+}
+
+func (c *coreClient) peerTags() []tracer.StartSpanOption {


we shouldn't be setting peer.* tags in this integration. You can mirror the behavior of the go-redis integration for these tags: https://github.com/DataDog/dd-trace-go/blob/main/contrib/redis/go-redis.v9/redis.go#L75-L111 (changing redis -> valkey where appropiate)

we shouldn't be setting peer.* tags in this integration.

Could you explain the reason for this?

After reading the following docs, I understood peer.* is necessary tags now. Since the last commit for go-redis.v9 was two years ago, I assumed that it was not required at that time.

DD_APM_PEER_TAGS_AGGREGATION

The new way of overriding service names

I'd appreciate it if you could let me know if my understanding is incorrect.

The agent checks a list of tags to compute peer.service, and I think both out.host and peer.hostname would be considered in this case. But for consistency with other integrations, I think its preferred to just set out.host (I am asking around if this preference has changed recently and if we have any documentation about it)

contrib/valkey-go/valkey.go

contrib/valkey-go/option.go

contrib/valkey-go/example_test.go

rarguelloF · 2025-01-22T13:59:07Z

contrib/valkey-go/valkey.go

+	ipAddr := net.ParseIP(c.host)
+	var peerHostKey string
+	if ipAddr == nil {
+		peerHostKey = ext.PeerHostname
+	} else if ipAddr.To4() != nil {
+		peerHostKey = ext.PeerHostIPV4
+	} else {
+		peerHostKey = ext.PeerHostIPV6
+	}


Suggested change

ipAddr := net.ParseIP(c.host)

var peerHostKey string

if ipAddr == nil {

peerHostKey = ext.PeerHostname

} else if ipAddr.To4() != nil {

peerHostKey = ext.PeerHostIPV4

} else {

peerHostKey = ext.PeerHostIPV6

}

this is not necessary, the integration should only set out.host when unsure whether the value is an IP address or a hostname. We already have this sort of logic in place in the backend.

out.host seems to be deprecated. So, should we set network.destination.name or network.destination.ip?

dd-trace-go/ddtrace/ext/tags.go

Lines 11 to 13 in 241a6f1

// TargetHost sets the target host address.

// Deprecated: Use NetworkDestinationName instead for hostname and NetworkDestinationIP for IP addresses

TargetHost = "out.host"

rarguelloF · 2025-01-22T14:01:51Z

contrib/valkey-go/valkey.go

+	skipRawCommand bool
+}
+
+func (c *coreClient) peerTags() []tracer.StartSpanOption {


The agent checks a list of tags to compute peer.service, and I think both out.host and peer.hostname would be considered in this case. But for consistency with other integrations, I think its preferred to just set out.host (I am asking around if this preference has changed recently and if we have any documentation about it)

rarguelloF · 2025-01-22T14:41:14Z

contrib/valkey-go/valkey.go

+		tracer.Tag(ext.ValkeyDatabaseIndex, c.option.SelectDB),
+		tracer.Tag("db.out", c.option.SelectDB),


Suggested change

tracer.Tag(ext.ValkeyDatabaseIndex, c.option.SelectDB),

tracer.Tag("db.out", c.option.SelectDB),

tracer.Tag("out.db", c.option.SelectDB),

db.out -> out.db

only one of the 2 tags (we want to avoid repeated information)

Also, if you don't mind, could you create a new constant ext.OutDB for out.db to prevent this kind of mistake to happen in the future? 🙏

Since out.host was deprecated, should we avoid setting out.db?
Or should we keep setting out.db for consistency of redis integration?

dc939eb marked it as deprecated.

rarguelloF · 2025-01-22T16:22:55Z

contrib/valkey-go/valkey.go

+		opts = append(opts, []tracer.StartSpanOption{
+			// valkeyotel tags
+			tracer.Tag("db.stmt_size", input.size),
+			tracer.Tag("db.operation", input.command),


We don't need to add obfuscation, since its already done in the agent (the code you linked). However I just noticed there's an issue, since that code will only run if span.type is redis, so it won't run for valkey. I will talk to the apm-agent team about this.

But basically what we want to do here is:

if !input.skipRawCommand { opts = append(opts, tracer.Tag(ext.ValkeyRawCommand, input.command)) }

Also it is preferred to set only the raw command tag, since its the one the agent checks for obfuscation (when the logic is updated to check for valkey as well).

github-actions bot added the apm:ecosystem contrib/* related feature requests or bugs label Jan 12, 2025

keisku force-pushed the keisku/valkey-go-support branch 8 times, most recently from e41ab88 to 7c97f5b Compare January 15, 2025 08:55

Support valkey-go tracing

6aa8d69

keisku force-pushed the keisku/valkey-go-support branch from 7c97f5b to 6aa8d69 Compare January 15, 2025 10:56

keisku marked this pull request as ready for review January 15, 2025 11:20

keisku requested review from a team as code owners January 15, 2025 11:20

keisku commented Jan 15, 2025

View reviewed changes

contrib/valkey-go/valkey.go Outdated Show resolved Hide resolved

keisku force-pushed the keisku/valkey-go-support branch from 11e7c51 to 2750600 Compare January 16, 2025 03:33

as per DD_APM_PEER_TAGS_AGGREGATION in agent

fe59e6d

keisku force-pushed the keisku/valkey-go-support branch from 2750600 to fe59e6d Compare January 16, 2025 03:36

Merge branch 'main' into keisku/valkey-go-support

26c6621

darccio approved these changes Jan 17, 2025

View reviewed changes

hannahkm requested changes Jan 17, 2025

View reviewed changes

contrib/valkey-go/example_test.go Show resolved Hide resolved

contrib/valkey-go/valkey.go Outdated Show resolved Hide resolved

keisku added 5 commits January 18, 2025 00:57

Merge remote-tracking branch 'origin' into keisku/valkey-go-support

9479b69

update example as per best practice

49ebeed

Signed-off-by: keisku <keisuke.umegaki@datadoghq.com>

fix doc

90f6c43

Signed-off-by: keisku <keisuke.umegaki@datadoghq.com>

fix spanName

46cacc3

Signed-off-by: keisku <keisuke.umegaki@datadoghq.com>

update contribIntegrations

6d5b6f6

Signed-off-by: keisku <keisuke.umegaki@datadoghq.com>

keisku force-pushed the keisku/valkey-go-support branch from fbb2f5f to 6d5b6f6 Compare January 18, 2025 01:22

keisku requested a review from hannahkm January 18, 2025 01:24

darccio assigned darccio and rarguelloF Jan 20, 2025

rarguelloF requested changes Jan 20, 2025

View reviewed changes

keisku added 9 commits January 21, 2025 00:34

rename to github.com/valkey-io/valkey-go

e99f4d7

remove App Analytics which was deprecated

4031cd7

for consistency with the other examples

aae0a85

remove ValkeyClientCommand* tags

14b4d21

there is not concept of db.instance in valkey / redis

6197ac1

remove db.type:valkey

7b7329f

remove db.application

fca4a18

fix valkey.raw_command behavior

e7758a3

add db.out tag

cdb4129

keisku requested a review from rarguelloF January 21, 2025 01:56

hannahkm approved these changes Jan 21, 2025

View reviewed changes

keisku mentioned this pull request Jan 21, 2025

Support obfuscation for valkey spans DataDog/datadog-agent#33187

Draft

rarguelloF requested changes Jan 22, 2025

View reviewed changes

keisku added 2 commits January 23, 2025 08:50

typo

21434c3

remove valkeyotel tags

27b0fec

keisku force-pushed the keisku/valkey-go-support branch from 787b31f to 27b0fec Compare January 23, 2025 08:51

keisku added 4 commits January 23, 2025 14:35

rename to out.db (ext.TargetDB)

06360d2

remove peer.* tags

b8c5c35

rework DD_TRACE_VALKEY_RAW_COMMAND behavior

fba84df

fix error handling when a span finishes

25088d1

keisku requested a review from rarguelloF January 23, 2025 15:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support valkey-go tracing #3081

Support valkey-go tracing #3081

keisku commented Jan 12, 2025 •

edited

Loading

datadog-datadog-prod-us1 bot commented Jan 12, 2025 •

edited

Loading

pr-commenter bot commented Jan 12, 2025 •

edited

Loading

darccio commented Jan 17, 2025

rarguelloF Jan 20, 2025

keisku Jan 21, 2025

rarguelloF Jan 22, 2025 •

edited

Loading

rarguelloF Jan 22, 2025

keisku Jan 23, 2025

rarguelloF Jan 22, 2025 •

edited

Loading

rarguelloF Jan 22, 2025

keisku Jan 23, 2025

rarguelloF Jan 22, 2025

	// TargetHost sets the target host address.
	// Deprecated: Use NetworkDestinationName instead for hostname and NetworkDestinationIP for IP addresses
	TargetHost = "out.host"

		tracer.Tag(ext.ValkeyDatabaseIndex, c.option.SelectDB),
		tracer.Tag("db.out", c.option.SelectDB),

	tracer.Tag(ext.ValkeyDatabaseIndex, c.option.SelectDB),
	tracer.Tag("db.out", c.option.SelectDB),
	tracer.Tag("out.db", c.option.SelectDB),

Support valkey-go tracing #3081

Are you sure you want to change the base?

Support valkey-go tracing #3081

Conversation

keisku commented Jan 12, 2025 • edited Loading

What does this PR do?

Motivation

Reviewer's Checklist

datadog-datadog-prod-us1 bot commented Jan 12, 2025 • edited Loading

Datadog Report

❌ Failed Tests (8)

pr-commenter bot commented Jan 12, 2025 • edited Loading

Benchmarks

darccio commented Jan 17, 2025

rarguelloF Jan 20, 2025

Choose a reason for hiding this comment

keisku Jan 21, 2025

Choose a reason for hiding this comment

rarguelloF Jan 22, 2025 • edited Loading

Choose a reason for hiding this comment

rarguelloF Jan 22, 2025

Choose a reason for hiding this comment

keisku Jan 23, 2025

Choose a reason for hiding this comment

rarguelloF Jan 22, 2025 • edited Loading

Choose a reason for hiding this comment

rarguelloF Jan 22, 2025

Choose a reason for hiding this comment

keisku Jan 23, 2025

Choose a reason for hiding this comment

rarguelloF Jan 22, 2025

Choose a reason for hiding this comment

keisku commented Jan 12, 2025 •

edited

Loading

datadog-datadog-prod-us1 bot commented Jan 12, 2025 •

edited

Loading

pr-commenter bot commented Jan 12, 2025 •

edited

Loading

rarguelloF Jan 22, 2025 •

edited

Loading

rarguelloF Jan 22, 2025 •

edited

Loading