Avoid side effects by installing event listeners

[Flarna]

trafficstars

followup on https://github.com/open-telemetry/opentelemetry-js/pull/161#discussion_r317842380

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)

[OlivierAlbertini]

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...

[markandrus]

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]

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]

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]

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]

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]

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.