-
Notifications
You must be signed in to change notification settings - Fork 223
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
fix: mongodb async context fix #3657
Changes from 5 commits
6437166
3f7d7e3
0e57880
d7ae1c0
f994ef0
9838d90
b67a5b1
629acd0
066de81
d5ac747
85e17d5
63abf30
1f863aa
527f669
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -73,6 +73,7 @@ var MODULES = [ | |
'mimic-response', | ||
'mongodb-core', | ||
'mongodb', | ||
'mongodb/lib/cmap/connection_pool', | ||
'mysql', | ||
'mysql2', | ||
'next/dist/server/api-utils/node', | ||
|
@@ -115,6 +116,8 @@ const IITM_MODULES = { | |
https: { instrumentImportMod: true }, | ||
ioredis: { instrumentImportMod: false }, | ||
knex: { instrumentImportMod: false }, | ||
// XXX: check this | ||
// mongodb: { instrumentImportMod: false }, | ||
pg: { instrumentImportMod: false }, | ||
}; | ||
|
||
|
@@ -358,35 +361,43 @@ Instrumentation.prototype._startHook = function () { | |
|
||
this._agent.logger.debug('adding hooks to Node.js module loader'); | ||
|
||
this._ritmHook = new RitmHook(this._patches.keys, function ( | ||
exports, | ||
name, | ||
basedir, | ||
) { | ||
const enabled = self._isModuleEnabled(name); | ||
var pkg, version; | ||
|
||
const isHandlingLambda = | ||
self._lambdaHandlerInfo && self._lambdaHandlerInfo.module === name; | ||
|
||
if (!isHandlingLambda && basedir) { | ||
pkg = path.join(basedir, 'package.json'); | ||
try { | ||
version = JSON.parse(fs.readFileSync(pkg)).version; | ||
} catch (e) { | ||
self._agent.logger.debug( | ||
'could not shim %s module: %s', | ||
name, | ||
e.message, | ||
); | ||
return exports; | ||
this._ritmHook = new RitmHook( | ||
this._patches.keys, | ||
// XXX: found this was not enabled so our redis instrumentations | ||
// may not be working as for now | ||
{ internals: true }, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note for reviewer: check the comment and give feedback please. Was okay to pass the new option? what about other instrumentations. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We weren't using
so I don't think this is what internals is about. I'm not positive, however. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
--- /Users/trentm/el/require-in-the-middle/index.js 2023-07-13 15:32:26
+++ ./node_modules/require-in-the-middle/index.js 2023-10-03 09:26:14
@@ -126,6 +126,7 @@
const self = this
const patching = new Set()
const internals = options ? options.internals === true : false
+ console.log('XXX ritm: internals=%s', internals);
const hasWhitelist = Array.isArray(modules)
debug('registering require hook')
@@ -204,6 +205,7 @@
}
moduleName = filename
} else if (hasWhitelist === true && modules.includes(filename)) {
+ console.log('XXX ritm abspath in allowlist: hasWhitelist=%s modules.includes(filename)=%s filename=%s', hasWhitelist, modules.includes(filename), filename);
// whitelist includes the absolute path to the file including extension
const parsedPath = path.parse(filename)
moduleName = parsedPath.name
@@ -227,6 +229,7 @@
if (hasWhitelist === true && modules.includes(moduleName) === false) {
if (modules.includes(fullModuleName) === false) return exports // abort if module name isn't on whitelist
+ console.log('XXX ritm allowlisted sub-module: moduleName=%s fullModuleName=%s filename=%s', moduleName, fullModuleName, filename)
// if we get to this point, it means that we're requiring a whitelisted sub-module
moduleName = fullModuleName
} else {
diff --git a/lib/instrumentation/index.js b/lib/instrumentation/index.js
index b90152f7..29e4bbb7 100644
--- a/lib/instrumentation/index.js
+++ b/lib/instrumentation/index.js
@@ -363,6 +363,7 @@ Instrumentation.prototype._startHook = function () {
name,
basedir,
) {
+ console.log('XXX RitmHook called:', name, basedir);
const enabled = self._isModuleEnabled(name);
var pkg, version;
and run examples/trace-redis.js:
The RITM There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I did not get those logs for if (hasWhitelist === true && modules.includes(moduleName) === false) {
if (modules.includes(fullModuleName) === false) return exports // abort if module name isn't on whitelist
console.log('XXX ritm allowlisted sub-module: moduleName=%s fullModuleName=%s filename=%s', moduleName, fullModuleName, filename)
// more code here ...
} In my case
The expression allows to pass sub-modules only if the root module is not in the whitelist. I this case the agent is instrumenting the main module and one sub-module. I think it works on I see two options here. Let me know what you think
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I'd prefer to avoid this. It will mean that we will be calling our Option 3: Remove that limitation in RITM. I think this is a bug in RITM that you cannot do this. Having 'mongodb' on the list of modules to patch shouldn't mean that you cannot patch There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I haven't finished this yet. There is also a potential hiccup in maintaining full support for https://www.elastic.co/guide/en/apm/agent/nodejs/current/agent-api.html#apm-add-patch I think it'll be fine. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This PR solves our use case for instrumenting module and sub-module at the same time. which is what we need for We still have to look at RITM but since the issue is only reported by us I think we have no rush. |
||
function (exports, name, basedir) { | ||
// RITM returns the path with extension for sub-modules | ||
// so we need to make sure is not there when patching | ||
if (name.endsWith('.js')) { | ||
name = name.substring(0, name.length - 3); | ||
} | ||
david-luna marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} else { | ||
version = process.versions.node; | ||
} | ||
|
||
return self._patchModule(exports, name, version, enabled, false); | ||
}); | ||
const enabled = self._isModuleEnabled(name); | ||
var pkg, version; | ||
|
||
const isHandlingLambda = | ||
self._lambdaHandlerInfo && self._lambdaHandlerInfo.module === name; | ||
|
||
if (!isHandlingLambda && basedir) { | ||
pkg = path.join(basedir, 'package.json'); | ||
try { | ||
version = JSON.parse(fs.readFileSync(pkg)).version; | ||
} catch (e) { | ||
self._agent.logger.debug( | ||
'could not shim %s module: %s', | ||
name, | ||
e.message, | ||
); | ||
return exports; | ||
} | ||
} else { | ||
version = process.versions.node; | ||
} | ||
|
||
return self._patchModule(exports, name, version, enabled, false); | ||
}, | ||
); | ||
|
||
this._iitmHook = IitmHook( | ||
Object.keys(IITM_MODULES), | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and other contributors where applicable. | ||
* Licensed under the BSD 2-Clause License; you may not use this file except in | ||
* compliance with the BSD 2-Clause License. | ||
*/ | ||
|
||
'use strict'; | ||
|
||
const { AsyncResource } = require('async_hooks'); | ||
|
||
const semver = require('semver'); | ||
|
||
module.exports = (mod, agent, { version, enabled }) => { | ||
if (!enabled) return mod; | ||
if (!semver.satisfies(version, '>=3.3 <7.0')) { | ||
agent.logger.debug( | ||
'mongodb version %s not instrumented (mongodb <3.3 is instrumented via mongodb-core)', | ||
version, | ||
); | ||
return mod; | ||
} | ||
|
||
if (mod.ConnectionPool) { | ||
class ConnectionPoolTraced extends mod.ConnectionPool { | ||
checkOut(callback) { | ||
return super.checkOut(AsyncResource.bind(callback)); | ||
} | ||
} | ||
|
||
Object.defineProperty(mod, 'ConnectionPool', { | ||
enumerable: true, | ||
get: function () { | ||
return ConnectionPoolTraced; | ||
}, | ||
}); | ||
|
||
return mod; | ||
} | ||
}; |
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.
Note for reviewer: this is a work in progress though I do not mind to have it in a separate 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.
Adding ESM in a separate PR would be helpful. Keeping this PR to a small change for using AsyncResource.bind might be helpful to compare with an equivalent OTel change.