events: allow monitoring error events #30932

Flarna · 2019-12-13T10:06:11Z

Installing an error listener has a side effect that emitted errors are considered as handled. This is quite bad for monitoring/logging tools which tend to be interested in errors but don't want to cause side
effects like swallow an exception.

There are some workarounds in the wild like monkey patching emit or remit the error if monitoring tool detects that it is the only listener but this is error prone and risky.

This PR allows to install a listener to monitor errors with the side effect to consume the error. To avoid conflicts with other events it exports a symbol on EventEmitter which owns this special meaning.

Refs: open-telemetry/opentelemetry-js#225

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Installing an error listener has a side effect that emitted errors are considered as handled. This is quite bad for monitoring/logging tools which tend to be interested in errors but don't want to cause side effects like swallow an exception. There are some workarounds in the wild like monkey patching emit or remit the error if monitoring tool detects that it is the only listener but this is error prone and risky. This PR allows to install a listener to monitor errors with the side effect to consume the error. To avoid conflicts with other events it exports a symbol on EventEmitter which owns this special meaning. Refs: open-telemetry/opentelemetry-js#225

lib/events.js

doc/api/events.md

BridgeAR

I very much like this addition.

doc/api/events.md

test/parallel/test-event-emitter-error-monitor.js

nodejs-github-bot · 2019-12-15T15:12:00Z

CI: https://ci.nodejs.org/job/node-test-pull-request/27714/

Trott · 2019-12-18T01:43:16Z

Non-blocking comment, but I wouldn't mind this (and most new features) going in initially as experimental status so we can roll it back or change it in the first few releases it shows up in.

Flarna · 2019-12-18T10:17:34Z

I'm fine with adding an experimental statement similar the "Capture Rejections" recently added.
Is it just about adding > Stability: 1 - errorMonitor experimental. in events.md or is there more to do?
How does this effect back-porting to e.g. nodejs 12?

I'm not involved enough in the node project that I feel that well on deciding this. Would be nice if some others could give hints if we should move to experimental nor not.

jasnell · 2019-12-18T13:56:17Z

I really disagree with the experimental bit. For larger and more complicated changes it makes sense but we really don't need every API or feature addition to be experimental. This one is a simple, straightforward semver-minor. I think that's enough.

Installing an error listener has a side effect that emitted errors are considered as handled. This is quite bad for monitoring/logging tools which tend to be interested in errors but don't want to cause side effects like swallow an exception. There are some workarounds in the wild like monkey patching emit or remit the error if monitoring tool detects that it is the only listener but this is error prone and risky. This PR allows to install a listener to monitor errors with the side effect to consume the error. To avoid conflicts with other events it exports a symbol on EventEmitter which owns this special meaning. Refs: open-telemetry/opentelemetry-js#225 PR-URL: #30932 Refs: open-telemetry/opentelemetry-js#225 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

BridgeAR · 2019-12-20T02:03:28Z

Landed in fcd4c2e 🎉

I landed this since the comment was not blocking.

Installing an error listener has a side effect that emitted errors are considered as handled. This is quite bad for monitoring/logging tools which tend to be interested in errors but don't want to cause side effects like swallow an exception. There are some workarounds in the wild like monkey patching emit or remit the error if monitoring tool detects that it is the only listener but this is error prone and risky. This PR allows to install a listener to monitor errors with the side effect to consume the error. To avoid conflicts with other events it exports a symbol on EventEmitter which owns this special meaning. Refs: open-telemetry/opentelemetry-js#225 PR-URL: #30932 Refs: open-telemetry/opentelemetry-js#225 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Notable changes: * assert: * Implement `assert.match()` and `assert.doesNotMatch()` (Ruben Bridgewater) #30929 * events: * Add `EventEmitter.on` to async iterate over events (Matteo Collina) #27994 * Allow monitoring error events (Gerhard Stoebich) #30932 * fs: * Allow overriding `fs` for streams (Robert Nagy) #29083 * perf_hooks: * Move `perf_hooks` out of experimental (legendecas) #31101 * repl: * Implement ZSH-like reverse-i-search (Ruben Bridgewater) #31006 * tls: * Add PSK (pre-shared key) support (Denys Otrishko) #23188 PR-URL: #31238

