Skip to content
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

Allow implementations to use their own default for OTLP compression. #1923

Merged

Conversation

carlosalberto
Copy link
Contributor

As discussed in yesterday's Spec SIG call, allow SIGs to define their own default. Specially important as OTel JS will be going GA very soon, and we want to allow it to use gzip as their default (at least for the browser case) - also a motivating reason to include this change in this week's Specification release.

(Also removed the mention of SIGs supporting other compression methods, as they should be vetted first by the Collector, etc)

Alternative to #1895

cc @dyladan @bogdandrutu

@arminru arminru added area:configuration Related to configuring the SDK area:sdk Related to the SDK labels Sep 15, 2021
@carlosalberto carlosalberto mentioned this pull request Sep 15, 2021
Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Member

@dyladan dyladan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good from JS perspective. Sorry for late review i had vacation

@tigrannajaryan
Copy link
Member

As discussed in the SIG, I think the best that we do this:

  • The OTLP protocol spec to require all OTLP receivers to support both "none" and "gzip" compression.
  • The SDK spec to allow SDKs exporters to use "none" or "gzip" as the default compression method when OTEL_EXPORTER_OTLP_TRACES_COMPRESSION env variable is unspecified. The choice of the default is language-specific.

@carlosalberto
Copy link
Contributor Author

@tigrannajaryan Updated it to match the feedback, please review again.

@dyladan FYI, now the new explicit none option exists (besides gzip).

Copy link
Member

@tigrannajaryan tigrannajaryan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, except the "none" part.

specification/protocol/exporter.md Show resolved Hide resolved
@carlosalberto
Copy link
Contributor Author

@tigrannajaryan @yurishkuro Used none for no compression now. Please confirm so we can go ahead and merge.

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@carlosalberto carlosalberto changed the title Allow SIGs to use their own default for OTLP compression. Allow implementations to use their own default for OTLP compression. Sep 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:configuration Related to configuring the SDK area:sdk Related to the SDK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants