dont send traces to agent for routes on blocklist #1989
Conversation
| @@ -59,6 +60,16 @@ class HttpServerPlugin extends Plugin { | |||
|
|
|||
| web.wrapRes(context, context.req, context.res, context.res.end)() | |||
| }) | |||
|
|
|||
| this.addSub('datadog:tracer:trace:finish', ({ traces }) => { | |||
| if (!this.config.blocklist || traces.length === 0) return undefined | |||
There was a problem hiding this comment.
undefined is redundant as it's the default.
There was a problem hiding this comment.
There should also be an additional check that the root span is actually an HTTP request.
|
|
||
| const urlParts = url.parse(traces[0].meta['http.url']) | ||
|
|
||
| if (this.config.blocklist.includes(urlParts.pathname)) { |
| @@ -59,6 +60,16 @@ class HttpServerPlugin extends Plugin { | |||
|
|
|||
| web.wrapRes(context, context.req, context.res, context.res.end)() | |||
| }) | |||
|
|
|||
| this.addSub('datadog:tracer:trace:finish', ({ traces }) => { | |||
There was a problem hiding this comment.
traces should be trace or spans.
| it('should drop traces for blocklist route', done => { | ||
| const spy = sinon.spy(() => { | ||
| agent | ||
| .use((traces, spy) => { |
There was a problem hiding this comment.
This should be calling the spy, otherwise this test will always pass even if there is a request to the agent.
| }, 100) | ||
|
|
||
| axios.get(`http://localhost:${port}/health`).catch(done) | ||
| expect(spy).to.not.have.been.called |
There was a problem hiding this comment.
The expect should be in the timer otherwise this will always be true.
khanayan123
force-pushed
the
khanayan123/support-blocklist
branch
from
a22db64
to
4047fcb
Compare
| @@ -59,6 +60,20 @@ class HttpServerPlugin extends Plugin { | |||
|
|
|||
| web.wrapRes(context, context.req, context.res, context.res.end)() | |||
| }) | |||
|
|
|||
| this.addSub('datadog:tracer:trace:finish', ({ spans }) => { | |||
| if ((!this.config.blocklist && !this.config.allowlist) || | |||
There was a problem hiding this comment.
Let's use the existing helper to keep this code light.
| spans[0].name !== 'http.request') { | ||
| return | ||
| } | ||
| const urlParts = url.parse(spans[0].meta['http.url']) |
There was a problem hiding this comment.
blocklist array only contain the endpoints and not the full url, so I assume we would need to parse?
There was a problem hiding this comment.
OK, but let's use the URL constructor here.
khanayan123
force-pushed
the
khanayan123/support-blocklist
branch
from
4047fcb
to
8b2367e
Compare
| spans[0].name !== 'http.request') { | ||
| return | ||
| } | ||
| const urlParts = url.parse(spans[0].meta['http.url']) |
There was a problem hiding this comment.
OK, but let's use the URL constructor here.
| .use((traces) => { | ||
| spy() | ||
| }) | ||
| .then(done) |
There was a problem hiding this comment.
If a trace is received before 100ms, this test will pass. As an alternative here, try removing the then. The only success case here should be when the spy has not been called after 100ms.
khanayan123
force-pushed
the
khanayan123/support-blocklist
branch
from
8b2367e
to
8703bad
Compare
Co-authored-by: Ayan Khan <ayan.khan@datadoghq.com>
Co-authored-by: Ayan Khan <ayan.khan@datadoghq.com>
What does this PR do?
don't send traces to agent for routes on blocklist
Motivation
to fix a regression in pkg version 2.x.x where tracer would still export traces to agent for routers that were on blocklist
Plugin Checklist
Additional Notes