tikv/rust-prometheus

Metrics::set_timestamp_ms incorrectly implies milliseconds should be used

Mikadore opened this issue · 6 comments

Mikadore commented

Metric::set_timestamp_ms is incorrectly named _ms, even though both prometheus and the Openmetrics standard expect this to be in seconds. Furthermore, since the code just stores the number as an i64, the semantics of the function don't care about units.

Metric::set_timestamp_ms should be either renamed to Metric::set_timestamp_s or just purely Metric::set_timestamp. In any case, it should be documented that both prometheus and openmetrics require seconds.

lucab commented

Thanks for the report. It would be good if you could link some references for your statements. According to https://prometheus.io/docs/instrumenting/exposition_formats/#comments-help-text-and-type-information, that field is:

The timestamp is an int64 (milliseconds since epoch, i.e. 1970-01-01 00:00:00 UTC, excluding leap seconds), represented as required by Go's ParseInt() function.

Mikadore commented

Hi, sorry for that. The relevant link is here. We needed the timestamps at work in order to support backfilling metrics into prometheus, so I can confirm firsthand that using promtool's promtool tsdb create-blocks-from openmetrics expects the metrics to be in seconds, meaning that using millis results in ridiculous far-distant future dates.

I may be wrong in approaching this only from the exporting and then backfilling perspective, but in either case I think that just naming it set_timestamp, not specifying units would be at least not a bad approach.

lucab commented

Oh well, it is sad that OpenMetrics has a different timestamp format, but in the end it's an explicitly new/separate standard so they are free to diverge however they prefer. Additionally, it seems that the Prometheus field is a signed integer, while the OpenMetrics one is a float with nanoseconds precision.

mxinden commented

For this crate, the the correct reference would be the one below, given that it implements the old Prometheus output format and not the new OpenMetrics output format.

https://prometheus.io/docs/instrumenting/exposition_formats/#comments-help-text-and-type-information

so I can confirm firsthand that using promtool's promtool tsdb create-blocks-from openmetrics expects the metrics to be in seconds

promtool tsdb create-blocks-from expects the metric file to be in the OpenMetrics output format, and not the old Prometheus output format.

➜  prometheus-2.31.0.linux-amd64 ./promtool tsdb --help
usage: promtool tsdb <command> [<args> ...]

[...]

Subcommands:

[ ...] 

  tsdb create-blocks-from openmetrics <input file> [<output directory>]
    Import samples from OpenMetrics input and produce TSDB blocks. Please refer to the storage docs for more details.

In case using the old Prometheus format is fine for you @Mikadore, you can continue using Metric::set_timestamp_ms using milliseconds, but make sure to set the content type to text/plain; version=0.0.4 in order to tell a Prometheus server to parse the output in the old format.

https://github.com/prometheus/docs/blob/main/content/docs/instrumenting/exposition_formats.md

  • 👍1
lucab commented

I split some further promtool discussion to prometheus/prometheus#9661.

lucab commented

Got some further feedback on the upstream discussion. The summary is:

  • "old" Prometheus format and "new" OpenMetrics format are seemingly similar but incompatible, at least on the topic of timestamps (by design)
  • promtool tsdb create-blocks-from only accepts the OpenMetrics format and they have no plans to support the "old" format

Thus both this library current behavior and promtool are correct, and they are incompatible by upstream design decisions.
I'm closing this ticket as there isn't an actionable step here.