fix: Ensure opts.headers is defined in request instrumenting #1926
Conversation
This fixes a bug where if `opts.headers` is `null`, which is not caught by further down functions and causes a crash.
|
|
| @@ -138,6 +138,8 @@ function instrumentRequest(agent, opts, makeRequest, hostname, segment) { | |||
| const transaction = segment.transaction | |||
| const outboundHeaders = Object.create(null) | |||
|
|
|||
| opts.headers = opts.headers || {}; | |||
There was a problem hiding this comment.
Can you please write a unit test for this?
Here's one that exhibits the issue.
t.test('should not crash when req.headers is null', (t) => {
const req = new events.EventEmitter()
helper.runInTransaction(agent, function (transaction) {
const path = '/asdf'
const name = NAMES.EXTERNAL.PREFIX + HOSTNAME + ':' + PORT + path
instrumentOutbound(agent, { headers: null, host: HOSTNAME, port: PORT }, makeFakeRequest)
t.equal(transaction.trace.root.children[0].name, name)
function makeFakeRequest() {
req.path = '/asdf?a=b&another=yourself&thing&grownup=true'
return req
}
})
t.end()
})You should put it in /test/unit/instrumentation/http/outbound.test.js
There was a problem hiding this comment.
Thanks for this! I had actually looked for a test to update, but was looking in /test/unit/instrumentation/core and didn't see one. I added the test and will @ you once I see things have passed.
There was a problem hiding this comment.
I tweaked it slightly to be more specific. I didn't mean for you to copy/paste but use for inspiration. Anyways, we appreciate the fix 🙏🏻
There was a problem hiding this comment.
Oh, sorry! Haha. Thanks for the tweak. I should have paid more attention to what you were proposing.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1926 +/- ##
=======================================
Coverage 96.98% 96.98%
=======================================
Files 212 212
Lines 40109 40111 +2
=======================================
+ Hits 38899 38901 +2
Misses 1210 1210
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
OuranosSkia
changed the title
Description
This fixes a bug where if
opts.headersisnull, it is not handled by further down functions and causes a crash.How to Test
Unfortunately, the situations in which this is reproduced are very specific to a flow that I have difficulty artificially reproducing. I am applying this fix as a technical code fix purely based on the way that it is crashing.
Related Issues
Fixes #1925