Fix gzip compression for OTLP HTTP exporter

[mattolson]

Which problem is this PR solving?

Fixes #3040

Currently if you configure your OTLP HTTP exporter to use gzip compression, it will succeed on the first request and fail on subsequent requests. This PR makes it more reliable.

Short description of the changes

Instead of sharing the same gzip stream across multiple requests, create a new one for each request. This bug was introduced in #2581

Type of change

Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

I have tested this manually in my application by running several requests through the OTLP exporter to a local lightstep dev collector.

In addition, I wrote a failing unit test and then show that the code change fixes it.

Without the code change, the newly written unit test fails with the following:

  sendWithHttp
    ✓ should send with no compression if configured to do so
    ✓ should send with gzip compression if configured to do so
    ✓ should work with gzip compression enabled even after multiple requests
(node:76508) MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 error listeners added to [Gzip]. Use emitter.setMaxListeners() to increase limit
(Use `node --trace-warnings ...` to show where the warning was created)
    1) should work with gzip compression enabled even after multiple requests
    2) "after each" hook in "sendWithHttp"


  37 passing (82ms)
  2 failing

  1) sendWithHttp
       should work with gzip compression enabled even after multiple requests:
 + .g/COMMIT_EDITMSG

      Uncaught AssertionError [ERR_ASSERTION]: false == true
      + expected - actual

      -false
      +true

      at HttpRequest.<anonymous> (test/node/util.test.ts:279:9)
      at endReadableNT (internal/streams/readable.js:1334:12)
      at processTicksAndRejections (internal/process/task_queues.js:82:21)

  2) sendWithHttp
       "after each" hook in "sendWithHttp":
     Error: done() called multiple times in hook <sendWithHttp "after each" hook> of file /Users/matt.olson/workspace/opentelemetry-js/experimental/packages/otlp-exporter-base/test/node/util.test.ts; in addition, done() received error: AssertionError [ERR_ASSERTION]: false == true
      at processEmit (/Users/matt.olson/workspace/opentelemetry-js/node_modules/signal-exit/index.js:199:34)
      at process.emit (/Users/matt.olson/workspace/opentelemetry-js/node_modules/source-map-support/source-map-support.js:516:21)
      at process._fatalException (internal/process/execution.js:167:25)

Checklist:

• Followed the style guidelines of this project
• Unit tests have been added
• Documentation has been updated

[mattolson]

@dyladan Can you take a look or forward to the appropriate person for review?

[dyladan]

I understand the changes, I'm just trying to understand why it was introduced in the first place. Looks like it was a performance optimization to avoid creating a new gzip for each request, but since requests don't happen often (by computer standards) I think it was an unnecessary micro-optimization.

[codecov]

Codecov Report

Merging #3046 (5cf2a10) into main (d2de661) will increase coverage by 0.46%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #3046      +/-   ##
==========================================
+ Coverage   92.64%   93.10%   +0.46%     
==========================================
  Files         187      188       +1     
  Lines        6198     6224      +26     
  Branches     1314     1316       +2     
==========================================
+ Hits         5742     5795      +53     
+ Misses        456      429      -27     

Impacted Files	Coverage Δ
...ter-base/src/platform/node/OTLPExporterNodeBase.ts	50.00% <ø> (ø)
...kages/otlp-exporter-base/src/platform/node/util.ts	75.34% <ø> (+54.00%)	⬆️

[dyladan]

Please fix linting errors and add your fix to the changelog at experimental/CHANGELOG.md

[mattolson]

@dyladan Ok, should be good to go. And yes, I think this was a case of an unnecessary micro-optimization in the original PR.

[dyladan]

I've been looking into this a little more and came across this https://nodejs.org/api/zlib.html#threadpool-usage-and-performance-considerations

I think this may be why the original author was avoiding creating many gzip objects which can affect not just the performance of the gzip but the whole application as it reduces the available thread pool.

But in the examples on that page a new gzip object is created for each request so I think this is probably fine.

[mattolson]

But in the examples on that page a new gzip object is created for each request so I think this is probably fine.

I agree - it would be one thing if we were creating these gzip streams rapidly and concurrently, but in this situation, we only do it when sending http requests to the collector, which happen at most every scheduledDelayMillis ms (default 5000), if using default BatchSpanProcessor.

[pichlermarc]

Looks good! Thank you for taking the time to fix this. 🙂