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.

Sign up for GitHub

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

Jump to bottom

Add details for Aggregator in Metrics Spec. #1842

Merged
merged 17 commits into from
Aug 19, 2021
Merged

Add details for Aggregator in Metrics Spec. #1842

merged 17 commits into from
Aug 19, 2021

Conversation

victlu
Copy link
Contributor

@victlu victlu commented Aug 2, 2021

Add more details for an Aggregator in the Metrics Spec.

This PR is a rework of #1804 after discussions and feedback from metric SIG meetings.

@victlu victlu requested review from a team August 2, 2021 15:29
@victlu victlu mentioned this pull request Aug 2, 2021
Closed
pirgeo
pirgeo reviewed Aug 3, 2021
View reviewed changes
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.

Flagged a few typos, but overall I think it is very clear and concise 👍

specification/metrics/sdk.md Outdated Show resolved Hide resolved
specification/metrics/sdk.md Outdated Show resolved Hide resolved
specification/metrics/sdk.md Outdated Show resolved Hide resolved
specification/metrics/sdk.md Outdated Show resolved Hide resolved
specification/metrics/sdk.md Outdated Show resolved Hide resolved
specification/metrics/sdk.md Outdated Show resolved Hide resolved
specification/metrics/sdk.md Outdated Show resolved Hide resolved
specification/metrics/sdk.md Show resolved Hide resolved
specification/metrics/sdk.md Outdated Show resolved Hide resolved
specification/metrics/sdk.md Outdated Show resolved Hide resolved
jmacd
jmacd reviewed Aug 6, 2021
View reviewed changes
specification/metrics/sdk.md Outdated Show resolved Hide resolved
specification/metrics/sdk.md Outdated Show resolved Hide resolved
specification/metrics/sdk.md Show resolved Hide resolved
@jsuereth jsuereth mentioned this pull request Aug 9, 2021
Merged
@jsuereth
Copy link
Contributor

Want to confirm a few points that I think are implicit in this PR that perhaps deserve some in person discussion:

  1. Default aggregation for synchronous instruments is DELTA right now, not cumulative.
  2. This PR does not define any kind of "back-hook" where exports can ask for a specific type of temporal aggregation (delta vs. cumulative). The configuration of the View (or default) determines the temporarlity.
  3. The public names exposed in SDK to users are "GAUGE", "SUM" and "HISTOGRAM", vs. the more specific name used in the section.

jmacd
jmacd approved these changes Aug 12, 2021
View reviewed changes
specification/metrics/sdk.md Outdated Show resolved Hide resolved
@jmacd
Copy link
Contributor

jmacd commented Aug 12, 2021

Thank you @victlu. I know this has been a lot of work!

reyang
reyang approved these changes Aug 17, 2021
View reviewed changes
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.

@jmacd
Copy link
Contributor

jmacd commented Aug 17, 2021

@cijothomas are you happy with the edits that @victlu has made? This looks ready to merge.

cijothomas
cijothomas reviewed Aug 17, 2021
View reviewed changes
specification/metrics/sdk.md
### Aggregation

An `Aggregation`, as configured via the [View](./sdk.md#view),
informs the SDK on the ways and means to compute
Copy link
Member

Choose a reason for hiding this comment

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

nit: since we don't specify any "how" here, "ways and means" sounds too much.

cijothomas
cijothomas approved these changes Aug 17, 2021
View reviewed changes
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.

left small editorial comments.

@jmacd jmacd merged commit 54d2e9c into open-telemetry:main Aug 19, 2021
@reyang reyang mentioned this pull request Oct 19, 2021
Closed
@pirgeo pirgeo mentioned this pull request Nov 30, 2021
Closed
6 tasks
This was referenced Feb 24, 2022
Closed
Closed
carlosalberto pushed a commit to carlosalberto/opentelemetry-specification that referenced this pull request Oct 31, 2024
@victlu @jmacd
Add details for Aggregator in Metrics Spec. (open-telemetry#1842)
03d2fa6 
* Aggregator Spec

* Cleanup

* Tweaks

* tweaks

* tweak

* Fix up nits from PR feedback

* Fix up nits from PR feedback

* Refactor based on SIG meeting feedback

* Allow idiomatic approach for Aggregator+Params

* Add Timestamp for Last Value Aggregator

* Fix misspelling

* Refactor to use Aggregation (as a config)

* Refactor to use Aggregation (as a config)

* Fix based on PR feedback

* fix misspelling

Co-authored-by: Joshua MacDonald <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pirgeo pirgeo pirgeo left review comments

@jkwatson jkwatson jkwatson left review comments

@jsuereth jsuereth jsuereth approved these changes

@reyang reyang reyang approved these changes

@cijothomas cijothomas cijothomas approved these changes

@jmacd jmacd jmacd approved these changes

Assignees

@jmacd jmacd

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@victlu @jsuereth @jmacd @jkwatson @cijothomas @reyang @pirgeo