contrib/database/sql: add db service name tag and _dd.dbm_trace_injected span tag #1448
Conversation
alexandre-normand
added
the
enhancement
alexandre-normand
force-pushed
the
alex.normand/add-db-service-name-to-injected-sql-tags
branch
from
d235547
to
23ec6e0
Compare
ddtrace/tracer/sqlcomment.go
Outdated
| @@ -83,6 +86,9 @@ func (c *SQLCommentCarrier) Inject(spanCtx ddtrace.SpanContext) error { | |||
| if v, ok := ctx.meta(ext.Version); ok { | |||
| version = v | |||
| } | |||
| if s := ctx.span; s != nil { | |||
There was a problem hiding this comment.
Any reason to create s here? Perhaps just if ctx.span != nil ?
There was a problem hiding this comment.
Good catch. Updated ✅
ddtrace/tracer/sqlcomment.go
Outdated
| @@ -141,6 +148,9 @@ func commentQuery(query string, tags map[string]string) string { | |||
| if query == "" { | |||
| return b.String() | |||
| } | |||
| if log.DebugEnabled() { | |||
There was a problem hiding this comment.
I don't think you need to check if DebugEnabled, you can just call log.Debug and it will not log if debug is disabled
There was a problem hiding this comment.
Yeah, I was wondering about that. The comment on that DebugEnabled said it was for things in "hot paths" to avoid the interface{} allocations. I wasn't sure if this would be considered a hot path or not. I'm fine to just switch to log.Debug.
There was a problem hiding this comment.
Updated ✅
|
@ajgajg1134 : I think I've addressed the comments. Thanks for the review 🙇 and let me know if there's anything else I should add/update prior to the merge. |
…of github.com:DataDog/dd-trace-go into alex.normand/add-db-service-name-to-injected-sql-tags
ajgajg1134
deleted the
alex.normand/add-db-service-name-to-injected-sql-tags
branch
This follows-up on #1226 where we added sql comment tag injection support. As we're fleshing out the feature, we decided to add the db service name to the injected tags so that we can then have a link from collected query activity back to the APM database service. Also, to facilitate being able to filter traces where we've injected trace context in sql comments, we decided on adding a
_dd.dbm_trace_injectedtag. Given how the go tracer is the first language being implemented, feel free to comment on the naming which I can take back to refine the spec that isn't 100% final yet.