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>
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>
35ee174 to
487086d
Compare
This changes the 2.0 specification so that a
TimeSeriescarrying only exemplars is described rather than forbidden, and keeps the expectation that those exemplars travel with the series they belong to.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
Three sentences, one file:
exemplarsas well, and says what aTimeSeriescarrying only exemplars means;#### Exemplarssection says the same thing from the exemplar's side;That last one matters because a sender does usually keep them together.
QueueManager.AppendandAppendExemplarsare called separately by the WAL watcher, so the batch can be cut between a sample and its exemplar, but that is the exception rather than the intent. SHOULD says exactly that.No field, field number or wire layout changes, and senders are not asked to do anything differently.
What this leaves alone
The proto comments in
prompb/io/prometheus/write/v2/types.protostill say exemplars are "attached to this series' samples", and the receiver tests inprometheus/complianceonly cover exemplars sent alongside a sample. Both would want the same treatment, but they are separate changes.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.