Convert property errorMonitor to a normal property as non-writable caused unwanted side effects. Refs: #30932 (comment) PR-URL: #31848 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matheus Marchini <mat@mmarchini.me> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Installing an error listener has a side effect that emitted errors are considered as handled. This is quite bad for monitoring/logging tools which tend to be interested in errors but don't want to cause side effects like swallow an exception. There are some workarounds in the wild like monkey patching emit or remit the error if monitoring tool detects that it is the only listener but this is error prone and risky. This PR allows to install a listener to monitor errors with the side effect to consume the error. To avoid conflicts with other events it exports a symbol on EventEmitter which owns this special meaning. Refs: open-telemetry/opentelemetry-js#225 PR-URL: nodejs#30932 Refs: open-telemetry/opentelemetry-js#225 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Convert property errorMonitor to a normal property as non-writable caused unwanted side effects. Refs: nodejs#30932 (comment)

Installing an uncaughtException listener has a side effect that process is not aborted. This is quite bad for monitoring/logging tools which tend to be interested in errors but don't want to cause side effects like swallow an exception or change the output on console. There are some workarounds in the wild like monkey patching emit or rethrow in the exception if monitoring tool detects that it is the only listener but this is error prone and risky. This PR allows to install a listener to monitor uncaughtException without the side effect to consider the exception has handled. PR-URL: nodejs#31257 Refs: nodejs#30932 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

Installing an error listener has a side effect that emitted errors are considered as handled. This is quite bad for monitoring/logging tools which tend to be interested in errors but don't want to cause side effects like swallow an exception. There are some workarounds in the wild like monkey patching emit or remit the error if monitoring tool detects that it is the only listener but this is error prone and risky. This PR allows to install a listener to monitor errors with the side effect to consume the error. To avoid conflicts with other events it exports a symbol on EventEmitter which owns this special meaning. Refs: open-telemetry/opentelemetry-js#225 Backport-PR-URL: nodejs#32004 PR-URL: nodejs#30932 Refs: open-telemetry/opentelemetry-js#225 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Convert property errorMonitor to a normal property as non-writable caused unwanted side effects. Refs: nodejs#30932 (comment) PR-URL: nodejs#31848 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matheus Marchini <mat@mmarchini.me> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Installing an uncaughtException listener has a side effect that process is not aborted. This is quite bad for monitoring/logging tools which tend to be interested in errors but don't want to cause side effects like swallow an exception or change the output on console. There are some workarounds in the wild like monkey patching emit or rethrow in the exception if monitoring tool detects that it is the only listener but this is error prone and risky. This PR allows to install a listener to monitor uncaughtException without the side effect to consider the exception has handled. PR-URL: #31257 Refs: #30932 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

Installing an error listener has a side effect that emitted errors are considered as handled. This is quite bad for monitoring/logging tools which tend to be interested in errors but don't want to cause side effects like swallow an exception. There are some workarounds in the wild like monkey patching emit or remit the error if monitoring tool detects that it is the only listener but this is error prone and risky. This PR allows to install a listener to monitor errors with the side effect to consume the error. To avoid conflicts with other events it exports a symbol on EventEmitter which owns this special meaning. Refs: open-telemetry/opentelemetry-js#225 Backport-PR-URL: #32004 PR-URL: #30932 Refs: open-telemetry/opentelemetry-js#225 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Convert property errorMonitor to a normal property as non-writable caused unwanted side effects. Refs: #30932 (comment) PR-URL: #31848 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matheus Marchini <mat@mmarchini.me> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

