Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid side effects by installing event listeners #225

Open
Flarna opened this issue Aug 27, 2019 · 7 comments
Open

Avoid side effects by installing event listeners #225

Flarna opened this issue Aug 27, 2019 · 7 comments
Labels
feature-request up-for-grabs Good for taking. Extra help will be provided by maintainers

Comments

@Flarna
Copy link
Member

Flarna commented Aug 27, 2019

followup on #161 (comment)

Is your feature request related to a problem? Please describe.
Use of OT should not result in side effects to user application by installing event listeners which change behavior.

Describe the solution you'd like
Installed listeners should take care to restore/execute the default behavior, e.g. in case of error listeners rethrowing the Error in case there are no other listeners.

Describe alternatives you've considered
Don't install listeners instead find other ways to gather this data, e.g. by wrapping emit(). But not sure if this is working in all cases and it may have a performance impact as emit is quite a hot function.

Additional context
Side effects I'm aware of (list is for sure not complete):

  • installing an error listener may eat up exceptions if user has not installed it's own listener
  • installing a response listener on a http,ClientRequest requires the listener to consume the data
  • installing signal handlers on process disables the default behavior (e.g. end process in case of SIGTERM)
@mayurkale22 mayurkale22 added the up-for-grabs Good for taking. Extra help will be provided by maintainers label Sep 13, 2019
@OlivierAlbertini
Copy link
Member

Don't install listeners instead find other ways to gather this data, e.g. by wrapping emit(). But not sure if this is working in all cases and it may have a performance impact as emit is quite a hot function.

Quickly make a test by wrapping the request.emit function (not all use cases were handled but it could be just slowest if I did)

Without wrapping the emit function

➜ ab -n 5000 -c 100 http://localhost:8080/helloworld
This is ApacheBench, Version 2.3 <$Revision: 1826891 $>
Copyright 1996 Adam Twiss, Zeus Technology Ltd, http://www.zeustech.net/
Licensed to The Apache Software Foundation, http://www.apache.org/

Benchmarking localhost (be patient)
Completed 500 requests
Completed 1000 requests
Completed 1500 requests
Completed 2000 requests
Completed 2500 requests
Completed 3000 requests
Completed 3500 requests
Completed 4000 requests
Completed 4500 requests
Completed 5000 requests
Finished 5000 requests


Server Software:
Server Hostname:        localhost
Server Port:            8080

Document Path:          /helloworld
Document Length:        12 bytes

