fix: do not trace APM server client requests #161
Conversation
Use uninstrumented https.request() et al (grabbing references before the agent does shimming) for requests to APM server. This avoids them interfering in tracing (adding trace-context headers to APM server requests, emitting debug logs about spans). This will fix elastic/apm-agent-nodejs#1995
github-actions
bot
added
the
agent-nodejs
|
before: after: Note these two log debug lines are now gone: using this example script: var apm = require('./').start({ // elastic-apm-node
captureExceptions: false,
logUncaughtExceptions: true,
captureSpanStackTraces: false,
stackTraceLimit: 3,
apiRequestTime: 3,
metricsInterval: 0,
cloudProvider: 'none',
// ^^ Boilerplate config above this line is to focus on just tracing.
serviceName: 'example-do-not-trace-http-client',
centralConfig: false, // TODO test central config is also not traced
// asyncHooks: false, // TODO test with and without asyncHooks
logLevel: 'debug'
})
// Explicitly require the possible APM server client transport modules so that
// the agent instruments them.
var http = require('http')
var https = require('https')
var t1 = apm.startTransaction('t1')
setImmediate(function () {
var s2 = apm.startSpan('s2')
setImmediate(function () {
s2.end()
t1.end()
})
}) |
|
If this sounds reasonable, then I'll test a bit more (with/without Then I'll follow up with an agent change that uses this new http-client version and drops the special-case handling that attempts to hide the instrumentation of APM server requests: diff --git a/lib/instrumentation/http-shared.js b/lib/instrumentation/http-shared.js
index 6b97de97..6bea384d 100644
--- a/lib/instrumentation/http-shared.js
+++ b/lib/instrumentation/http-shared.js
@@ -169,13 +169,8 @@ exports.traceOutgoingRequest = function (agent, moduleName, method) {
var req = orig.apply(this, newArgs)
if (!span) return req
- if (getSafeHost(req) === agent._conf.serverHost) {
- agent.logger.debug('ignore %s request to intake API %o', moduleName, { id: id })
- return req
- } else {
- var protocol = req.agent && req.agent.protocol
- agent.logger.debug('request details: %o', { protocol: protocol, host: getSafeHost(req), id: id })
- }
+ var protocol = req.agent && req.agent.protocol
+ agent.logger.debug('request details: %o', { protocol: protocol, host: getSafeHost(req), id: id })
ins.bindEmitter(req) |
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
Trends 🧪 |
|
Oh, also relevant from those before and after examples are the headers sent to APM server. before: Note that trace-context headers being unintentionally sent. after: |
There was a problem hiding this comment.
Overall looks like a reasonable approach. 👍 Stream of conscious notes/review follows
I see that we're stashing the original values of of get and request from the http and https in module-global variables and then using these for our requests to APM Server.
Not an issue, but worth asking/noting: This works because the agent loads the client module (var ElasticAPMHttpClient = require('elastic-apm-http-client')) before it does its monkey patching, right?
The elastic-apm-http-client also references the Agent transport --
this._agent = new this._transport.Agent(agentOpts)
but I don't think we need to change anything here.
Yes, exactly. That require statement happens when the agent itself is
Yup. Currently http.Agent isn't wrapped for anything. Theoretically a future agent change could change how the http/https modules are instrumented such that this could break... but I'll try to have a test case in the agent that would catch a span for the agent comms getting erroneously included. |
|
@astorm I'm re-requesting review. I'll want a release for the agent immediately, so I'm bumping the version. I don't know what meaningful testing I can do from the http-client side, so I haven't added any. I could use Object.getOwnPropertySymbols on that are indicators that it is wrapped. However, those symbol names are not published names so it would be somewhat of a hack to bother checking. |
|
[email protected] published, v9.9.0 tagged |
Use uninstrumented https.request() et al (grabbing references before the
agent does shimming) for requests to APM server. This avoids them
interfering in tracing (adding trace-context headers to APM server
requests, emitting debug logs about spans). This will fix
elastic/apm-agent-nodejs#1995