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

Clarify Metric data type #2106

Merged

Conversation

reyang
Copy link
Member

@reyang reyang commented Nov 5, 2021

Fixes #2073.

This change adds extra clarification, it doesn't change the spec requirement.

@reyang reyang requested review from a team November 5, 2021 19:31
@reyang reyang added area:sdk Related to the SDK spec:metrics Related to the specification/metrics directory labels Nov 5, 2021
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

reyang and others added 2 commits November 5, 2021 19:39
Co-authored-by: Cijo Thomas <cithomas@microsoft.com>
Copy link
Contributor

@MrAlias MrAlias left a comment

Choose a reason for hiding this comment

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

This tries to clarify the types that implementations can use, but I think the original issue that this was opened to resolve is that the term Metric itself is not well defined. Here in the specification we are using the term to describe a group of MetricPoints, but above it also describes a "metric time series". This seems like a overloading of the term.

Additionally, above we refer to Aggregated Metrics above, but it seems to be referring to the same conceptual thing we a describing here.

I think adding these changes can help clarify implementation details for implementers, but we should also be consistent in our use of terms. I would recommend using Aggregated Metrics here if they are the same as above, or maybe instead just say "a batch of MetricPoint groups".

specification/metrics/sdk.md Outdated Show resolved Hide resolved
@reyang
Copy link
Member Author

reyang commented Nov 15, 2021

I think adding these changes can help clarify implementation details for implementers, but we should also be consistent in our use of terms. I would recommend using Aggregated Metrics here if they are the same as above, or maybe instead just say "a batch of MetricPoint groups".

I've updated the wording, PTAL.

@reyang reyang force-pushed the reyang/metrics-sdk-clarify-metric-type branch from a405c96 to 7eeede9 Compare November 15, 2021 18:17
@jmacd jmacd merged commit e2a4f05 into open-telemetry:main Nov 16, 2021
@reyang reyang deleted the reyang/metrics-sdk-clarify-metric-type branch November 17, 2021 03:59
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
None yet
Development

Successfully merging this pull request may close these issues.

Clarification on model metric of exporter.export(batch)
9 participants