RW 2.0-rc.4 (breaking): Move CT from TimeSeries to Sample; Rename to ST (Start Timestamp) #2762
Conversation
Given the recent movement for Prometheus native support of ST ([PROM-60](prometheus/proposals#60)) and plans for delta temporality ([PROM-48](prometheus/proposals#48)) it might be beneficial to make (hopefully) last change to Remote Write 2.0 before stabilizing, so: * Raname Created Timestamp to Start Timestamp * Move CT/ST from TimeSeries to Sample and Histogram messages. * Clarified optionality (0 value meaning unset) See implementation change that will follow: prometheus/prometheus#17411. Notice that only receiver part was implemented for CT/ST. Given no sending part was done we expect this feature (ST/CT) not being used, thus breakage impact is minimal. This has been confirmed with early adopters like Mimir (Grafana), Chronosphere, Thanos, Cortex and Google. See previous discussions and 3 expilcit approvals: prometheus/prometheus#17036 Additionally: * I updated link to proto * Updated links to new compliance tests * Update native histogram spec link Signed-off-by: bwplotka <bwplotka@gmail.com>
bwplotka
changed the title
bwplotka
changed the title
bwplotka
changed the title
| // in ms format. This information is typically used for counter, histogram (cumulative) | ||
| // or delta type metrics. | ||
| // | ||
| // For cumulative metrics, the start timestamp represents the time when the |
There was a problem hiding this comment.
nit: if you're going to explain cumulative metrics, I'd also say something about delta metrics. E.g. "For delta type metrics, the start timestamp represents the time to which the delta value is relative to ? Probably something better.
There was a problem hiding this comment.
They don't exist now and we can do it later. I'd skip for now.
| } | ||
| // Exemplar represents additional information attached to some series' samples. | ||
| // Exemplar is an additional information attached to some series' samples. |
There was a problem hiding this comment.
nit: not related to start time
There was a problem hiding this comment.
Yes. This is intended, see description. It's better to make changes in atomic version bump especially cosmetic like this.
| // TimeSeries represents a single series. | ||
| message TimeSeries { | ||
| reserved 6; |
There was a problem hiding this comment.
nit: why not put it in order ? I'd move to the end
There was a problem hiding this comment.
This is for brevity. This is just shorter snippet of proto.
| * `Written` refers to data the `Receiver` has received and is accepting. Whether or not it has ingested this data to persistent storage, written it to a WAL, etc. is up to the `Receiver`. The only distinction is that the `Receiver` has accepted this data rather than explicitly rejecting it with an error response. | ||
| * a `Sample` is a pair of (timestamp, value). | ||
| * a `Histogram` is a pair of (timestamp, [histogram value](https://github.com/prometheus/docs/blob/b9657b5f5b264b81add39f6db2f1df36faf03efe/content/docs/concepts/native_histograms.md)). | ||
| * a `Sample` is a triplet of (start timestamp, timestamp, value). |
There was a problem hiding this comment.
But start time is optional, so maybe
| * a `Sample` is a triplet of (start timestamp, timestamp, value). | |
| * a `Sample` is a pair (timestamp, value) or a triplet of (start timestamp, timestamp, value). |
and similar for Histograms.
There was a problem hiding this comment.
Nice I like it, although one could argue (0, timestamp, value) == (timestamp, value), no?
| // See https://github.com/prometheus/prometheus/blob/remote-write-2.0/prompb/io/prometheus/write/v2/types.proto#L142 | ||
| // for a full message that follows the native histogram spec for both sparse | ||
| // and exponential, as well as, custom bucketing. | ||
| // A native histogram message, supporting |
There was a problem hiding this comment.
nit: not related to start time
There was a problem hiding this comment.
Yes. This is intended, see description. It's better to make changes in atomic version bump especially cosmetic like this.
Co-authored-by: George Krajcsovits <krajorama@users.noreply.github.com> Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com>
Given the recent movement for Prometheus native support of ST (PROM-60) and plans for delta temporality
(PROM-48) it might be beneficial to make (hopefully) last change to Remote Write 2.0 before stabilizing, so:
This change is in theory breaking. However, practically we impact should be minimal. See implementation change that will follow. Notice that only receiver part was implemented for CT/ST. Given no sending part was done we expect this feature (ST/CT) not being used, thus breakage impact is minimal. This has been confirmed with early adopters like Mimir (Grafana), Chronosphere, Thanos, Cortex and Google.
See previous discussions and 3 explicit approvals: prometheus/prometheus#17036
Additionally:
cc @alexgreenbank @cstyan @ArthurSens @aknuds1 @krajorama @bboreham @kgoudeaux