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

Rename None aggregation to Drop #2101

Merged
merged 9 commits into from
Nov 16, 2021

Conversation

cijothomas
Copy link
Member

@cijothomas cijothomas commented Nov 5, 2021

Drop feels like a better name than "none". Want to check if others feel the same.

"The None Aggregation informs the SDK to ignore/drop all Instrument Measurements for this Aggregation."

Found some history: https://github.com/open-telemetry/opentelemetry-specification/pull/956/files

@jsuereth
Copy link
Contributor

jsuereth commented Nov 6, 2021

From one point of view I agree with this, WDYT of this change in relation to: #2102

@cijothomas cijothomas added the spec:metrics Related to the specification/metrics directory label Nov 8, 2021
@reyang
Copy link
Member

reyang commented Nov 12, 2021

We've discussed this during the 11/11/2021 Metrics SIG Meeting, folks think Drop is better than None as it gives clarity by explicitly communicating the intention.

I suggest that we move forward and merge the PR before Stable release.

@reyang reyang added this to In progress in Spec - Metrics API/SDK Stable Release via automation Nov 12, 2021
@reyang reyang added this to the Metrics API/SDK Stable Release milestone Nov 12, 2021
Copy link
Member

@jonatan-ivanov jonatan-ivanov left a comment

Choose a reason for hiding this comment

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

+1 for Drop: dropping values has different meaning than not aggregating them.

@arminru arminru added the area:sdk Related to the SDK label Nov 16, 2021
specification/metrics/sdk.md Show resolved Hide resolved
Copy link
Contributor

@jsuereth jsuereth left a comment

Choose a reason for hiding this comment

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

Approve pending Armin's comment

@jmacd jmacd merged commit b594f44 into open-telemetry:main Nov 16, 2021
Spec - Metrics API/SDK Stable Release automation moved this from In progress to Done Nov 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:sdk Related to the SDK spec:metrics Related to the specification/metrics directory
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

10 participants