Skip to content

fix: OTLP exporter compression should be none by default. fixes #1798. #1822

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

patriciomacadden
Copy link

No description provided.

Copy link

linux-foundation-easycla bot commented Mar 3, 2025

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: patriciomacadden / name: Patricio Mac Adden (a2aacad, 927e53f)

@kaylareopelle kaylareopelle linked an issue Mar 3, 2025 that may be closed by this pull request
@fbogsany
Copy link
Contributor

fbogsany commented Mar 4, 2025

The spec carries the important caveat:

[3]: If no compression value is explicitly specified, SIGs can default to the value they deem most useful among the supported options.

We chose to make gzip the default in #934 as a result of this spec change: open-telemetry/opentelemetry-specification#1923. Do we really want to reverse this decision? For users that rely on the default compression, this change in behaviour risks an erosion of trust in the project.

cc @arielvalentin

Also note that there's another copy of this code here:

compression: OpenTelemetry::Common::Utilities.config_opt('OTEL_EXPORTER_OTLP_TRACES_COMPRESSION', 'OTEL_EXPORTER_OTLP_COMPRESSION', default: 'gzip'),

@patriciomacadden
Copy link
Author

thanks for reviewing @fbogsany . I'll amend the trace exporter as well.

Regarding your other comment, I'm not sure and I'll leave the decision to you. I'm an opentelemetry user, I was reading the issue list and I thought it was a good opportunity to start contributing to the project. So feel free to close this PR.

@arielvalentin
Copy link
Contributor

The spec carries the important caveat:

[3]: If no compression value is explicitly specified, SIGs can default to the value they deem most useful among the supported options.

We chose to make gzip the default in #934 as a result of this spec change: open-telemetry/opentelemetry-specification#1923. Do we really want to reverse this decision? For users that rely on the default compression, this change in behaviour risks an erosion of trust in the project.

What process have you all used in the past to make big decisions like this? I haven't earned my maintainers badge so all I can do is lay out a case for this. I did not get any feedback/input/concerns on the issue I opened so I assumed this was ok? #1798

I advocated for switching to none since is my experience so far has been inconsistent accross language SIGs. AFAICT Java, Go, and JavaScript default to no compression making Ruby the outlier and that has caught some of our end users by surprise.

I do empathaize with how this will impact our end users, however IMO it is also difficult to tell measure "erosion of trust" since we do not capture those metrics or have a sense outside of GitHub how important these kind of changes are.

I do understand the sentiment of making a big change like this but the gem has not yet made 1.x status giving us some flexibility in making changes. It may be frustrating for users who are expecting default compression to gzip to change in the OTLP Exporters but if we accept this change we should probably communicate it as a breaking change.

All that being said, I will leave the final decision of which optional default settings in the SDK components to the maintainers.

Copy link
Contributor

github-actions bot commented Apr 5, 2025

👋 This pull request has been marked as stale because it has been open with no activity. You can: comment on the issue or remove the stale label to hold stale off for a while, add the keep label to hold stale off permanently, or do nothing. If you do nothing this pull request will be closed eventually by the stale bot

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OTLP Exporter Compression should be none by default
4 participants