instrument by resolved path, include package versions in events #50
Conversation
|
@ChristopherBiscardi you have bandwidth to review this one? I can if not, but I feel like you might have more useful background context. |
| @@ -4,7 +4,7 @@ const child_process = require("child_process"), | |||
| tracker = require("../async_tracker"), | |||
| event = require("../event"); | |||
|
|
|||
| instrumentChildProcess(child_process); | |||
| instrumentChildProcess(child_process, {}); | |||
There was a problem hiding this comment.
would be nice to set this second arg as a default arg instead of passing it in like:
let instrumentChildProcess = (child_process, opts = {}) => {but the amount of code in that fn is pretty low, so maybe:
shimmer.wrap(child_process, "execFile", wrapExecLike("execFile", opts.packageVersion || {}));There was a problem hiding this comment.
yeah, I think at some point in the past they were optional, but then I moved the default up to instrumentLoad and removed it from the individual instrumentations (this was before there were tests which poked at the instrumentations directly.)
👍
lib/magic/http-magic.test.js
Outdated
| @@ -4,7 +4,7 @@ const http = require("http"), | |||
| tracker = require("../async_tracker"), | |||
| event = require("../event"); | |||
|
|
|||
| instrumentHttp(http); | |||
| instrumentHttp(http, {}); | |||
There was a problem hiding this comment.
same default value comment here. Would be nice to accept undefined in the second arg and default to {}
lib/magic.js
Outdated
| // arguments are the same as to require.resolve. | ||
| function getPackageVersion(request, options) { | ||
| // treat `require("react-dom/server")` like `require("react-dom")` here | ||
| if (request.indexOf("/") !== -1) { |
There was a problem hiding this comment.
includes is supported back to node 6 (and node 4 is officially unsupported this month).
request.includes("/")
There was a problem hiding this comment.
sigh, I never remember this method. #old
… node version for builtins), and add meta.version to each event
…rument*. use includes instead of indexOf !== -1
toshok
force-pushed
the
toshok.npm-package-version
branch
from
d97de9b
to
536487e
Compare
….version' everywhere.
A couple of fixes:
packageVersiondown into the magic. Include that version in all events the magic sends. This will let us do nice things like COUNT/P99(durationMs) broken down by express version 🔥