elastic / elastic/apm-agent-nodejs

`apm.middleware.connect()` and 'finalhandler' instrumentation result in double-reporting errors

Open
#2,437 1 comment 0 reactions 0 assignees View on GitHub
agent-nodejs
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.