-
Notifications
You must be signed in to change notification settings - Fork 227
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
feat: set "destination" context for "mongodb" instrumentation #1893
Conversation
Current status: just a first pass. TODOs:
|
TAV test run for me:
|
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.
code and concepts look solid and reflect what we discovered about this in slack today. Soft approving, presuming that the lint and ci pass.
// available via the `address` or `connectionId` field. | ||
// https://github.com/mongodb/node-mongodb-native/blob/dd356f0ede/lib/core/connection/apm.js#L155-L169 | ||
const address = event.address || event.connectionId | ||
let match |
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.
Capturing an online conversation here -- both the docs and some spot checking of a port-less mongo connection (mongoose.connect('mongodb://localhost/myapp', {useNewUrlParser: true});
) indicate that the address/connection ID will be normalized to include a port, even if the client is configured without one. If somehow a portless address does end up in there, we won't hard fail as exec
won't match and the conditional will fail.
Fixes #1892