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 min/max fields to HistogramDataPoint #279

Merged
merged 14 commits into from
Apr 1, 2022

Conversation

jmacd
Copy link
Contributor

@jmacd jmacd commented Mar 15, 2021

Adds a definition for min and max in the Histogram data point.
See the definition of these fields in the data model.
Fixes #266.

@jmacd jmacd requested review from a team March 15, 2021 19:40
@jmacd
Copy link
Contributor Author

jmacd commented Mar 15, 2021

Note the idea would be to apply this to the Histogram point after PR #272.

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
@jmacd
Copy link
Contributor Author

jmacd commented Jun 14, 2021

This is stale and we may revisit this topic after other histogram work is finished.

@jmacd jmacd closed this Jun 14, 2021
@jmacd jmacd reopened this Sep 7, 2021
@jmacd
Copy link
Contributor Author

jmacd commented Sep 16, 2021

I modified this PR to use proto3 "optional" which means to use explicit presence, see this document:
https://chromium.googlesource.com/external/github.com/protocolbuffers/protobuf/+/refs/heads/master/docs/field_presence.md

Copy link
Member

@mx-psi mx-psi left a comment

Choose a reason for hiding this comment

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

Now that #322 has been merged, should this PR add min/max fields for ExponentialHistogram too?

opentelemetry/proto/metrics/v1/metrics.proto Show resolved Hide resolved
opentelemetry/proto/metrics/v1/metrics.proto Outdated Show resolved Hide resolved
@jmacd
Copy link
Contributor Author

jmacd commented Oct 5, 2021

The build says:

opentelemetry/proto/metrics/v1/metrics.proto: This file contains proto3 optional fields, but --experimental_allow_proto3_optional was not set.

@codeboten
Copy link
Contributor

I believe with open-telemetry/opentelemetry-collector#4929 this is now fully unblocked. @jmacd please rebase the branch

@jmacd
Copy link
Contributor Author

jmacd commented Mar 2, 2022

Thank you @codeboten!

@codeboten
Copy link
Contributor

@bogdandrutu please review

@bogdandrutu
Copy link
Member

@jmacd @codeboten as in any ideal world, we should start with fixing a bug first #336 than adding new functionality. Should we merge the optional sum?

@codeboten
Copy link
Contributor

Either one is fine with me @bogdandrutu, I don't think I have permissions to push changes to #336 to unblock the PR 😬

@bogdandrutu
Copy link
Member

Let’s fix that first. Maybe clone that PR and open a new one with necessary changes.

@codeboten
Copy link
Contributor

@bogdandrutu see #366

@bogdandrutu
Copy link
Member

@codeboten I want to do a release #367 then start using this in collector to prove everything is fine, then will merge this.

@jmacd jmacd mentioned this pull request Apr 1, 2022
@Falmarri
Copy link

I modified this PR to use proto3 "optional" which means to use explicit presence, see this document: https://chromium.googlesource.com/external/github.com/protocolbuffers/protobuf/+/refs/heads/master/docs/field_presence.md

This is really annoying. It means opentelemetry isn't build-able or usable on ubuntu bionic with upstream provided packages.

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.

Add min and max values to the histogram data points
9 participants