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

Metrics supplemental guidelines for async views #2208

Merged

Conversation

jmacd
Copy link
Contributor

@jmacd jmacd commented Dec 8, 2021

Fixes #2187

Changes

Clarifies the available strategies for calculating asynchronous Views and what it means for the receiver, especially when streams "die" and restart, requiring reset detection.

Related issues #1874

@jmacd jmacd added spec:metrics Related to the specification/metrics directory release:required-for-ga Must be resolved before GA release, or nice to have before GA labels Dec 8, 2021
@jmacd jmacd requested a review from jsuereth December 8, 2021 22:03
@jsuereth
Copy link
Contributor

jsuereth commented Dec 9, 2021

This looks great to me. I think it nails options available to SDKs and provides a simple, allowable implementation, demonstrating why it works.

Should we recommend ONE option to SDKs first though, while allowing the others?

Co-authored-by: legendecas <legendecas@gmail.com>
@jmacd jmacd marked this pull request as ready for review December 9, 2021 20:52
@jmacd jmacd requested review from a team as code owners December 9, 2021 20:52
Copy link
Member

@pirgeo pirgeo left a comment

Choose a reason for hiding this comment

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

Thanks for the clarification! I wonder: for the Synchronous example, there are the Delta, then the Cumulative aggregation temporality sections, but for Asynchronous there is the Cumulative section first, then the Delta section. In the attribute removal section right after that, there are references to Cumulative temporality again. Do you think it would make sense to swap the Delta and Cumulative sections for the Asynchronous example?

specification/metrics/supplementary-guidelines.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
jmacd and others added 4 commits December 10, 2021 14:35
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
Co-authored-by: Georg Pirklbauer <georg.pirklbauer@dynatrace.com>
…telemetry-specification into jmacd/supplemental_async_view
@jmacd
Copy link
Contributor Author

jmacd commented Dec 10, 2021

@pirgeo to rationalize the differently-ordered temporality sections for sync vs async, 8556ff6.

@github-actions
Copy link

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Dec 25, 2021
Copy link
Member

@legendecas legendecas left a comment

Choose a reason for hiding this comment

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

👍

@github-actions
Copy link

github-actions bot commented Jan 2, 2022

Closed as inactive. Feel free to reopen if this PR is still being worked on.

@github-actions github-actions bot closed this Jan 2, 2022
@carlosalberto
Copy link
Contributor

@jmacd Should we reopen this PR? It had a few approvals already, etc ;)

@jmacd jmacd reopened this Jan 4, 2022
@jmacd
Copy link
Contributor Author

jmacd commented Jan 4, 2022

Given that these are guidelines--not specs--and have several approvals, I feel this can merge. @reyang PTAL.

@github-actions github-actions bot removed the Stale label Jan 5, 2022
Copy link
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

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

LGTM.

@carlosalberto carlosalberto merged commit 507e9de into open-telemetry:main Jan 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:required-for-ga Must be resolved before GA release, or nice to have before GA spec:metrics Related to the specification/metrics directory
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clarify SumAggregation on asynchronous counter
8 participants