opentelemetry-ruby icon indicating copy to clipboard operation
opentelemetry-ruby copied to clipboard

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

Open patriciomacadden opened this issue 9 months ago • 7 comments

patriciomacadden avatar Mar 03 '25 02:03 patriciomacadden

CLA Signed

The committers listed above are authorized under a signed CLA.

  • :white_check_mark: login: patriciomacadden / name: Patricio Mac Adden (927e53fa70795be7a7f994aac090ee2b532e6db3, 470c9275dbaf32b513040c639841fc0283224f30, a2aacad0f96f347f5c7237af3ae5deaa93d226f5)

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 https://github.com/open-telemetry/opentelemetry-ruby/pull/934 as a result of this spec change: https://github.com/open-telemetry/opentelemetry-specification/pull/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: https://github.com/open-telemetry/opentelemetry-ruby/blob/aacd8c8e264b110507c2a733dcc5309ca26aac66/exporter/otlp-http/lib/opentelemetry/exporter/otlp/http/trace_exporter.rb#L39

fbogsany avatar Mar 04 '25 15:03 fbogsany

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.

patriciomacadden avatar Mar 04 '25 17:03 patriciomacadden

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? https://github.com/open-telemetry/opentelemetry-ruby/issues/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.

arielvalentin avatar Mar 05 '25 17:03 arielvalentin

👋 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

github-actions[bot] avatar Apr 05 '25 02:04 github-actions[bot]

@arielvalentin, @fbogsany - checking in on this issue.

kaylareopelle avatar May 06 '25 16:05 kaylareopelle

I don't think my plea was successful.

arielvalentin avatar May 07 '25 12:05 arielvalentin

👋 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

github-actions[bot] avatar Jul 12 '25 02:07 github-actions[bot]