Skip to content

Metrics supplemental guidelines for async views#2208

Merged
carlosalberto merged 10 commits intoopen-telemetry:mainfrom
jmacd:jmacd/supplemental_async_view
Jan 8, 2022
Merged

Metrics supplemental guidelines for async views#2208
carlosalberto merged 10 commits intoopen-telemetry:mainfrom
jmacd:jmacd/supplemental_async_view

Conversation

@jmacd
Copy link
Copy Markdown
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
Copy Markdown
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?

legendecas
legendecas reviewed Dec 9, 2021
View reviewed changes
Comment thread specification/metrics/supplementary-guidelines.md Outdated
legendecas
legendecas reviewed Dec 9, 2021
View reviewed changes
Comment thread specification/metrics/supplementary-guidelines.md
@jmacd jmacd marked this pull request as ready for review December 9, 2021 20:52
@jmacd jmacd requested review from a team December 9, 2021 20:52
pirgeo
pirgeo reviewed Dec 10, 2021
View reviewed changes
Copy link
Copy Markdown
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?

Comment thread specification/metrics/supplementary-guidelines.md Outdated
MrAlias
MrAlias approved these changes Dec 10, 2021
View reviewed changes
Comment thread CHANGELOG.md Outdated
@jmacd jmacd mentioned this pull request Dec 10, 2021
Merged
Joshua MacDonald and others added 4 commits December 10, 2021 14:35
Merge branch 'main' of github.com:open-telemetry/opentelemetry-specif…
79ebf71 
…ication into jmacd/supplemental_async_view
Explain the order of sections
8556ff6
@jmacd @MrAlias @pirgeo
Apply suggestions from code review
286a34c 
Co-authored-by: Tyler Yahn <[email protected]>
Co-authored-by: Georg Pirklbauer <[email protected]>
Merge branch 'jmacd/supplemental_async_view' of github.com:jmacd/open…
9a059c2 
…telemetry-specification into jmacd/supplemental_async_view
@jmacd
Copy link
Copy Markdown
Contributor Author

jmacd commented Dec 10, 2021

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

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Dec 25, 2021

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
legendecas
legendecas approved these changes Dec 25, 2021
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown

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
Copy Markdown
Contributor

carlosalberto commented Jan 3, 2022

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

@jmacd jmacd reopened this Jan 4, 2022
@jmacd
Copy link
Copy Markdown
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
reyang
reyang approved these changes Jan 7, 2022
View reviewed changes
Copy link
Copy Markdown
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
carlosalberto pushed a commit to carlosalberto/opentelemetry-specification that referenced this pull request Oct 31, 2024
@jmacd
Metrics supplemental guidelines for async views (open-telemetry#2208)
8700725
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jsuereth jsuereth jsuereth approved these changes

@MrAlias MrAlias MrAlias approved these changes

@reyang reyang reyang approved these changes

+2 more reviewers

@pirgeo pirgeo pirgeo left review comments

@legendecas legendecas legendecas approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@arminru arminru

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Clarify SumAggregation on asynchronous counter

8 participants

@jmacd @jsuereth @carlosalberto @MrAlias @legendecas @reyang @pirgeo @arminru