"_unknown" uri passed for POSTs to signalfx

[avolfson]

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: 1) start with https://yaorg.github.io/node-measured/packages/measured-signalfx-reporter/tutorial-SignalFx%20Express%20Full%20End%20to%20End%20Example.html 2) s/app.get/app.post/g 3) 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.

[avolfson]

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.

[fieldju]

I think You're not the first person to mention this can you please make a PR.

I will get it merged asap.

Thank you very much,

Justin

On Tue, Jan 29, 2019 at 7:23 PM avolfson notifications@github.com wrote:

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.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/yaorg/node-measured/issues/57#issuecomment-458797451, or mute the thread https://github.com/notifications/unsubscribe-auth/AArcLpUjB3LRiNOD9MxbNwtQeTRQe1oFks5vIRAcgaJpZM4aZVw0 .

-- Justin Field fieldju@gmail.com LinkedIn https://www.linkedin.com/in/fieldju | @fieldju

[avolfson]

Cool!

https://github.com/yaorg/node-measured/pull/58

Alex

---

From: Justin Field notifications@github.com Sent: Tuesday, January 29, 2019 10:41:43 PM To: yaorg/node-measured Cc: Alexander Volfson; Author Subject: Re: [yaorg/node-measured] "_unknown" uri passed for POSTs to signalfx (#57)

Note - This message originated from outside Care.com - Please use caution before opening attachments, clicking on links or sharing information.

I think You're not the first person to mention this can you please make a PR.

I will get it merged asap.

Thank you very much,

Justin

On Tue, Jan 29, 2019 at 7:23 PM avolfson notifications@github.com wrote:

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.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/yaorg/node-measured/issues/57#issuecomment-458797451, or mute the thread https://github.com/notifications/unsubscribe-auth/AArcLpUjB3LRiNOD9MxbNwtQeTRQe1oFks5vIRAcgaJpZM4aZVw0 .

-- Justin Field fieldju@gmail.com LinkedIn https://www.linkedin.com/in/fieldju | @fieldju

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://github.com/yaorg/node-measured/issues/57#issuecomment-458800582, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AYCyNDYtl9fvSKtG9Ctl9MhQfpYBsNLtks5vIRR3gaJpZM4aZVw0.

This email is intended for the person(s) to whom it is addressed and may contain information that is PRIVILEGED or CONFIDENTIAL. Any unauthorized use, distribution, copying, or disclosure by any person other than the addressee(s) is strictly prohibited. If you have received this email in error, please notify the sender immediately by return email and delete the message and any attachments from your system.

[avolfson]

Created an integration test to show the failure... but it actually works :open_mouth: https://github.com/yaorg/node-measured/pull/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.

[fieldju]

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?

[avolfson]

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 & https://github.com/avolfson/node-measured/commit/31aeae8f9fefc076efd3a39d6ca5a371bbe39308

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.

[fieldju]

Closed by https://github.com/yaorg/node-measured/pull/59