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(NODE-5968): Container and Kubernetes Awareness #4005
feat(NODE-5968): Container and Kubernetes Awareness #4005
Conversation
6da9072
to
7140454
Compare
added tests - no docker tests removed extraneous export and newline removed extraneous export and newline removed unnecesary helper funcs removed unnecesary let, changed to const added more tests to comply w kickoff - no docker tests still cleared up logic
7140454
to
03e5e13
Compare
6355e4e
to
25d1b44
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.
two small comments, otherwise LGTM.
src/connection_string.ts
Outdated
@@ -551,6 +551,7 @@ export function parseOptions( | |||
); | |||
|
|||
mongoOptions.metadata = makeClientMetadata(mongoOptions); | |||
mongoOptions.extendedMetadata = addContainerMetadata(mongoOptions.metadata); |
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.
we might want to add a catch
block, like Anna did when parallelizing dns lookup and srv resolution, so that this doesn't result in an unhandled promise rejection on the off chance it rejects
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.
Good catch.
Why are we calling this up here again? seems like addContainerMetadata
has what it needs down in prepareHandshake no?
Could we pass the LimitedSizeDocument down on options a different 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.
We cache the promise during options parsing so that we only construct the extended metadata once. Then we just await
it during every call to connect. Aditi filed https://jira.mongodb.org/browse/NODE-5994 to make metadata
private, remove extendedMetadata
and just store its result in metadata
in driver v7
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.
Ah ok, thanks for the context sorry I missed that.
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.
Added in the catch block!
if (isDocker == null) { | ||
dockerPromise ??= fs.access('/.dockerenv').then( | ||
() => true, | ||
() => false | ||
); | ||
isDocker = await dockerPromise; | ||
} | ||
|
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.
(optional) the original suggestion was intended to make it unnecessary to cache two variables. but we can leave it as-is too.
if (isDocker == null) { | |
dockerPromise ??= fs.access('/.dockerenv').then( | |
() => true, | |
() => false | |
); | |
isDocker = await dockerPromise; | |
} | |
dockerPromise ??= fs.access('/.dockerenv').then( | |
() => true, | |
() => false | |
); | |
const isDocker = await dockerPromise; |
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.
Sounds good, made the change.
9a2a74f
to
2ac1971
Compare
fixed failing test fixed failing test
lint fix
2ac1971
to
e33b33d
Compare
synced new test files added support for error response added api docs made MongoServerError.errorResponse required + casted resulting type errors test(NODE-5992): fix env var restoration in tests (#4017) refactor(NODE-5903): add newline to stdio logging (#4018) fix(NODE-5985): throw Nodejs' certificate expired error when TLS fails to connect instead of `CERT_HAS_EXPIRED` (#4014) test(NODE-5962): gossip cluster time in utr (#4019) chore(NODE-5997): update saslprep to ^1.1.5 (#4023) feat(NODE-5968): container and Kubernetes awareness in client metadata (#4005) fix(NODE-5993): memory leak in the `Connection` class (#4022) added TODO(NODE-XXXX)
Description
Track user usage of containers.
What is changing?
When kubernetes is in
process.env
, we add a{orchestrator: 'kubernetes'}
field to theclient.env
field of the handshake document (if this does not make the document exceed 512 bytes).When '/.dockerenv' exists, we add a
{runtime: 'docker'}
field to theclient.env
field of the handshake document (if this does not make the document exceed 512 bytes).Note: We do not unit test docker, but I tested our logic for detecting docker on an EVG Host, ran Docker and detection succeeds for both root and non-root user cases.
Is there new documentation needed for these changes?
No, non-user facing change.
What is the motivation for this change?
Product wants to track user usage of docker and kubernetes.
Release Highlight
Container and Kubernetes Awareness
The Node.js driver now keeps track of container metadata in the
client.env.container
field of the handshake document.If space allows, the following metadata will be included in
client.env.container
:Note: If neither Kubernetes nor Docker is present,
client.env
will not have thecontainer
property.Double check the following
npm run check:lint
scripttype(NODE-xxxx)[!]: description
feat(NODE-1234)!: rewriting everything in coffeescript