adapter: work-around missing res.response #216
Conversation
| @@ -377,7 +377,7 @@ function createMessageListener() { | |||
| function createProbeListener(probeName) { | |||
| monitor.on(probeName, function(res) { | |||
| var duration = 0; | |||
| if (probeName === 'express') { | |||
| if (probeName === 'express' && res.response) { | |||
There was a problem hiding this comment.
probeName isn't used to make any other decisions in this function, except for creating a probeMetrics entry for the name if it's not already there. Do we need to examine the probeName anymore? If the probeName isn't express, but the duration is stored on res.response.duration, this will miss it.
There was a problem hiding this comment.
the intention of the code is that there is only a res.response.duration for express probes, I've no reason to think any other probes have one, and I've no idea why the express probe doesn't always have one. This PR is a work-around, we can figure out what is really broken next week - but it doesn't effect apiconnect, which doesn't depend on this metric anyway.
There was a problem hiding this comment.
There was a problem hiding this comment.
so the res.response is only relevant to code that was deleted
|
LGTM |
|
@slnode test please |
1 similar comment
|
@slnode test please |
| @@ -377,7 +377,7 @@ function createMessageListener() { | |||
| function createProbeListener(probeName) { | |||
| monitor.on(probeName, function(res) { | |||
There was a problem hiding this comment.
I think the argument name is wrong here, this is the data object from the express even that had docs removed in https://github.com/RuntimeTools/appmetrics/pull/260/files, right? res makes it seem like it is a https://nodejs.org/docs/latest/api/http.html#http_class_http_serverresponse
But the express probe got removed, and replace with strong-express-metrics... and now we are hitting this, which means that there is still an express event, what is the structure of this event?
fix #213