elastic / elastic/apm-agent-nodejs
`apm.middleware.connect()` and 'finalhandler' instrumentation result in double-reporting errors
- Dominant language
- JavaScript
- Stars
- 594
- Forks
- 244
- Avg merge
- 1d 8h
- Merged PRs (30d)
- 16
Description
Way back in the early days of this APM agent (about 10y ago) `.middleware.connect()` was added to support `apm.captureError()`'ing with Connect middleware:
```
var app = connect()
// app.use(...)
app.use(apm.middleware.connect()) // installs error middleware to `captureError()`
```
About three years ago in #629 support for [finalhandler](https://www.npmjs.com/package/finalhandler) was added to better handle error reporting for Express usage. Express uses finalhandler. At the time an `errorReportedSymbol` symbol was added to the reported errors to coordinate between the 'express' and 'finalhandler' instrumentation to avoid double-reporting the errors.
What was missed is that Connect also uses finalhandler. The `errorReportedSymbol`-based coordination is not being done in "lib/middleware/connect.js". So if one uses `apm.middleware.connect()`, then server response errors will be double-reported.
I see no current value in `apm.middleware.connect()`. I think we should:
1. Deprecate it and drop it in the next major
2. Document that it is automatically handled by 'finalhandler' middleware.
3. Fix 'finalhandler' middleware to be disablable via `disableInstrumentations`. Currently it is not.
Low priority: This has been the state for 3y and there are no issues about it. Connect and finalhandler have not had releases for 2y+ I think.
Contributor guide
Assessment
This issue has not been assessed yet.