contrib: Adding `WithCustomTag` to various integrations #1359

ajgajg1134 · 2022-06-27T19:36:32Z

Pulling from fork to local repo (#1309) This will allow our system tests to run

support custom tags for gorm.v1 instrumentation Fixes #1308

support custom tags for confluent-kafka-go/kafka instrumentation

support custom tags for google.golang.org/grpc instrumentation

duxing · 2022-06-27T21:22:58Z

@ajgajg1134 thank you! finally all the tests were passed

gbbr · 2022-06-29T07:40:59Z

contrib/confluentinc/confluent-kafka-go/kafka/kafka.go

+
+	if c.cfg.tagFns != nil {
+		for key, tagFn := range c.cfg.tagFns {
+			opts = append(opts, tracer.Tag(key, tagFn(msg)))
+		}
+	}
+


Suggested change

if c.cfg.tagFns != nil {

for key, tagFn := range c.cfg.tagFns {

opts = append(opts, tracer.Tag(key, tagFn(msg)))

}

}

if c.cfg.tagFns != nil {

for key, tagFn := range c.cfg.tagFns {

opts = append(opts, tracer.Tag(key, tagFn(msg)))

}

}

Please see point 3 in https://github.com/DataDog/dd-trace-go/wiki/Style-guidelines#general-guidelines. Happy to discuss these guidelines at any time and happy to change them if we see any issues.

gbbr · 2022-06-29T07:41:42Z

contrib/database/sql/conn.go

@@ -216,6 +216,13 @@ func (tp *traceParams) tryTrace(ctx context.Context, qtype queryType, query stri
 		tracer.SpanType(ext.SpanTypeSQL),
 		tracer.StartTime(startTime),
 	)
+


Same comment here.

contrib/confluentinc/confluent-kafka-go/kafka/option.go

gbbr · 2022-06-29T07:45:51Z

contrib/database/sql/option.go

@@ -88,6 +89,16 @@ func WithChildSpansOnly() Option {
 	}
 }

+// WithCustomTag will attach the value to the span tagged by the key.


Suggested change

// WithCustomTag will attach the value to the span tagged by the key.

// WithCustomTag will attach the key:value tag to all spans.

Is this correct?

For some reason the current explanation is a bit confusing.

contrib/database/sql/option.go

gbbr · 2022-06-29T07:49:11Z

contrib/google.golang.org/grpc/grpc.go

+
+	ret := make([]tracer.StartSpanOption, len(cfg.tags)+len(opts))
+	i := 0
+	for _, opt := range opts {
+		ret[i] = opt
+		i++
+	}
+	for key, tag := range cfg.tags {
+		ret[i] = tracer.Tag(key, tag)
+		i++
+	}


Suggested change

ret := make([]tracer.StartSpanOption, len(cfg.tags)+len(opts))

i := 0

for _, opt := range opts {

ret[i] = opt

i++

}

for key, tag := range cfg.tags {

ret[i] = tracer.Tag(key, tag)

i++

}

ret := make([]tracer.StartSpanOption, 0, len(cfg.tags)+len(opts))

for _, opt := range opts {

ret = append(ret, opt)

}

for key, tag := range cfg.tags {

ret = append(ret, tracer.Tag(key, tag))

}

Why not append?

gbbr · 2022-06-29T07:49:40Z

contrib/google.golang.org/grpc/grpc.go

@@ -31,6 +49,7 @@ func startSpanFromContext(
 		tracer.Tag(tagMethodName, method),
 		tracer.SpanType(ext.AppTypeRPC),
 	)
+


Is this really needed?

I didn't check the style guidance prior to submitting the PR.
it's my personal preference for better legibility. I will remove the empty lines

gbbr · 2022-06-29T07:50:28Z

contrib/gorm.io/gorm.v1/option.go

+		if cfg.tagFns == nil {
+			cfg.tagFns = make(map[string]func(db *gorm.DB) interface{})
+		}
+


Same comment here about spacing around curly braces.

duxing · 2022-06-29T08:21:38Z

