Make gzip default for otlp gRPC/HTTP exporters

[jack-berg]

Description:
Change OTLP gRPC exporter to use gzip compression by default. If this is merged, I can followup with the OTLP HTTP exporter to use gzip by default as well.

Link to tracking Issue:
#4587

Testing:

• Added benchmarks comparing different compression algorithms.
• Updated configgrpc_test.go to validate default behavior

Documentation:
Added details to /config/configgrpc/README.md with benchmark results and notes on compression.

[bogdandrutu]

Before debating the change of default, I want to say huge thank you for this work. I would be extremely happy if you can split this into adding the benchmark and the results into a separate PR since that is a no brainer to merge.

[jack-berg]

I've split out the benchmarks to #4640. This PR is now only related to changing the default compression algo to gzip.

[codecov]

Codecov Report

Merging #4632 (d224686) into main (5b572f4) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #4632   +/-   ##
=======================================
  Coverage   90.70%   90.70%           
=======================================
  Files         179      179           
  Lines       10562    10566    +4     
=======================================
+ Hits         9580     9584    +4     
  Misses        763      763           
  Partials      219      219           

Impacted Files	Coverage Δ
exporter/otlpexporter/factory.go	91.54% <100.00%> (+0.24%)	⬆️
exporter/otlphttpexporter/factory.go	87.62% <100.00%> (+0.26%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5b572f4...d224686. Read the comment docs.

[bogdandrutu]

@tigrannajaryan @jpkrohling @codeboten please let me know if you are ok, since this is kind of a very subtle but major change

[tigrannajaryan]

I agree with the spirit of the change. Data likely goes to non-localhost from the Collector, so our previous reasoning that localhost doesn't need compression and non-localhost needs applies here.

I have one concern: are we certain all existing receivers are fine with receiving gzip-ed payload? Was support of "gzip" always part of our OTLP receiver specification? We don't want to break anyone who was following the OTLP spec in their implementation.

[jack-berg]

Was support of "gzip" always part of our OTLP receiver specification?

I think so, but there's a bit of interpretation involved in the answer.

Here's the first version of OTLP in the spec. It says:

The client MAY gzip the content and in that case SHOULD include "Content-Encoding: gzip" request header. The client MAY include "Accept-Encoding: gzip" request header if it can receive gzip-encoded responses.

A reader should assume that a functioning OTLP server should be able to read gzip payloads. This requirement to support gzip was solidified relatively recently on Sept 29, 2021, with the additional language:

All server components MUST support the following transport compression options:
No compression, denotated by none.
Gzip compression, denoted by gzip.

The justification for the gzip clarification was that the OTel JS SIG wanted to use gzip as the default for the browser case. Not sure where that landed.

Overall, my interpretation is that even from the beginning, an OTLP server that doesn't support gzip is not spec compliant, since the spec has always allowed clients to gzip their payloads.

This change may be overbroad.

[Aneurysm9]

Upon re-review and considering Tigran's comments, it seems that this change is overly broad as it changes the gRPC client and not the OTLP exporter's use of the gRPC client. This would impact any other exporter that uses configgrpc. That may not be a huge deal, but it's not what this PR says it does.

As for whether the change is safe, I think I agree with the reading of the spec that compliant OTLP consumers should have always supported gzip. If, however, some consumer does not, does this change result in the exporter emitting clear and actionable logs instructing the operator that they need to disable gzip? I think if that is done there should be no issue with enabling it by default.

[tigrannajaryan]

Was support of "gzip" always part of our OTLP receiver specification?

I think so, but there's a bit of interpretation involved in the answer.

Here's the first version of OTLP in the spec. It says:

The client MAY gzip the content and in that case SHOULD include "Content-Encoding: gzip" request header. The client MAY include "Accept-Encoding: gzip" request header if it can receive gzip-encoded responses.

@jack-berg you are quoting from the OTLP/HTTP section. That does not apply to OTLP/gRPC. In fact, unfortunately OTLP/gRPC spec doesn't say anything about compression as far as I can tell.

Given the state of the spec I don't think we can rely on the spec guiding our decision. I am reluctant on approving this change since I am worried we may break receivers that we have no idea about. Perhaps we could ask vendors who support OTLP in the backend, do some sort of poll, I am not sure.

[jack-berg]

it seems that this change is overly broad as it changes the gRPC client and not the OTLP exporter's use of the gRPC client.

Good point. I'll address this if we can agree on a strategy for changing the default.

That does not apply to OTLP/gRPC. In fact, unfortunately OTLP/gRPC spec doesn't say anything about compression as far as I can tell.

Oops - must have been reading too fast. That's a good point. That should mean that there isn't anything in the spec blocking making gzip the default for OTLP over HTTP.

In terms of OTLP over gRPC, I still do think there are a couple bits of evidence in spec that all compliant servers support gzip:

• The spec says all server components MUST support gzip. This change was added Sept 29th, 2021 after the OTLP when the tracing and metrics OTLP specs were marked as stable. The fact that this language was added with "MUST" and without any qualifying phrasing arguably indicates that the original omission of this language was not intentional. In other words, this language was a bugfix.
• The spec allows SDKs to default to gzip if they deem it most useful. An OTLP gRPC server that isn't compatible with the default options an SDK may choose is arguably not compliant. Shouldn't the collector be given the same freedom as SDKs to choose the most useful compression algorithm?

On a separate note, I do think that the argument for this change benefits from the collector being pre-1.0.0: It's a change that has a small but non-zero chance of impacting users, but which users can easily reconfigure and disable. It's a stroke of good luck that we're having this discussion now when users are more understanding of changes.

[github-actions]

This PR was marked stale due to lack of activity. It will be closed in 14 days.

[bogdandrutu]

@tigrannajaryan please help with this PRto even get merged or comment what we need to do.. you have better experience with the protocol specifications

[tigrannajaryan]

I think the change is reasonable safe from OTLP compliance perspective. There is a small chance that we may break some receiver that does not support compression. That's OK, the requires gzip support, so they will need to fix their implementation.

The only remaining question is this comment:

it seems that this change is overly broad as it changes the gRPC client and not the OTLP exporter's use of the gRPC client. This would impact any other exporter that uses configgrpc. That may not be a huge deal, but it's not what this PR says it does.

I haven't looked into other usages of configgrpc in the codebase. @jack-berg can you address this comment?

Once this is addressed we can merge.

[jack-berg]

I haven't looked into other usages of configgrpc in the codebase. @jack-berg can you address this comment?
Once this is addressed we can merge.

Yes, I can take care of that. 👍

[tigrannajaryan]

@jack-berg Please add a changelog entry.

[jack-berg]

Please add a changelog entry.

Let me know if you think a more descriptive entry is needed.

[jack-berg]

I'm not super familiar with this project - is the failure of the "contrib-tests" check expected?

[tigrannajaryan]

I'm not super familiar with this project - is the failure of the "contrib-tests" check expected?

It is possible. @open-telemetry/collector-approvers anyone knows what the contrib-test failure is? I am not sure.

[Aneurysm9]

This looks to be due to a new module added after v0.43.0 but using that tag in dependencies. There should be replace statements that make it work, not sure why it is failing here.

[tigrannajaryan]

@jack-berg can try a rebase from main?

[jack-berg]

Looks like main is fixed now and the build is passing so should be good to go 👍