RW 2.0-rc.5: allow exemplar-only time series - #3085
Conversation
|
This looks what wanted as per https://docs.google.com/document/d/1ZcyKVF87Xf6FhaGkb33LgVLb27FH5NIlRXWEGO2Cc4Q/edit?tab=t.0#bookmark=id.nllt8bbnl0i decision. However we need strict versioning. I initially planned to pack this and async change under rc.5 for an easy reference -- we don't use git tags so it's a bit challenging to pack two separate changes under same version (: |
06d0a88 to
73870cf
Compare
|
Retitled with the version, the way rc.4 was done in #2762, so the title carries the record that git tags would otherwise hold. #3080 keeps the single bump and #3081 is the other change in rc.5, so nothing has to be packed together beyond the two titles saying the same version. Also added the second half of the decision, which I had left out: exemplars SHOULD be sent in the same request as the samples or histograms of their series, including when they travel in a Out of draft now. |
|
I changed base to #3085 so we can squash it all as a rc.5, hope that's ok |
|
Ups I think you need to manually rebase, sorry @thc1006 - do you mind doing this? |
There was a problem hiding this comment.
This would be updated once the proto is updated then?
There was a problem hiding this comment.
Let's update it at one go and prep a PR.
There was a problem hiding this comment.
Both done: the copy here in this PR, the original in prometheus/prometheus#19530.
krajorama
left a comment
There was a problem hiding this comment.
LGTM, except the question about the copied protobuf
Prometheus stores exemplars per series, not per sample. populateV2TimeSeries writes one output TimeSeries per queue item, and for an exemplar item it appends only an exemplar, so an exemplar can leave the sender in a TimeSeries of its own. The specification requires every TimeSeries to carry a sample or a histogram, so that shape is not covered. Describe it instead of forbidding it, and keep the expectation that the exemplars travel in the same request as the series they belong to. Related to prometheus/prometheus#17857 and prometheus/prometheus#16944. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
b96cd5f to
f5268d0
Compare
|
Rebased onto @krajorama good catch on the copied protobuf. The comment you pointed at turned out to be one of three saying the same thing: that one says a The originals live in One thing I would rather ask than guess. In prometheus/prometheus#17857 back in January you proposed two halves, "Add SHOULD for per sample, but allow per series by spec". This PR has the second one. The Last thing, unrelated to the diff: prometheus/prometheus#18014 has been open since February taking the other route, dropping exemplars that arrive without a matching sample. @AftAb-25 wrote it against the spec as it read then. If this lands, they deserve to hear that the direction moved. The bot has already marked it stale. |
populateV2TimeSeries builds one TimeSeries per queue item, so a tExemplar item produces a message holding labels and one exemplar and nothing else. The comments on TimeSeries say that a message specifies samples or histograms, and that its exemplars belong to the series' samples, so they do not describe what the sender does. The specification is being changed to match in prometheus/docs#3085. krajorama asked there whether the copy of this file inside the specification document would follow the real one, so the two change together. types.pb.go carries the same three comments and is updated with them. The embedded file descriptors are left alone because they do not encode comments, which two generator runs, one with the change and one without, confirmed. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
f5268d0 to
6a5c22f
Compare
populateV2TimeSeries builds one TimeSeries per queue item, so a tExemplar item produces a message holding labels and one exemplar and nothing else. The comments on TimeSeries say that a message specifies samples or histograms, and that its exemplars belong to the series' samples, so they do not describe what the sender does. The specification is being changed to match in prometheus/docs#3085. krajorama asked there whether the copy of this file inside the specification document would follow the real one, so the two change together. types.pb.go carries the same three comments and is updated with them. The embedded file descriptors are left alone because they do not encode comments, which two generator runs, one with the change and one without, confirmed. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
6a5c22f to
35ee174
Compare
populateV2TimeSeries builds one TimeSeries per queue item, so a tExemplar item produces a message holding labels and one exemplar and nothing else. The comments on TimeSeries say that a message specifies samples or histograms, and that its exemplars belong to the series' samples, so they do not describe what the sender does. The specification is being changed to match in prometheus/docs#3085. krajorama asked there whether the copy of this file inside the specification document would follow the real one, so the two change together. types.pb.go carries the same three comments and is updated with them. The embedded file descriptors are left alone because they do not encode comments, which two generator runs, one with the change and one without, confirmed. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
populateV2TimeSeries builds one TimeSeries per queue item, so a tExemplar item produces a message holding labels and one exemplar and nothing else. The comments on TimeSeries say that a message specifies samples or histograms, and that its exemplars belong to the series' samples, so they do not describe what the sender does. The specification is being changed to match in prometheus/docs#3085. krajorama asked there whether the copy of this file inside the specification document would follow the real one, so the two change together. types.pb.go carries the same three comments and is updated with them. The embedded file descriptors are left alone because they do not encode comments, which two generator runs, one with the change and one without, confirmed. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
35ee174 to
487086d
Compare
populateV2TimeSeries builds one TimeSeries per queue item, so a tExemplar item produces a message holding labels and one exemplar and nothing else. The comments on TimeSeries say that a message specifies samples or histograms, and that its exemplars belong to the series' samples, so they do not describe what the sender does. The specification is being changed to match in prometheus/docs#3085. krajorama asked there whether the copy of this file inside the specification document would follow the real one, so the two change together. types.pb.go carries the same three comments and is updated with them. The embedded file descriptors are left alone because they do not encode comments, which two generator runs, one with the change and one without, confirmed. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
populateV2TimeSeries builds one TimeSeries per queue item, so a tExemplar item produces a message holding labels and one exemplar and nothing else. The comments on TimeSeries say that a message specifies samples or histograms, and that its exemplars belong to the series' samples, so they do not describe what the sender does. The specification is being changed to match in prometheus/docs#3085. krajorama asked there whether the copy of this file inside the specification document would follow the real one, so the two change together. types.pb.go carries the same three comments and is updated with them. The embedded file descriptors are left alone because they do not encode comments, which two generator runs, one with the change and one without, confirmed. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
populateV2TimeSeries builds one TimeSeries per queue item, so a tExemplar item produces a message holding labels and one exemplar and nothing else. The comments on TimeSeries say that a message specifies samples or histograms, and that its exemplars belong to the series' samples, so they do not describe what the sender does. The specification is being changed to match in prometheus/docs#3085. krajorama asked there whether the copy of this file inside the specification document would follow the real one, so the two change together. types.pb.go carries the same three comments and is updated with them. The embedded file descriptors are left alone because they do not encode comments, which two generator runs, one with the change and one without, confirmed. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
populateV2TimeSeries builds one TimeSeries per queue item, so a tExemplar item produces a message holding labels and one exemplar and nothing else. The comments on TimeSeries say that a message specifies samples or histograms, and that its exemplars belong to the series' samples, so they do not describe what the sender does. The specification is being changed to match in prometheus/docs#3085. krajorama asked there whether the copy of this file inside the specification document would follow the real one, so the two change together. types.pb.go carries the same three comments and is updated with them. The embedded file descriptors are left alone because they do not encode comments, which two generator runs, one with the change and one without, confirmed. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
populateV2TimeSeries builds one TimeSeries per queue item, so a tExemplar item produces a message holding labels and one exemplar and nothing else. The comments on TimeSeries say that a message specifies samples or histograms, and that its exemplars belong to the series' samples, so they do not describe what the sender does. The specification is being changed to match in prometheus/docs#3085. krajorama asked there whether the copy of this file inside the specification document would follow the real one, so the two change together. types.pb.go carries the same three comments and is updated with them. The embedded file descriptors are left alone because they do not encode comments, which two generator runs, one with the change and one without, confirmed. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
populateV2TimeSeries builds one TimeSeries per queue item, so a tExemplar item produces a message holding labels and one exemplar and nothing else. The comments on TimeSeries say that a message specifies samples or histograms, and that its exemplars belong to the series' samples, so they do not describe what the sender does. The specification is being changed to match in prometheus/docs#3085. krajorama asked there whether the copy of this file inside the specification document would follow the real one, so the two change together. types.pb.go carries the same three comments and is updated with them. The embedded file descriptors are left alone because they do not encode comments, which two generator runs, one with the change and one without, confirmed. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
487086d to
46b865c
Compare
There was a problem hiding this comment.
General thought when I look a the changes: It feels this change is a step back when it comes to compatibility with systems oriented around OpenTelemetry model. As-in, you can use PRW2 to have a reliable transport against those, but it requires being strict on various elements beyond what protocol requires from sender e.g.:
- Avoid unknown metadata.
- Use NHCB instead of classic histograms.
Now with this change it also technically needs:
- Exemplar per sample (or at least within same request).
In the same time this relaxation of rules for exemplars allows (slightly) simpler protocol for existing Prometheus systems, and it's unlikely the exemplar part would change for those given the current APIs.
I wonder how to tackle this redundancy of implementing PRW2 around those intentions to battle ossification/proliferation and being too lean on sending so "I implement PRW2 sending" is not ambiguous.
Perhaps what we need is to introduce some "mode" or consider OTLP export again given advances we have in the storage.
The copy of io.prometheus.write.v2 in this document still says a TimeSeries specifies samples or histograms, and that exemplars belong to the series' samples. Neither holds once a TimeSeries can carry exemplars on its own. krajorama spotted this in review. The same three comments live in prompb/io/prometheus/write/v2/types.proto, which is the source of truth, so they are changed there too. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
46b865c to
8f9d1fa
Compare
populateV2TimeSeries builds one TimeSeries per queue item, so a tExemplar item produces a message holding labels and one exemplar and nothing else. The comments on TimeSeries say that a message specifies samples or histograms, and that its exemplars belong to the series' samples, so they do not describe what the sender does. The specification is being changed to match in prometheus/docs#3085. krajorama asked there whether the copy of this file inside the specification document would follow the real one, so the two change together. types.pb.go carries the same three comments and is updated with them. The embedded file descriptors are left alone because they do not encode comments, which two generator runs, one with the change and one without, confirmed. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
|
One piece of data first, since it may change the shape of the question. Prometheus already sends metadata in a message with no samples. The same holds for the OTel side. A receiver reading real Prometheus traffic already had to decide what to do with exemplar-only messages, and the specification told it they were invalid. This does not hand it a new case, it names one it already had. What the change does add is the SHOULD about the same request, and that one is mine to defend or drop. It is there so a receiver has a bounded window to associate within. If the cost is another implicit requirement on senders, dropping it leaves the MAY describing the wire format and nothing more. On modes and on OTLP export I do not have a view worth your time. What would help me is knowing whether you want this narrowed to the wire-format statement, held until that larger question settles, or something else. |
I understood it wrongly -- for exemplars sure, but why exemplars even have metadata then? 🙈 |
|
Thanks, that framing helps, and the Correcting something in what I wrote above: I leaned on What is left is narrower. Exemplars are enqueued separately from their samples whichever way metadata goes, so that part does not resolve with it. Happy to drop the SHOULD and leave this describing only the wire format, or to park it until the metadata direction settles. Your call. |
What this PR is about is that an exemplar-only TimeSeries is valid and that it identifies its series by labels_refs. Whether a sender keeps a series' exemplars in the same request as its samples is a different question. It is about interoperability rather than about what the wire format allows, and it is the part of this change that is under discussion in review. Taking it out leaves the wire format statement on its own. Co-location is worth writing down, just not in this rule. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
|
I took the same-request SHOULD out in 81c879a. What is left is only that an exemplar-only @bwplotka that was the same-request half of the third item on your list, and the only one of the three this PR was writing into the spec. Whether a sender keeps a series' exemplars with its samples is about interoperability, not about what the wire format allows, so it belongs with the mode or profile question rather than as a SHOULD in the base spec. @krajorama flagging this because of prometheus/prometheus#17857 (comment). I read it as asking to soften the per-sample MUST so the mode in that plan would not be a violation, rather than as asking for a new rule about batching. This PR makes the shape valid instead, so there is nothing left to violate. If I have read you wrong, say so and I will put the line back. Co-location is still worth writing down somewhere. Does this settle the protocol format part of your review? |
The at-least-one rule names samples, histograms and exemplars, so a TimeSeries carrying only metadata is already invalid. That is left to be inferred from a list rather than stated, which bwplotka flagged in review. State it in the rule. Then say what metadata means on the shape this PR adds, because an exemplar-only TimeSeries still carries the field and the Prometheus sender fills it. Calling it series-level and letting Receivers ignore it means a sender cannot rely on that shape to deliver metadata. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
bwplotka
left a comment
There was a problem hiding this comment.
Sorry for lag - let's clean this up!
Will you have time to address suggestions?
bwplotka's suggestions, applied as given. The at-least-one rule splits into two bullets and makes the metadata point an example rather than a second clause. The exemplar bullet loses the sentence about metadata on a sample-less TimeSeries. The two exemplar comments in the embedded protobuf now say the exemplars may belong to the series or to its samples and histograms. Those two comments are the ones prometheus/prometheus#19530 changes in the source proto, so they move together there, which is what krajorama asked for when this pair was opened. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
bwplotka asked for this wording on prometheus/docs#3085, where the copy of this file inside the specification document carries the same two comments. Applying it only there would leave the copy ahead of the source, which is the drift krajorama asked about when this pair was opened. types.pb.go carries both comments and is updated with them, the same way the first commit here did. The embedded file descriptors are untouched for the same reason: they do not encode comments. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
|
All four applied in f9578cf, as written. The two exemplar comments also live in One thing I left alone because it is your wording: the |
* rw2: bump version Signed-off-by: bwplotka <bwplotka@gmail.com> * rw2: Async Receiver Support (#3081) * rm2: Async spec support See https://docs.google.com/document/d/1AzEt6kOz4MJ855fBxtzvyyS-kDu8ynXiJy6VCSUQcSI/edit Signed-off-by: bwplotka <bwplotka@gmail.com> * Update docs/specs/prw/remote_write_spec_2_0.md Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com> * Update docs/specs/prw/remote_write_spec_2_0.md Co-authored-by: George Krajcsovits <krajorama@users.noreply.github.com> Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com> --------- Signed-off-by: bwplotka <bwplotka@gmail.com> Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com> Co-authored-by: George Krajcsovits <krajorama@users.noreply.github.com> * RW 2.0-rc.5: allow exemplar-only time series (#3085) * RW 2.0-rc.5: allow exemplar-only time series Prometheus stores exemplars per series, not per sample. populateV2TimeSeries writes one output TimeSeries per queue item, and for an exemplar item it appends only an exemplar, so an exemplar can leave the sender in a TimeSeries of its own. The specification requires every TimeSeries to carry a sample or a histogram, so that shape is not covered. Describe it instead of forbidding it, and keep the expectation that the exemplars travel in the same request as the series they belong to. Related to prometheus/prometheus#17857 and prometheus/prometheus#16944. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> * RW 2.0-rc.5: match the embedded protobuf comments The copy of io.prometheus.write.v2 in this document still says a TimeSeries specifies samples or histograms, and that exemplars belong to the series' samples. Neither holds once a TimeSeries can carry exemplars on its own. krajorama spotted this in review. The same three comments live in prompb/io/prometheus/write/v2/types.proto, which is the source of truth, so they are changed there too. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> * RW 2.0-rc.5: drop the same-request SHOULD What this PR is about is that an exemplar-only TimeSeries is valid and that it identifies its series by labels_refs. Whether a sender keeps a series' exemplars in the same request as its samples is a different question. It is about interoperability rather than about what the wire format allows, and it is the part of this change that is under discussion in review. Taking it out leaves the wire format statement on its own. Co-location is worth writing down, just not in this rule. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> * RW 2.0-rc.5: rule out metadata-only time series The at-least-one rule names samples, histograms and exemplars, so a TimeSeries carrying only metadata is already invalid. That is left to be inferred from a list rather than stated, which bwplotka flagged in review. State it in the rule. Then say what metadata means on the shape this PR adds, because an exemplar-only TimeSeries still carries the field and the Prometheus sender fills it. Calling it series-level and letting Receivers ignore it means a sender cannot rely on that shape to deliver metadata. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> * RW 2.0-rc.5: apply the review wording bwplotka's suggestions, applied as given. The at-least-one rule splits into two bullets and makes the metadata point an example rather than a second clause. The exemplar bullet loses the sentence about metadata on a sample-less TimeSeries. The two exemplar comments in the embedded protobuf now say the exemplars may belong to the series or to its samples and histograms. Those two comments are the ones prometheus/prometheus#19530 changes in the source proto, so they move together there, which is what krajorama asked for when this pair was opened. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --------- Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --------- Signed-off-by: bwplotka <bwplotka@gmail.com> Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com> Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> Co-authored-by: George Krajcsovits <krajorama@users.noreply.github.com> Co-authored-by: 秀吉 <84045975+thc1006@users.noreply.github.com>
This changes the 2.0 specification so that a
TimeSeriescarrying only exemplars is described rather than forbidden.Why the current wording does not cover what a sender produces
Prometheus stores exemplars per series, not per sample.
populateV2TimeSeriesinstorage/remote/queue_manager.gowrites one outputTimeSeriesper queue item, and for an exemplar item it appends only an exemplar, having already reset that entry's samples and histograms. So an exemplar can leave the sender in aTimeSeriesof its own.The specification currently says:
That shape therefore has no wording behind it, and a receiver has nothing to rely on when it arrives.
What this changes
Two normative sentences, plus the proto copy this document carries:
exemplarsas well, and says what aTimeSeriescarrying only exemplars means;#### Exemplarssection says the same thing from the exemplar's side;No field, field number or wire layout changes, and nothing is asked of a sender that was not asked before.
What the first version also had
It added a SHOULD that exemplars travel in the same request as the samples or histograms of their series. That is a rule about how a sender batches rather than a statement about what the wire format allows, and it is the part of this change review is stuck on, so it is out as of 81c879a.
Co-location is still worth writing down somewhere.
QueueManager.AppendandAppendExemplarsboth enqueue on the series ref, andshards.enqueuepicks the shard withref % len(queues), so a series' samples and its exemplars go into the same shard queue in order. They are usually in one request and a full batch is what splits them. That belongs wherever the receiver profile question lands, not in this rule.What this leaves alone
The receiver tests in
prometheus/complianceonly cover exemplars sent alongside a sample. Covering aTimeSeriesthat carries only exemplars is a separate change. The canonical proto comments are in prometheus/prometheus#19530, which carries the source version bump to rc.5 as well.On versioning
Retitled with the version, the way rc.4 was done in #2762, so the title is the record of what went into the release. #3080 still carries the single bump and #3081 is the other change in this one.
Related to prometheus/prometheus#17857 and prometheus/prometheus#16944.