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

Prepare for the metrics SDK experimental release #1912

Merged

Conversation

reyang
Copy link
Member

@reyang reyang commented Sep 8, 2021

This PR is pending on #1888 and #1913 to be merged.

Changes:

  • Removed the warning & banner.
  • Updated changelog.

@jack-berg
Copy link
Member

Hi @reyang - I'm curious to get your (and others) thoughts on when best to resolve Issue #1902 regarding some sort of support for a min, max, sum, count style aggregation.

The current thought is that we can achieve this by extending the histogram proto with optional min / max fields. If we went down this route, it would be nice to have this type of histogram specified in the SDK spec. Specifically, I think we could add a Zero Bucket Summary Histogram Aggregation specification akin to the Explicit Bucket Histogram Aggregation.

Personally, I think it would be valuable to make this aggregation available in the experimental release.

@jsuereth
Copy link
Contributor

jsuereth commented Sep 8, 2021

@jack-berg Adding min/max fields to histogram proto would be part of "metrics data model" specification. If we add min/max there, then implicitly histogram aggregators will need to be updated to support it. I think that, combined with providing no buckets to histogram explicit bucket will get you exactly what you want out of Summary from the Histogram instrument type.

Happy to help shepherd you through the process and drive Prs on appropriate repositories.

@reyang
Copy link
Member Author

reyang commented Sep 8, 2021

@jack-berg thanks for calling this out. I think these are not blockers for SDK spec Experimental release, and this is definitely a nice-to-have and we should get it before Feature-freeze. Here goes the 09/07/2021 #196 Metrics SIG meeting notes.

There are several parts I could imagine:

  1. The data model spec (which is Stable) has to be updated.
  2. The proto has to be updated, using the protobuf v3 OPTIONAL.
  3. The SDK/View might need to be updated so folks can specify if they want to spend the extra CPU/memory to get min/max, and whether it should be opt-in (default off) or opt-out (default on).

@reyang reyang added spec:metrics Related to the specification/metrics directory area:sdk Related to the SDK labels Sep 8, 2021
Copy link
Contributor

@jsuereth jsuereth left a comment

Choose a reason for hiding this comment

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

Approved, pending PRs listed as blocking.

@reyang reyang changed the title Prepare for the metrics SDK experiemental release Prepare for the metrics SDK experimental release Sep 10, 2021
@jmacd jmacd merged commit 5d27637 into open-telemetry:main Sep 14, 2021
@reyang reyang deleted the reyang/metrics-sdk-experimental-release branch September 14, 2021 15:28
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.

8 participants