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

View aggregation examples modified to remove unsupported things #2305

Merged

Conversation

cijothomas
Copy link
Member

The current spec for View does not allow specifying temporality (cumulative/delta) on aggregation, and it is only allowed along with MetricReader. This PR is modifying the examples to remove the unsupported capabilities, to avoid confusion.

jmacd
jmacd previously requested changes Feb 1, 2022
Copy link
Contributor

@jmacd jmacd left a comment

Choose a reason for hiding this comment

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

I'd like it to be clear how the reader should interpret the first argument of the reader. I want it to explain this is the treatment for synchronous instruments, which the SDK has control over. Asynchronous instruments should be exported in their natural temporality, and we do not want to encourage anyone to expect Cumulative-to-Delta conversion inside the SDK.

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

Asynchronous instruments should be exported in their natural temporality, and we do not want to encourage anyone to expect Cumulative-to-Delta conversion inside the S

Don't think this example need to talk about it. There is spec already existing here https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk.md#metricreader which talk about temporality, cumulative->delta conversions etc.

@jmacd
Copy link
Contributor

jmacd commented Feb 3, 2022

Don't think this example need to talk about it.

Later in the SDK specification, we write:

The SDK SHOULD provide a way to allow the preferred Aggregation
Temporality
to be specified for a MetricReader
instance during the setup

But the reader will not have reached that point in the specification yet, and I want it to be clear what this means.

I changed my suggestion from "using" to "preferring" which is more correct.

@jmacd
Copy link
Contributor

jmacd commented Feb 4, 2022

See #2314

@jmacd jmacd enabled auto-merge (squash) February 4, 2022 00:08
@jmacd jmacd merged commit 93d8388 into open-telemetry:main Feb 4, 2022
@cijothomas cijothomas deleted the cijothomas/remove_temporality_agg branch February 4, 2022 00:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants