-
Notifications
You must be signed in to change notification settings - Fork 441
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/database/sql: add in ddh, dddb propagation #2550
contrib/database/sql: add in ddh, dddb propagation #2550
Conversation
BenchmarksBenchmark execution time: 2024-02-22 20:03:14 Comparing candidate commit b51d551 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 40 metrics, 1 unstable metrics. |
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.
Nice work, but I think we need a slightly different approach because of the issue mentioned in one of the comments (it will only work when there's a parent span + the parent span is modified).
The code in this integration is a little bit tricky, since it starts the span after the actual query is executed and the DBM comments are injected (there is a comment in the injectComments
function explaining the rationale behind this).
For this reason, probably the simplest approach to achieve this is:
- Update the
tracer.SQLCommentCarrier
to include 2 new fields:PeerHostname
andPeerDBName
and use those for the comment injection. - Update
injectComments
to set these 2 new fields accessing the corresponding tags fromtc.meta
.
Please let me know if you agree and/or wanna discuss the approach in more detail! 🙇
d6d365a
to
17cb8aa
Compare
92d604b
to
822a758
Compare
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.
Looking great! Just a small change in the newly introduced ParseDSN
logic changes and should be good to go!
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.
One last comment I missed in the previous review, which I noticed from the failed tests in the CI.
5456182
to
5274823
Compare
c44de59
to
134e57c
Compare
44ec0de
to
60ec21b
Compare
60ec21b
to
2138e10
Compare
/merge |
❌ MergeQueue You are not allowed to use the merge queue towards If you need support, contact us on Slack #ci-interfaces with those details! |
/merge |
❌ MergeQueue You are not allowed to use the merge queue towards If you need support, contact us on Slack #ci-interfaces with those details! |
/merge |
🚂 MergeQueue Pull request added to the queue. This build is going to start soon! (estimated merge in less than 58s) Use |
What does this PR do?
This PR adds in the
ddh
anddddb
peer service tags to SQL comments.Motivation
This change allows the DBM product to propagate service name information which maps with the inferred entities/services used by APM.
Reviewer's Checklist
For Datadog employees:
@DataDog/security-design-and-guidance
.Unsure? Have a question? Request a review!