@gbbr I've addressed your feedback on my own PR (#1309 ) but it's not reflected here.
(this PR is created since my own PR fails CI tests, creating noise)

duxing · 2022-07-01T22:36:50Z

thank @ajgajg1134 for updating the branch.
@gbbr lmk if there's any outstanding comments that need to be addressed. I may have missed some of your feedback

gbbr

Thanks!

gbbr · 2022-07-04T08:31:02Z

contrib/confluentinc/confluent-kafka-go/kafka/kafka_test.go

+		}
+	}()
+
+	_ = (<-c.Events()).(*kafka.Message)


Suggested change

_ = (<-c.Events()).(*kafka.Message)

<-c.Events()

Why does it need a cast and a _? Or are you trying to test the type of the returned value, in which case ignore my comment.

the object from the channel is not consumed and what you suggested is preferred.

gbbr · 2022-07-04T08:31:39Z

contrib/database/sql/conn.go

+			opts = append(opts, tracer.Tag(key, tag))
+		}
+	}
+


Suggested change

gbbr · 2022-07-04T08:35:53Z

contrib/google.golang.org/grpc/grpc.go

@@ -22,6 +22,21 @@ import (
 	"google.golang.org/grpc/status"
 )

+func addCustomTags(cfg *config, opts ...tracer.StartSpanOption) []tracer.StartSpanOption {


Maybe this is better suited as a method on *config inside option.go?

// appendOpts creates a new set of StartSpanOptions by appending all options defined by configuration // to opts. func (cfg *config) appendOpts(opts ...tracer.StartSpanOption) []tracer.StartSpanOption {

You can probably include tracer.WithAnalytics into the body of this method because it seems to be added everywhere and is bound to config, unlike tracer.Measured() below. It will simplify things a bit.

gbbr · 2022-07-04T08:40:16Z

contrib/gorm.io/gorm.v1/gorm.go

@@ -120,6 +120,14 @@ func after(db *gorm.DB, operationName string, cfg *config) {
 		opts = append(opts, tracer.Tag(ext.EventSampleRate, cfg.analyticsRate))
 	}

+	if cfg.tagFns != nil {
+		for key, tagFn := range cfg.tagFns {
+			if tagFn != nil {


You are already checking nil in option.go. Is it necessary here too?

it still is in case WithCustomTag is never invoked.
but I do prefer creating an empty map as default config to avoid this nil check. it would also prevent concurrency issue

duxing · 2022-07-05T22:34:09Z

sorry to bother you again @ajgajg1134: I've addressed @gbbr 's feedback on my branch (#1309 )
can you pull and update this PR? Thanks in advance!

ajgajg1134 · 2022-07-14T20:09:44Z

👍 sorry for the delay @duxing , I've just merged the changes in

duxing · 2022-07-14T22:33:34Z

thx @ajgajg1134 !

@gbbr can you take another look? I'd love to see these methods being available in the next release.

duxing · 2022-07-29T08:31:41Z

hi @gbbr ! it's been almost a month since I got your last feedback. Can you take another look?

duxing · 2022-08-11T17:53:39Z

hi @ajgajg1134 @gbbr

any updates on this PR?

gbbr

LGTM! Sorry for the delay!

xdu-opendoor and others added 14 commits May 25, 2022 16:20

contrib/gorm.io/gorm.v1: adding WithCustomTag

f01e184

support custom tags for gorm.v1 instrumentation Fixes #1308

contrib/confluentinc/confluent-kafka-go/kafka: adding WithCustomTag

f8c0559

support custom tags for confluent-kafka-go/kafka instrumentation

contrib/database/sql: adding WithCustomTag

97b98bd

support custom tags for confluent-kafka-go/kafka instrumentation

contrib/google.golang.org/grpc: adding WithCustomTag

ce2377c

support custom tags for google.golang.org/grpc instrumentation

Merge branch 'main' into v1

44f1af4

fixing lint

b34c1ef

Merge branch 'main' into v1

7791857

adding unit tests

fa1a32d

Merge branch 'v1' of github.com:duxing/dd-trace-go into v1

be8d52d

fixing tests for integration environment

d3e6207

adding WithChildSpanOnly to gorm.io test

52921ec

correcting typo

ba1c464

change test condition

0cfc41f

Merge branch 'DataDog:main' into v1

3d28837

ajgajg1134 added this to the Triage milestone Jun 27, 2022

ajgajg1134 requested a review from a team June 27, 2022 19:36

ajgajg1134 changed the title ~~Duxing/with custom tag~~ contrib: Adding WithCustomTag to various integrations Jun 27, 2022

Merge branch 'main' into duxing/WithCustomTag

4f31706

gbbr reviewed Jun 29, 2022

View reviewed changes

address feedback

4be53e6

ajgajg1134 added 2 commits June 29, 2022 10:23

Merge remote-tracking branch 'duxing/v1' into duxing/WithCustomTag

7830af8

Merge branch 'main' into duxing/WithCustomTag

1e2d3f9

gbbr reviewed Jul 4, 2022

View reviewed changes

xdu-opendoor and others added 3 commits July 5, 2022 15:27

addressing feedback

bbd4673

Merge branch 'DataDog:main' into v1

a4a48ed

fixing lint

37fef06

Merge remote-tracking branch 'duxing/v1' into duxing/WithCustomTag

b4a432a

gbbr previously approved these changes Aug 12, 2022

View reviewed changes

Merge branch 'main' into duxing/WithCustomTag

2847535

ajgajg1134 dismissed gbbr’s stale review via 2847535 August 12, 2022 13:15

gbbr approved these changes Aug 12, 2022

View reviewed changes

ajgajg1134 merged commit 7ac0936 into main Aug 12, 2022

ajgajg1134 deleted the duxing/WithCustomTag branch August 12, 2022 14:00

ajgajg1134 mentioned this pull request Aug 12, 2022

adding WithCustomTag to a few libs in contrib/: gorm/gorm.v1, confluentinc/confluent-kafka-go/kafka, database/sql, google.golang.org/grpc #1309

Closed

zarirhamza mentioned this pull request Mar 15, 2023

Request: example for custom tags for contrib/gorm.io #1308

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

contrib: Adding `WithCustomTag` to various integrations #1359

contrib: Adding `WithCustomTag` to various integrations #1359

ajgajg1134 commented Jun 27, 2022

duxing commented Jun 27, 2022

gbbr Jun 29, 2022

gbbr Jun 29, 2022

gbbr Jun 29, 2022

gbbr Jun 29, 2022

gbbr Jun 29, 2022

duxing Jun 29, 2022

gbbr Jun 29, 2022

duxing commented Jun 29, 2022

duxing commented Jul 1, 2022

gbbr left a comment

gbbr Jul 4, 2022

duxing Jul 5, 2022

gbbr Jul 4, 2022

gbbr Jul 4, 2022

duxing Jul 5, 2022

gbbr Jul 4, 2022

duxing Jul 5, 2022 •

edited

duxing commented Jul 5, 2022

ajgajg1134 commented Jul 14, 2022

duxing commented Jul 14, 2022

duxing commented Jul 29, 2022

duxing commented Aug 11, 2022

gbbr left a comment

	// WithCustomTag will attach the value to the span tagged by the key.
	// WithCustomTag will attach the key:value tag to all spans.

contrib: Adding WithCustomTag to various integrations #1359

contrib: Adding WithCustomTag to various integrations #1359

Conversation

ajgajg1134 commented Jun 27, 2022

duxing commented Jun 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duxing commented Jun 29, 2022

duxing commented Jul 1, 2022

gbbr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duxing Jul 5, 2022 • edited

Choose a reason for hiding this comment

duxing commented Jul 5, 2022

ajgajg1134 commented Jul 14, 2022

duxing commented Jul 14, 2022

duxing commented Jul 29, 2022

duxing commented Aug 11, 2022

gbbr left a comment

Choose a reason for hiding this comment

contrib: Adding `WithCustomTag` to various integrations #1359

contrib: Adding `WithCustomTag` to various integrations #1359

duxing Jul 5, 2022 •

edited