handle conflits in prw v2 #39570
handle conflits in prw v2 #39570
Conversation
| if len(ts1.LabelsRefs) != len(ts2.LabelsRefs) { | ||
| return false | ||
| } | ||
| // As the labels are sorted as name, value, name, value, ... we can compare the labels by index jumping 2 steps at a time |
There was a problem hiding this comment.
plz update the comment on (new) line 134:
// TODO: Read the PRW spec to see if labels need to be sorted. If it is, then we need to sort in export code. If not, we can sort in the test. (@dashpole have more context on this)
since we're going to depend on this, it should say something like "We need to sort labels for..."
There was a problem hiding this comment.
Sorry, I don't know if I got it. Here we are comparing the LabelsRefs from different TSs, that are a list of integer arranged to represent: name, value, name, value....
The sort that we are doing on L132-L135 is related to sort the []prompb.Label. The confusion here was made because I used the sort word?
There was a problem hiding this comment.
Yes, the sort word is a bit ambiguous here, let's say
| // As the labels are sorted as name, value, name, value, ... we can compare the labels by index jumping 2 steps at a time | |
| // As the labels are ordered as name, value, name, value, ... we can compare the labels by index jumping 2 steps at a time |
But anyway, isSameMetricV2 will only work correctly if the order of labels is consistent, since otherwise it will say "not the same" for {a="1", b="2"} vs {b="2", a="1"} label sets.
On lines 132-135 we're sorting the labels []prompb.Label , but then we're converting these labels into the references on lines 137-143. So the order of the references is the same as the order of the sorted labels. Which means that we ensure consistent order for isSameMetricV2 with the sort on 132-135.
I did take a look at where the labels come from and it's createAttributes which will return consistent ordering probably - config changes not withstanding. So we could probably get away without making the sort, but I feel like that's brittle.
| } | ||
| ts := writev2.TimeSeries{ | ||
|
|
||
| sig := timeSeriesSignature(lbls) |
There was a problem hiding this comment.
For a future PR: timeSeriesSignature also does a sort by label name.
Description
Handle conflicts in prw v2
Link to tracking issue
Partially fixes #33661