The length property should be non enumerable to match behavior of normal functions. The asyncResource property is enumerable and therefore it should be also writable to avoid issues like there: nodejs#30932 (comment) Both properties should be configurable. Refs: nodejs#34574

The length property should be non enumerable to match behavior of normal functions. The asyncResource property is enumerable and therefore it should be also writable to avoid issues like there: #30932 (comment) Both properties should be configurable. Refs: #34574 PR-URL: #34620 Reviewed-By: Andrey Pechkurov <apechkurov@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

nodejs-github-bot added the events Issues and PRs related to the events subsystem / EventEmitter. label Dec 13, 2019

Flarna force-pushed the monitor-error-events branch from 6c53686 to 0a773db Compare December 13, 2019 10:53

addaleax reviewed Dec 13, 2019

View reviewed changes

lib/events.js Outdated Show resolved Hide resolved

mscdex reviewed Dec 13, 2019

View reviewed changes

doc/api/events.md Outdated Show resolved Hide resolved

mscdex reviewed Dec 13, 2019

View reviewed changes

doc/api/events.md Outdated Show resolved Hide resolved

mscdex reviewed Dec 13, 2019

View reviewed changes

doc/api/events.md Outdated Show resolved Hide resolved

BridgeAR approved these changes Dec 13, 2019

View reviewed changes

doc/api/events.md Outdated Show resolved Hide resolved

test/parallel/test-event-emitter-error-monitor.js Outdated Show resolved Hide resolved

Flarna added 2 commits December 13, 2019 16:58

rename symbol

d156268

fix review findings

793205c

lpinca approved these changes Dec 13, 2019

View reviewed changes

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 15, 2019

targos added the semver-minor PRs that contain new features and should be released in the next minor version. label Dec 16, 2019

Trott approved these changes Dec 18, 2019

View reviewed changes

jasnell approved these changes Dec 18, 2019

View reviewed changes

BridgeAR closed this Dec 20, 2019

Flarna deleted the monitor-error-events branch December 20, 2019 06:51

BridgeAR mentioned this pull request Jan 7, 2020

v13.6.0 proposal #31238

Merged

Flarna mentioned this pull request Jan 8, 2020

process: allow monitoring uncaughtException #31257

Closed

4 tasks

Flarna mentioned this pull request Feb 28, 2020

[v12.x backport] events: allow monitoring error events #32004

Closed

4 tasks

Flarna added a commit to dynatrace-oss-contrib/node that referenced this pull request Apr 6, 2020

events: convert errorMonitor to a normal property

c483e6d

Convert property errorMonitor to a normal property as non-writable caused unwanted side effects. Refs: nodejs#30932 (comment)

targos added backported-to-v12.x and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. backport-requested-v12.x labels Apr 25, 2020

targos mentioned this pull request May 2, 2020

v12.17.0 release proposal #33197

Merged

Flarna mentioned this pull request Aug 4, 2020

async_hooks: improve property descriptors in als.bind #34620

Closed

Flarna mentioned this pull request Feb 4, 2021

Convert span to ReadableSpan before sending to span processor open-telemetry/opentelemetry-js#1902

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

events: allow monitoring error events #30932

events: allow monitoring error events #30932

Flarna commented Dec 13, 2019 •

edited

Loading

BridgeAR left a comment

nodejs-github-bot commented Dec 15, 2019

Trott commented Dec 18, 2019 •

edited

Loading

Flarna commented Dec 18, 2019

jasnell commented Dec 18, 2019

BridgeAR commented Dec 20, 2019

events: allow monitoring error events #30932

events: allow monitoring error events #30932

Conversation

Flarna commented Dec 13, 2019 • edited Loading

Checklist

BridgeAR left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Dec 15, 2019

Trott commented Dec 18, 2019 • edited Loading

Flarna commented Dec 18, 2019

jasnell commented Dec 18, 2019

BridgeAR commented Dec 20, 2019

Flarna commented Dec 13, 2019 •

edited

Loading

Trott commented Dec 18, 2019 •

edited

Loading