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

Add Gauge. Change Metric type to be a more detailed structure with details about the aggregation and temporality. #182

Merged
merged 9 commits into from
Jul 30, 2020

Conversation

bogdandrutu
Copy link
Member

@bogdandrutu bogdandrutu commented Jul 29, 2020

This PR takes a more descriptive approach where the Aggregation (the last applied aggregation if any) is clearly defined and Temporality is available only where it makes sense. This can help clearly identify what are the possible values and properties for different types of Aggregations.

There are some things that can be considered (TODOs left in the code):

IMPORTANT:

@bogdandrutu bogdandrutu requested review from a team July 29, 2020 04:44
@bogdandrutu bogdandrutu changed the title Change Metric type to be a more detailed structure with details about the aggregation and temporality. [Current] Change Metric type to be a more detailed structure with details about the aggregation and temporality. Jul 29, 2020
… the aggregation and temporality.

This PR takes a more descriptive approach where the Aggregation (the last applied aggregation if any) is clearly defined and Temporality is available only where it makes sense. This can help clearly identify what are the possible values and properties for different types of Aggregations.

There are some things that can be considered (TODOs left in the code):

* open-telemetry/opentelemetry-specification#725
* Histogram/Summary sum - monotonicity?
* Previous aggregation measurements: raw measurements or "derived" measurements (results of another aggregation).
* Support for RawMeasurements (recorded via the Sync Instruments)
* Decide if support for INSTANTANEOUS temporality is still needed.

IMPORTANT:
* This PR is not equivalent with open-telemetry#168 or open-telemetry#181, this is inspired by these PRs but makes some changes that are not compatible with that PR.
* This PR is an incremental update, without any significant semantic changes (except adding the notion of GAUGE), from the current state, more changes will be added in the future to resolve the TODOs.

Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
@bogdandrutu
Copy link
Member Author

bogdandrutu commented Jul 29, 2020

@james-bebbington

  • Temp = Instantaneous only seems to be relevant for "Last" (i.e. Summary?) or Raw values, whereas Temp = Delta/Cumulative seems to be only relevant for Gauge/Histogram. Should these concepts be separated?
  • I'm not across the exemplar stuff, but I would prefer to keep individual measurements (raw measurements?) as a separate category similar to your not-current PR. I think sharing the same data structure for exemplars and Instantaneous aggregations is confusing and unnecessary.

@bogdandrutu

  • Temp - there is a todo to be discussed in a followup PR, trying to limit changes in this
  • Exemplar - Not yet finalized, more discussion to happen at that moment

@bogdandrutu bogdandrutu changed the title [Current] Change Metric type to be a more detailed structure with details about the aggregation and temporality. Add Gauge. Change Metric type to be a more detailed structure with details about the aggregation and temporality. Jul 29, 2020
@bogdandrutu
Copy link
Member Author

@open-telemetry/specs-metrics-approvers please review this 👍

@james-bebbington
Copy link
Member

(please remember to tag with "Metrics" label)

Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
Copy link
Member

@aabmass aabmass left a comment

Choose a reason for hiding this comment

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

LGTM

opentelemetry/proto/metrics/v1/metrics.proto Outdated Show resolved Hide resolved
Co-authored-by: Aaron Abbott <aaronabbott@google.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.

LGTM, only asks are around comments. Nothing blocking.

opentelemetry/proto/metrics/v1/metrics.proto Outdated Show resolved Hide resolved
opentelemetry/proto/metrics/v1/metrics.proto Outdated Show resolved Hide resolved
opentelemetry/proto/metrics/v1/metrics.proto Outdated Show resolved Hide resolved
opentelemetry/proto/metrics/v1/metrics.proto Outdated Show resolved Hide resolved
opentelemetry/proto/metrics/v1/metrics.proto Outdated Show resolved Hide resolved
opentelemetry/proto/metrics/v1/metrics.proto Outdated Show resolved Hide resolved
opentelemetry/proto/metrics/v1/metrics.proto Show resolved Hide resolved
bogdandrutu and others added 6 commits July 30, 2020 10:18
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
@bogdandrutu
Copy link
Member Author

This PR has 6 approves, and 3 owners approves, + myself so I think it is good to be merged.

@bogdandrutu bogdandrutu merged commit 7ae34aa into open-telemetry:master Jul 30, 2020
@bogdandrutu bogdandrutu deleted the currenttype branch July 30, 2020 22:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants