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.
| @@ -131,9 +143,39 @@ func (c *prometheusConverterV2) addSample(sample *writev2.Sample, lbls []prompb. | |||
| off = c.symbolTable.Symbolize(l.Value) | |||
| buf = append(buf, off) | |||
| } | |||
| 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.
There was a problem hiding this comment.
Yep, I added it here because we are triggering a flaky test. I added more details here. I was trying to debug it with @dashpole before his leaving. This is a TODO task to me figure out how to fix🙃
| @@ -109,10 +112,19 @@ func (c *prometheusConverterV2) fromMetrics(md pmetric.Metrics, settings Setting | |||
|
|
|||
| // timeSeries returns a slice of the writev2.TimeSeries that were converted from OTel format. | |||
| func (c *prometheusConverterV2) timeSeries() []writev2.TimeSeries { | |||
| allTS := make([]writev2.TimeSeries, 0, len(c.unique)) | |||
| conflicts := 0 | |||
There was a problem hiding this comment.
nit: would be a bit more efficient to keep the number of conflicts as a running tally in prometheusConverterV2
There was a problem hiding this comment.
I'm also updated the comment over the conflict map, because I hadn't thought that we can handle with hash conflits.. Like you share with my on slack
| require.Len(t, converter.unique, 1) | ||
| require.Len(t, converter.unique[timeSeriesSignature(labels)].Samples, 2) | ||
| }) | ||
| // TODO: Test 3 Conflict - different metrics with same hash |
There was a problem hiding this comment.
I think that the current set of tests were a little bit poor. Struggling to implement this one... Maybe these that I added could be a good start point. Let me know what you think.
Description
Handle conflicts in prw v2
Link to tracking issue
Partially fixes #33661