[v12.x backport] events: allow monitoring error events#32004
Closed
Flarna wants to merge 4 commits intonodejs:v12.x-stagingfrom
Closed
[v12.x backport] events: allow monitoring error events#32004Flarna wants to merge 4 commits intonodejs:v12.x-stagingfrom
Flarna wants to merge 4 commits intonodejs:v12.x-stagingfrom
Conversation
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
Collaborator
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
Collaborator
Collaborator
Collaborator
Flarna
added
the
author ready
2 tasks
Member
|
The CI links above are 404-ish. Is there any stale scavenger cleaning outdated CI results? 🤔 |
Member
Author
|
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>
Member
|
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>
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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