feat(express): auto-track errors #127
Conversation
Codecov Report
@@ Coverage Diff @@
## master #127 +/- ##
==========================================
+ Coverage 82.55% 82.86% +0.31%
==========================================
Files 38 38
Lines 1920 1938 +18
==========================================
+ Hits 1585 1606 +21
+ Misses 335 332 -3
Continue to review full report at Codecov.
|
| shimmer.wrap(layer.constructor.prototype, 'handle_error', function (orig) { | ||
| return function (err, req) { | ||
| if (!err[reportedSymbol]) { | ||
| err[reportedSymbol] = true |
There was a problem hiding this comment.
I really like this approach. I can think of a few places in our code base where we should do that instead 😅
| @@ -12,12 +12,45 @@ module.exports = function (express, agent, version) { | |||
| return express | |||
| } | |||
|
|
|||
| // express 5 moves the router methods onto a prototype | |||
There was a problem hiding this comment.
Nice that you're thinking of Express 5 already 👍
There was a problem hiding this comment.
I haven't fully tested express 5, but that was an easy part to fix.
There was a problem hiding this comment.
Unless you see a reason for keeping middleware.express around, I suggest that we remove it from the source code and from the docs. What do you think?
I've found it referenced in these sections in the docs:
- Remove it from code snippet here: https://github.com/elastic/apm-agent-nodejs/blob/master/docs/express.asciidoc#initialization
- If I'm not mistaken we can now replace the entirety of the Express error logging section with a copy of the same section from hapi
- The section about
middleware.connectshould be cleaned up as well
Missing tests:
- We should probably also add a test for what happens if a 404 is returned. Normally in Express this is treated as an error, which we don't want to catch in the error logger (see: https://github.com/elastic/apm-agent-nodejs/blob/master/docs/express.asciidoc#ignore-404-not-found-errors)
- Optional: Would it make sense to make a test for the case where the developer doesn't add any error handling middleware?
|
We should probably also add the new Express test file file to .tav.yml 😃 |
|
Since it's in end-user code, would it be better to have a deprecation warning for a bit? |
|
Normally I would deprecate yes, but as we're still in beta and since we're making a few other breaking changes when we go GA, I think it's ok for us to make this one breaking as well. The more we can clean up now and not carry around as legacy for a while the cleaner/better I think. We'll have to make an upgrade guide for both old Opbeat users and for beta users when we go into GA anyway, so we can have a section mentioning breaking changes like this here. That's at least my current philosophy 😄 |
Qard
force-pushed
the
express-errors
branch
2 times, most recently
from
c5c45be
to
cca845b
Compare
|
Made those changes and also, when adding the TAV details discovered the approach I took didn't work before express 4.6 so I made some changes to how the patch gets applied. It's not using the |
|
@Qard if you feel it's better I think we should consider just dropping support for Express <4.6 as that version was released in June 2014. What do you think? |
docs/agent-api.asciidoc
Outdated
| Returns a middleware function used to collect and send errors to the APM Server. | ||
|
|
||
| The middleware can be used as-is with either Connect or Express in the same way. | ||
| The middleware can be used as-is with either Connect in the same way. |
There was a problem hiding this comment.
I'd just remove the entire line here
|
@Qard As you also mentioned in #131 (comment), we forgot to add the new tav test to the Before merging we need to figure out if we want to publish a new version of the module right after (as it's changing the docs), or if there's a better way to handle this. See #134 for more details. Tip: I just tested it locally on Node 9 by having ... and it all worked perfectly 👍 |
This enables automatic tracking of express errors, rather than requiring manual use of the middleware.
|
Weird... I tried to resolve the conflicts using the "Resolve conflicts" button, but even though the commit that resolves them are included in the PR, GitHub still thinks the conflicts exists. |
This enables automatic tracking of express errors, rather than requiring manual use of the middleware.
Fixes #84