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

Exemplar support in metrics.proto #39

Merged
merged 1 commit into from
Jan 18, 2020
Merged

Exemplar support in metrics.proto #39

merged 1 commit into from
Jan 18, 2020

Conversation

beorn7
Copy link
Member

@beorn7 beorn7 commented Jan 15, 2020

We need it here because client_golang uses this as the data model.

(There are also changes necessary in prometheus/common. This is just the start. I need this merged and tagged for proper Go Module support in the upcoming PR in prometheus/common, and then the same for the upcoming PR in prometheus/client_golang.)

@beorn7 beorn7 requested a review from cstyan January 15, 2020 22:43
metrics.proto Outdated
message Exemplar {
repeated LabelPair label = 1;
optional double value = 2;
optional double timestamp = 3; // OpenMetrics-style: seconds as float.
Copy link
Contributor

Choose a reason for hiding this comment

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

OM is using the timestamp from google/protobuf/timestamp.proto

Copy link
Contributor

Choose a reason for hiding this comment

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

(Not that you need to follow that, Prometheus wouldn't presumably support more than millisecond anyway)

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. That might even simplify things. I'll do a bit of research before merging this.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you just want to get something working for now, this is fine. If you want to fully support OpenMetrics longer term then nanoseconds should be supported even if we never end up using it in Prometheus itself.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think that this proto file will ever be considered as a real OpenMetrics representation. (The proto definition in the OM repo look quite different.) Since here, the timestamp is a float, nanosecend precision can be provided (and it is in my PoC implementation in expfmt). Nevertheless, consistency is good. I'll play with the timestamp.proto.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nanosecond precision can't be done with a float64, you can only get to microsecond.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, right. We live too far away from 1970-01-01…

This makes metrics.proto a bit more OpenMetrics-like. Note that this
is not an attempt to make metrics.proto a full and valid
representation of OpenMetrics. It's merely added here so that v1 of
prometheus/client_golang (which uses metrics.proto as the internal
data model) can expose OpenMetrics text format.

Signed-off-by: beorn7 <beorn@grafana.com>
@beorn7
Copy link
Member Author

beorn7 commented Jan 17, 2020

Changed timestamp to google/protobuf/timestamp.proto. PHAL.

@beorn7 beorn7 merged commit 7bc5445 into master Jan 18, 2020
@beorn7 beorn7 deleted the beorn7/exemplars branch January 18, 2020 22:30
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.

None yet

3 participants