Concurrency Level:      100
Time taken for tests:   1.933 seconds
Complete requests:      5000
Failed requests:        0
Total transferred:      435000 bytes
HTML transferred:       60000 bytes
Requests per second:    2586.31 [#/sec] (mean)
Time per request:       38.665 [ms] (mean)
Time per request:       0.387 [ms] (mean, across all concurrent requests)
Transfer rate:          219.74 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    2   4.5      1      56
Processing:     2   36   7.6     36      81
Waiting:        0   29   7.8     29      80
Total:          4   38   7.7     38      84

Percentage of the requests served within a certain time (ms)
  50%     38
  66%     40
  75%     41
  80%     43
  90%     46
  95%     48
  98%     56
  99%     71
 100%     84 (longest request)

Wrapping the emit function

ref: https://github.com/VilledeMontreal/opentelemetry-js/tree/feature/http-request-wrap-emit

➜ ab -n 5000 -c 100 http://localhost:8080/helloworld
This is ApacheBench, Version 2.3 <$Revision: 1826891 $>
Copyright 1996 Adam Twiss, Zeus Technology Ltd, http://www.zeustech.net/
Licensed to The Apache Software Foundation, http://www.apache.org/

Benchmarking localhost (be patient)
Completed 500 requests
Completed 1000 requests
Completed 1500 requests
Completed 2000 requests
Completed 2500 requests
Completed 3000 requests
Completed 3500 requests
Completed 4000 requests
Completed 4500 requests
Completed 5000 requests
Finished 5000 requests


Server Software:
Server Hostname:        localhost
Server Port:            8080

Document Path:          /helloworld
Document Length:        12 bytes

Concurrency Level:      100
Time taken for tests:   2.070 seconds
Complete requests:      5000
Failed requests:        0
Total transferred:      435000 bytes
HTML transferred:       60000 bytes
Requests per second:    2415.92 [#/sec] (mean)
Time per request:       41.392 [ms] (mean)
Time per request:       0.414 [ms] (mean, across all concurrent requests)
Transfer rate:          205.26 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    1   0.9      1       5
Processing:     1   40  10.3     38      98
Waiting:        1   32  10.3     31      97
Total:          5   41  10.3     40     100

Percentage of the requests served within a certain time (ms)
  50%     40
  66%     42
  75%     43
  80%     47
  90%     50
  95%     56
  98%     70
  99%     85
 100%    100 (longest request)

I ran this test multiple times and wrapping the emit function seems to be the slowest. We could make a better benchmark by using something better than ab...

Flarna added a commit to dynatrace-oss-contrib/node that referenced this issue Dec 13, 2019
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
BridgeAR pushed a commit to nodejs/node that referenced this issue Dec 20, 2019
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 pushed a commit to nodejs/node that referenced this issue Jan 3, 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

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>
targos pushed a commit to nodejs/node that referenced this issue Jan 14, 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

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>
BethGriggs pushed a commit to nodejs/node that referenced this issue Feb 6, 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

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>
Flarna added a commit to dynatrace-oss-contrib/node that referenced this issue Apr 6, 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

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>
targos pushed a commit to targos/node that referenced this issue 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>
targos pushed a commit to nodejs/node that referenced this issue 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>
@markandrus
Copy link

I just hit this recently when trying to add fastify-graceful-shutdown to my project. This plugin expects to be the one-and-only library to listen to SIGTERM, but opentelemetry has already registered.

@Flarna
Copy link
Member Author

Flarna commented Jul 19, 2022

There are other modules which assume they are alone regarding signal handling - e.g. signal-exit which has just about 34M downloads/week.
In short every module expecting to be the only one is mostly wrong.

For error event there is meanwhile a replacement but not yet in node.js 8, 10 and older 12 versions.

@legendecas
Copy link
Member

This plugin expects to be the one-and-only library to listen to SIGTERM, but opentelemetry has already registered.

I didn't find an occurrence in Otel packages that installed the SIGTERM handler, would you mind sharing what you found?

@dyladan
Copy link
Member

dyladan commented Jul 20, 2022

This plugin expects to be the one-and-only library to listen to SIGTERM, but opentelemetry has already registered.

I didn't find an occurrence in Otel packages that installed the SIGTERM handler, would you mind sharing what you found?

This is quite an old issue I believe we used to detect shutdown but no longer do.

@Flarna
Copy link
Member Author

Flarna commented Jul 21, 2022

Regarding signals: at least he readme for NodeSDK shows a usage of SIGTERM.

It's not only about shutdown. e.g. the HTTP instrumentation installs an error listener. If user application doesn't install one it would crash without OTel but error is ignored once OTel is present.

@dyladan
Copy link
Member

dyladan commented Jul 28, 2022

Regarding signals: at least he readme for NodeSDK shows a usage of SIGTERM.

The user can always do whatever they want to detect shutdowns. The readme is an example of what the user would do not what we do inside the SDK

It's not only about shutdown. e.g. the HTTP instrumentation installs an error listener. If user application doesn't install one it would crash without OTel but error is ignored once OTel is present.

In this case we should probably use the errorMonitor because we don't support the node versions which don't support it.

pichlermarc pushed a commit to dynatrace-oss-contrib/opentelemetry-js that referenced this issue Dec 15, 2023
* chore: gitignore

* chore: fixing context propagation for koa middleware layer
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request up-for-grabs Good for taking. Extra help will be provided by maintainers
Projects
None yet
Development

No branches or pull requests

6 participants