"_unknown" uri passed for POSTs to signalfx
avolfson opened this issue · 7 comments
https://github.com/yaorg/node-measured/blob/master/packages/measured-node-metrics/lib/nodeHttpRequestMetrics.js#L32
req.route is always undefined here when I make a POST, so uri is sent as "_unknown". Docs don't mention this. Would be nice if they did, but a working middleware would be even better :)
Minimal reproducible example:
- start with https://yaorg.github.io/node-measured/packages/measured-signalfx-reporter/tutorial-SignalFx%20Express%20Full%20End%20to%20End%20Example.html
- s/app.get/app.post/g
- make a post call to '/hello'
4a) observe uri dimension is not passed to SFX
4b) observe status code is passed as 200 even if you change it to return something else
Fix:
req.url seems to be available, but since you're trying to avoid that, I would suggest setting a property on req, possibly triggered by an event like 'data' or 'end'
Speaking of which, it might be helpful to mention that the middleware needs to be added before any bodyParser middleware as 'data' and 'end' get swallowed during POSTs.
On further investigation, it looks like both the uri dimension and the status code issue are resolved by using res.on('finish', () => { instead of req.on('end', () => {
Happy to make a PR unless this is just some quirk in my setup.
Created an integration test to show the failure... but it actually works 😮
#59
The cause appears to be app.use(express.json()). If (and only if) I don't pass a Content-type express.json() is listening to, the dimensions come through properly.
I saw that the build for #58 is failing, I assume you are going to work through that and update your PRs to get this issue closed?
I have the fix for the failing build, but was holding off until I was able to produce an integration test that actually showed the issue. I now have, you can see it here: https://github.com/avolfson/node-measured/compare/master...avolfson:avolfson-patch-57-it?expand=1 & avolfson@31aeae8
This failing test requires use of express.json() as a middleware. @fieldju, do you consider this valid? If so, I'll add it to the PR with the fix & unit tests as well. Appreciate any other feedback as well.