(feat): Add created timestamps to counter, summary and histogram

[ArthurSens]

Following the design document, this PR adds the created timestamp to Prometheus protobuf.

[ArthurSens]

cc @bwplotka @macxamin @TheSpiritXIII

[ArthurSens]

Related to prometheus/common#504

[bwplotka]

Amazing work!

PTAL @beorn7 as I never modified this file. With the change in name or type in my suggestion, I would merge it (:

I don't like the inconsistency of Exemplar.timestamp vs Metric.timestamp_ms - I think we should (if we plan to use this proto more) to fix in some direction (in separate PR ofc). Sounds like timestamp.proto is the direction: #39 (comment)

[bwplotka]

Nice! I would vote for unit in the name so we protocol users can clearly pass that OR the timestamp type.

Also "time" or "ts" or "timestamp" word in the name would help too.

E.g. following what Exemplar message has:

Suggested change

	optional int64 created = 3;
	optional google.protobuf.Timestamp created_timestamp = 3;

The alternative:

Suggested change

	optional int64 created = 3;
	optional int64 created_timestamp_ms = 3;

What would be your choice @beorn7 ?

[bwplotka]

Sounds like google.protobuf.Timestamp might be a better choice: #39 (comment)

[ArthurSens]

Agreed to have consistency between all timestamp fields in the proto! Do we have a consensus about resurrecting this format though? I had the impression that we haven't even discussed with the Prometheus team yet

If we're using just for experimentation with created timestamps, I guess consistency is not a big deal right now

[beorn7]

Yeah, let's use google.protobuf.Timestamp and the name created_timestamp. That's consistent with how we handled timestamps for exemplars. The timestamp_ms is simply legacy that we cannot easily change.

[beorn7]

Let's separate this by a blank line and a comment explaining that it doesn't belong to native histograms specifically anymore (and also update the comment in line 75, which currently reads "Everything below here is for native histograms…").

[beorn7]

Maybe you could even pull it up to add this line just after the classic buckets. Might be least confusing, even if the field numbering is out of order then.

[ArthurSens]

I just moved it upwards :)

[beorn7]

Note that you have to do similar changes in the proto3 file in prometheus/prometheus: https://github.com/prometheus/prometheus/blob/main/prompb/io/prometheus/client/metrics.proto

(Sorry for the duplication. It's a messed up situation.)

[beorn7]

Do we have a consensus about resurrecting this format though? I had the impression that we haven't even discussed with the Prometheus team yet

I guess the goal is to tidy up the OM situation and then use the OM protobuf as the future protobuf format, thereby avoiding to formally resurrect the old protobuf for anything else than experimentation.

However, I don't expect that the existing protobuf parse code in prometheus/prometheus will be ripped out any time soon. As long as keeping it in isn't a significant maintenance burden, we don't have to break anyone.

[ArthurSens]

I guess the goal is to tidy up the OM situation and then use the OM protobuf as the future protobuf format, thereby avoiding to formally resurrect the old protobuf for anything else than experimentation.

I think we need to discuss with OM's team about this as well since there's a possibility that OM will be archived.

[beorn7]

By "tidy up the OM situation", I mean that this archiving thing has to get resolved. And then things have to get moving around native histograms, see OpenObservability/OpenMetrics#256 , and other issues of OM, see https://docs.google.com/document/d/1P4SVXBYsDSeUThDJrI_5_aWv6mWemx4x0e0swq7vq6Q/edit#heading=h.5prvoamow70t and https://docs.google.com/document/d/1VuUBdRyIDR2uID2j2abWGt6PbWu7o-s06btZgrwH4vQ/edit#heading=h.5prvoamow70t

All of this shouldn't keep us back, so changing things here and in https://github.com/prometheus/prometheus/blob/main/prompb/io/prometheus/client/metrics.proto is fine. Don't worry to much about how things around OM will evolve in the future.