Support gzip compression for node exporter collector

[alisabzevari]

Fixes #1637,

I was not sure this is the right approach to address this issue. Please let me know if it is wrong.

Which problem is this PR solving?

• This PR provides an option to compress (using gzip) the request body in exporter collector for node.

Short description of the changes

• A new config named compress has been added to CollectorExporterNodeConfigBase. It is an optional boolean. The default behavior is to not compress the request body.
• The main change happened in util.ts which pipes the body through a gzip instance. The code is inspired from here.
• I have added only one test.
• I have changed the usage of sinon stub for the request in tests to use Passthrough stream instead. Using Passthrough we can work with a real stream (readable and writable) which provides more control on the tests.

TODO

• Content-Length header should be correct in case of compression enabled.
• Update the readme to reflect the new config.
• Compression is meant to be configurable through environment variables (e.g., OTEL_EXPORTER_OTLP_COMPRESSION) as well, according to spec.

[codecov]

Codecov Report

Merging #2337 (3863d48) into main (d8fbedd) will increase coverage by 0.29%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #2337      +/-   ##
==========================================
+ Coverage   92.35%   92.64%   +0.29%     
==========================================
  Files         128      142      +14     
  Lines        4249     5101     +852     
  Branches      868     1050     +182     
==========================================
+ Hits         3924     4726     +802     
- Misses        325      375      +50     

Impacted Files	Coverage Δ
...async-hooks/src/AsyncLocalStorageContextManager.ts
...sync-hooks/src/AbstractAsyncHooksContextManager.ts
...ontext-async-hooks/src/AsyncHooksContextManager.ts
packages/opentelemetry-web/src/types.ts	100.00% <0.00%> (ø)
packages/opentelemetry-web/src/utils.ts	94.90% <0.00%> (ø)
...kages/opentelemetry-exporter-collector/src/util.ts	100.00% <0.00%> (ø)
...mentation-xml-http-request/src/enums/EventNames.ts	100.00% <0.00%> (ø)
...ntelemetry-web/src/enums/PerformanceTimingNames.ts	100.00% <0.00%> (ø)
...-instrumentation-fetch/src/enums/AttributeNames.ts	100.00% <0.00%> (ø)
...ackages/opentelemetry-web/src/WebTracerProvider.ts	100.00% <0.00%> (ø)
... and 10 more

[MSNev]

General comment, why define the enum values as strings?
Why not just a numeric value as the "NAME" will still be available is the resulting code and if required we can always look it up in config via uppercasing ... Just a thought.

[vmarchaud]

Generally i advise using string to avoid problem when trying to update available value (that can share the same number even if values are different across versions)

[Flarna]

as far as I know normal enums are hard to use for non typescript users.

[dyladan]

@Flarna I know that's true for const enums but I'm not sure it's the case for regular enums. Do you have a particular case you're thinking of?

[dyladan]

I think I know what you mean after thinking a little more. You're not thinking they can't do CompressionAlgorithm.GZIP but that 1 doesn't mean anything to them and passing a string 'gzip' is more clear?

[Flarna]

Normal enums are exported as objects so a JS user can use CompressionAlgorithm.GZIP. A JS user can also use the underlying constant (e.g. gzip).
The main difference is documentation. A typescript user can't to much wrong as editor will tell the allowed values to use and compiler checks again.

For JS users it's needed to document that an object named CompressionAlgorithm is exported incl. the properties (GZIP and NONE) referring to the constants to use.

This works fine but often JS APIs document the actual values to use instead referring to an object holding the constants (would be gzip/none here).

Standard const enums are really bad as JS users have to use the numeric values then.
const enums with strings as values would require JS users to use these strings in their code.

[obecny]

I think we should not depend on zlib directly. We should create a config param and accepts a defined object with 2 function (zip, unzip) as interface. This can be compatible with zlib. This way people might be using any library with any version. ZLIB is quite popular people might have it already in their repo with different version etc. That's why I think we should define an interface that will allow more freedom to choose and will not create a strict dependency on 3rd party.

[alisabzevari]

