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

Mark cardinality limits as Stable #4222

Merged
merged 7 commits into from
Oct 7, 2024

Conversation

reyang
Copy link
Member

@reyang reyang commented Sep 23, 2024

Mark cardinality limits as Stable.

Fixes #3904

These are the language implementations:

Changes

Please provide a brief description of the changes here.

For non-trivial changes, follow the change proposal process.

pellared
pellared previously approved these changes Sep 24, 2024
@cijothomas
Copy link
Member

There are few issues that were raised earlier on this topic. We should mark them as resolved (and close the issue), or mark them as non-blocking for stability for cardinality limits.
#3904 (comment)

@reyang
Copy link
Member Author

reyang commented Sep 25, 2024

... or mark them as non-blocking for stability for cardinality limits

How do you want to mark them as non-blocking? @cijothomas

@MrAlias
Copy link
Contributor

MrAlias commented Sep 25, 2024

I feel this issue needs a resolution before this can become stable.

@cijothomas
Copy link
Member

... or mark them as non-blocking for stability for cardinality limits

How do you want to mark them as non-blocking? @cijothomas

TC can leave a comment on the issues saying "it is not a blocker for stabilizing cardinality limits feature spec".
(I don't think it worth creating github labels like blocking-cardinality-stable, allowed-cardinality-stable, nice-to-have-cardinality-stable etc, so a simple comment can help.)

@reyang
Copy link
Member Author

reyang commented Sep 27, 2024

I feel this issue needs a resolution before this can become stable.

@MrAlias Is this something that cannot be an additive change later, and must be resolved now? Would you help to clarify? Thanks!

@jmacd
Copy link
Contributor

jmacd commented Oct 2, 2024

@reyang My opinion is that #3813 should not be a blocker. We are trying to impose a limit on the SDK without specifying the exact mechanism that it uses, because SDKs will take different and reasonable approaches to aggregation, and all that matters is that they reach a limit and respect it. How the limit is reached and respected is an implementation detail, and when the limit is reached the telemetry coming from an application is degraded, that is all that I expect from cardinality limits.

Copy link
Member

@cijothomas cijothomas left a comment

Choose a reason for hiding this comment

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

LGTM. The listed issues are non-blocking in my opinion, so we can proceed with stabilizing.

@reyang reyang merged commit 90a9ed1 into open-telemetry:main Oct 7, 2024
6 checks passed
@reyang reyang deleted the reyang/cardinality-limits branch October 7, 2024 17:15
@carlosalberto carlosalberto mentioned this pull request Oct 8, 2024
carlosalberto added a commit that referenced this pull request Oct 10, 2024
October 2024 Release.

Changes of special interest:

* [Mark cardinality limits as
Stable](#4222)
carlosalberto pushed a commit to carlosalberto/opentelemetry-specification that referenced this pull request Oct 31, 2024
carlosalberto added a commit to carlosalberto/opentelemetry-specification that referenced this pull request Oct 31, 2024
October 2024 Release.

Changes of special interest:

* [Mark cardinality limits as
Stable](open-telemetry#4222)
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.

Stabilize Overflow attribute section under Cardinality Limits
8 participants