[v12.x backport] events: allow monitoring error events #32004
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nodejs-github-bot
added
events
targos
added
the
semver-minor
BridgeAR
approved these changes
Mar 9, 2020
codebytere
force-pushed
the
v12.x-staging
branch
from
a01fb4a
to
e5dc2fb
Compare
codebytere
force-pushed
the
v12.x-staging
branch
2 times, most recently
from
63a03d2
to
d577190
Compare
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)
Flarna
force-pushed
the
v12-backport-eventmonitor
branch
from
11e93b0
to
a9356f4
Compare
Flarna
added
the
author ready
2 tasks
|
The CI links above are 404-ish. Is there any stale scavenger cleaning outdated CI results? 🤔 |
|
12.17.0 is planned mid/end of may therefore CI can wait at bit |
targos
pushed a commit
to targos/node
that referenced
this pull request
Apr 25, 2020
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>
|
thanks @Flarna. Landed on my release preparation branch: https://github.com/targos/node/commits/prepare-minor |
targos
pushed a commit
that referenced
this pull request
Apr 28, 2020
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>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Backport #30932 to v12 as it was reverted in 12.16.1 (see #30932 (comment)).
To avoid the issue seen in 12.16.0 I added the changes from #31848 on top.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes