fix(restify): support an array of handlers #709
Conversation
If an array of handlers is given as an argument to `server.use` instead of just a single function, the agent would cause an exception to be raised. This commit changes the Restify instrumentation logic so that the handlers no longer needs to be wrapped, and so that the arguments to the `server.use` function no longer needs to be parsed by the agent. Instead the agent now patches a central (private) error handling function to be notified when ever an error is passed to the `next` function. The route name is instead gathered by the same logic that also gathers the route name for Express - from `req.route.path` - as Restify just so happens to use the same API for storing the route name on the request object as Express in this case. This way of getting the route name is actually more stable as the previous implementation didn't work if the request never made it to the actual route handler (e.g. if an error occurred in a middleware handler). In that case the transaction would have been named `use` (the function name used to register the middleware handler). Fixes: elastic#706 BREAKING CHANGE: Restify <5.2.0 is no longer supported
|
@Qard This is technically a breaking change as Restify 5.1.0, 5.0.1 and 5.0.0 is no longer supported if this is merged. I couldn't find an easy way to keep support for those version, though it should be possible if we feel it's necessary - it would just take a little more time and code. Previous lowest supported version: v5.0.0, released June 27th 2017 As you can see, going from v5.0.0 to v5.2.0 as the lowest supported version, shouldn't mean much as they were released very close to each other and users should be able to upgrade to a supported version in the 5.x range without any problems. So I don't think it's an issue changing this. Question is if this warrants a major bump or not, or if we should go back to the drawing board and come up with another fix for this bug. Update: I'm not even really sure if we actually supported v5.0.0 as it was marked as broken in the TAV tests. In that case the lowest version we previously supported was v5.0.1 which was released on the 18th of July 2017. |
| # - 6.0.1 | ||
| # - 6.0.0 | ||
| # - 5.0.0 | ||
| versions: '>=4.4 <5 || >5 <6 || >6.0.1 <6.2 || >6.2' |
There was a problem hiding this comment.
FYI: During research on this bug I also found that this version string does't produce the expected range:
semver.validRange('>=4.4 <5 || >5 <6 || >6.0.1 <6.2 || >6.2')
// '>=4.4.0 <5.0.0||>=6.0.0 <6.0.0||>6.0.1 <6.2.0||>=6.3.0'As you can see, >5 is not translated into >5.0.0 but into >=6.0.0. So our TAV tests have been ignoring a big part of the version range when testing.
There was a problem hiding this comment.
Hmm this might actually explain why I can't get the TAV tests to run now. They refuse to install v5.2.0 due to a weird bug. This might have been a problem all along and we just never knew because we didn't run the tests with 5.x
There was a problem hiding this comment.
Weird. I could have sworn it was testing 5.x properly. 🤔
There was a problem hiding this comment.
I found the bug: [email protected] had a (sub)dependency to npm@2 which then took over when tav tried to continue to test v6.0.1. That's why 6.2.0, 6.0.1, and 6.0.0 failed. I updated test-all-versions to not fail even if one of the modules it installs depends on npm. So now all Restify tests pass without us having to skip any tests.
|
We need to backport this to 1.x as well in which case it can't really be a breaking change anyway. So we have to decide if we can drop support for early versions of Restify 5.x and not consider it a breaking change, or if we need to find another solution that's fixing this bug without loosing support for early versions. Btw, though the docs say we currently support version 4.4+, this is misleading. The highest 4.x version is 4.3.4, so there is no such thing as 4.4. In reality this means that we currently support 5.0.0 and above. |
|
I think it's reasonable to consider it a bug fix. It never truly supported the versions being dropped anyway, due to the bug, and it's an older version too, so probably okay. 🤔 |
|
True, and you could also choose to see it this way: It's not a change that will break anything in running apps. It might collect less data, but the app will continue to run as normal. So in that sense it's not a breaking change. |
|
@Qard now all tests should be fixed. Could I get you to do another review? |
If an array of handlers is given as an argument to `server.use` instead of just a single function, the agent would cause an exception to be raised. This commit changes the Restify instrumentation logic so that the handlers no longer needs to be wrapped, and so that the arguments to the `server.use` function no longer needs to be parsed by the agent. Instead the agent now patches a central (private) error handling function to be notified when ever an error is passed to the `next` function. The route name is instead gathered by the same logic that also gathers the route name for Express - from `req.route.path` - as Restify just so happens to use the same API for storing the route name on the request object as Express in this case. This way of getting the route name is actually more stable as the previous implementation didn't work if the request never made it to the actual route handler (e.g. if an error occurred in a middleware handler). In that case the transaction would have been named `use` (the function name used to register the middleware handler). Fixes: elastic#706
If an array of handlers is given as an argument to `server.use` instead of just a single function, the agent would cause an exception to be raised. This commit changes the Restify instrumentation logic so that the handlers no longer needs to be wrapped, and so that the arguments to the `server.use` function no longer needs to be parsed by the agent. Instead the agent now patches a central (private) error handling function to be notified when ever an error is passed to the `next` function. The route name is instead gathered by the same logic that also gathers the route name for Express - from `req.route.path` - as Restify just so happens to use the same API for storing the route name on the request object as Express in this case. This way of getting the route name is actually more stable as the previous implementation didn't work if the request never made it to the actual route handler (e.g. if an error occurred in a middleware handler). In that case the transaction would have been named `use` (the function name used to register the middleware handler).
If an array of handlers is given as an argument to `server.use` instead of just a single function, the agent would cause an exception to be raised. This commit changes the Restify instrumentation logic so that the handlers no longer needs to be wrapped, and so that the arguments to the `server.use` function no longer needs to be parsed by the agent. Instead the agent now patches a central (private) error handling function to be notified when ever an error is passed to the `next` function. The route name is instead gathered by the same logic that also gathers the route name for Express - from `req.route.path` - as Restify just so happens to use the same API for storing the route name on the request object as Express in this case. This way of getting the route name is actually more stable as the previous implementation didn't work if the request never made it to the actual route handler (e.g. if an error occurred in a middleware handler). In that case the transaction would have been named `use` (the function name used to register the middleware handler).
2 participants
If an array of handlers is given as an argument to
server.useinstead of just a single function, the agent would cause an exception to be raised.This commit changes the Restify instrumentation logic so that the handlers no longer needs to be wrapped, and so that the arguments to the
server.usefunction no longer needs to be parsed by the agent.Instead the agent now patches a central (private) error handling function to be notified when ever an error is passed to the
nextfunction.The route name is instead gathered by the same logic that also gathers the route name for Express - from
req.route.path- as Restify just so happens to use the same API for storing the route name on the request object as Express in this case.This way of getting the route name is actually more stable as the previous implementation didn't work if the request never made it to the actual route handler (e.g. if an error occurred in a middleware handler). In that case the transaction would have been named
use(the function name used to register the middleware handler).Fixes: #706
BREAKING CHANGE: Restify <5.2.0 is no longer supported