-
Notifications
You must be signed in to change notification settings - Fork 435
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
contrib: Adding WithCustomTag
to various integrations
#1359
Conversation
support custom tags for gorm.v1 instrumentation Fixes #1308
support custom tags for confluent-kafka-go/kafka instrumentation
support custom tags for confluent-kafka-go/kafka instrumentation
support custom tags for google.golang.org/grpc instrumentation
WithCustomTag
to various integrations
@ajgajg1134 thank you! finally all the tests were passed |
|
||
if c.cfg.tagFns != nil { | ||
for key, tagFn := range c.cfg.tagFns { | ||
opts = append(opts, tracer.Tag(key, tagFn(msg))) | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
contrib/database/sql/conn.go
Outdated
@@ -216,6 +216,13 @@ func (tp *traceParams) tryTrace(ctx context.Context, qtype queryType, query stri | |||
tracer.SpanType(ext.SpanTypeSQL), | |||
tracer.StartTime(startTime), | |||
) | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment here.
contrib/database/sql/option.go
Outdated
@@ -88,6 +89,16 @@ func WithChildSpansOnly() Option { | |||
} | |||
} | |||
|
|||
// WithCustomTag will attach the value to the span tagged by the key. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// 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.
|
||
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++ | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
?
@@ -31,6 +49,7 @@ func startSpanFromContext( | |||
tracer.Tag(tagMethodName, method), | |||
tracer.SpanType(ext.AppTypeRPC), | |||
) | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this really needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
contrib/gorm.io/gorm.v1/option.go
Outdated
if cfg.tagFns == nil { | ||
cfg.tagFns = make(map[string]func(db *gorm.DB) interface{}) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment here about spacing around curly braces.
thank @ajgajg1134 for updating the branch. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
} | ||
}() | ||
|
||
_ = (<-c.Events()).(*kafka.Message) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_ = (<-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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the object from the channel is not consumed and what you suggested is preferred.
contrib/database/sql/conn.go
Outdated
opts = append(opts, tracer.Tag(key, tag)) | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -22,6 +22,21 @@ import ( | |||
"google.golang.org/grpc/status" | |||
) | |||
|
|||
func addCustomTags(cfg *config, opts ...tracer.StartSpanOption) []tracer.StartSpanOption { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed
contrib/gorm.io/gorm.v1/gorm.go
Outdated
@@ -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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are already checking nil
in option.go
. Is it necessary here too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
sorry to bother you again @ajgajg1134: I've addressed @gbbr 's feedback on my branch (#1309 ) |
👍 sorry for the delay @duxing , I've just merged the changes in |
thx @ajgajg1134 ! @gbbr can you take another look? I'd love to see these methods being available in the next release. |
hi @gbbr ! it's been almost a month since I got your last feedback. Can you take another look? |
hi @ajgajg1134 @gbbr any updates on this PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Sorry for the delay!
Pulling from fork to local repo (#1309) This will allow our system tests to run