zlib is a part of nodejs. It is not a 3rd party lib.

[obecny]

zlib is a part of nodejs. It is not a 3rd party lib.

Oh you are absolutely right, :)

yes

[obecny]

I thought the compression concerns should be hidden from the user in this function. What is the valid use-case for the user to set the Content-Encoding?

this is general approach, if user is able to pass a headers to the config, those headers should take precedence over other things, there can be edge cases which we are not aware of, but we should not prevent user from overriding things if for any reason wants to do that.

[dyladan]

I thought the compression concerns should be hidden from the user in this function. What is the valid use-case for the user to set the Content-Encoding?

this is general approach, if user is able to pass a headers to the config, those headers should take precedence over other things, there can be edge cases which we are not aware of, but we should not prevent user from overriding things if for any reason wants to do that.

Not sure I agree. Sure, the user can set headers, but that doesn't mean we can't override them. The content-length header is a good example of one that the user could technically pass to the headers object but we would be incorrect not to overwrite. Since the user can't affect the encoding, I'm not sure it makes sense to preserve the value they set for the content-encoding header.

[obecny]

I thought the compression concerns should be hidden from the user in this function. What is the valid use-case for the user to set the Content-Encoding?

this is general approach, if user is able to pass a headers to the config, those headers should take precedence over other things, there can be edge cases which we are not aware of, but we should not prevent user from overriding things if for any reason wants to do that.

Not sure I agree. Sure, the user can set headers, but that doesn't mean we can't override them. The content-length header is a good example of one that the user could technically pass to the headers object but we would be incorrect not to overwrite. Since the user can't affect the encoding, I'm not sure it makes sense to preserve the value they set for the content-encoding header.

well in linux if you want to run rm -fr / as a root, no1 will prevent you from doing that. The same here if you want to override it you know what you are doing and you should be able to do that. We are providing api and sdk that should be as flexible as it can be. If you are writing a final product you can take care of such things, making sure your product will not fail and you can decide to what extent you want to correct user behaviour. But here we should imho give users as much freedom as possible.

[Flarna]

I miss an update of the Readme to explain the new options.
This would be also a good place to explain that it may be a bad idea for the user to set e.g. a Content-Encoding or Content-Length header.

[vreynolds]

Might be a concern for another PR: compression is meant to be configurable through environment variables (e.g., OTEL_EXPORTER_OTLP_COMPRESSION) as well, according to spec

[alisabzevari]

I thought the compression concerns should be hidden from the user in this function. What is the valid use-case for the user to set the Content-Encoding?

this is general approach, if user is able to pass a headers to the config, those headers should take precedence over other things, there can be edge cases which we are not aware of, but we should not prevent user from overriding things if for any reason wants to do that.

Not sure I agree. Sure, the user can set headers, but that doesn't mean we can't override them. The content-length header is a good example of one that the user could technically pass to the headers object but we would be incorrect not to overwrite. Since the user can't affect the encoding, I'm not sure it makes sense to preserve the value they set for the content-encoding header.

well in linux if you want to run rm -fr / as a root, no1 will prevent you from doing that. The same here if you want to override it you know what you are doing and you should be able to do that. We are providing api and sdk that should be as flexible as it can be. If you are writing a final product you can take care of such things, making sure your product will not fail and you can decide to what extent you want to correct user behaviour. But here we should imho give users as much freedom as possible.

If there is an option to compress the body accepted by sendWithHttp, the function should be able to completely take the responsibility of handling the compression correctly. Otherwise, it will be a leaky abstraction.
I can suggest two alternatives here:

1. Remove the compression from sendWithHttp completely and move this responsibility to whichever class that has complete control over the http headers. Not recommended though, because there will be a lot of duplicate implementation in exporter classes.
2. sendWithHttp withdraws from interfereing with compression concerns if compression == CompressionAlgorithm.NONE. In this case, the class calling sendWithHttp should be able to set the headers and compress the body itself and sendWithHttp should not override the headers. Otherwise, sendWithHttp will override the needed headers in order to make sure compression works as promised.

[dyladan]

Retrigger CLA