pkg/ingester: handle labels mapping to the same fast fingerprint. #1247
Conversation
Uses slightly adapted fpMapper code from Cortex. Fixes issue #898 Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
pstibrany
force-pushed
the
issue-898-out-of-order
branch
from
b00e33d
to
b3d13c7
Compare
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
There was a problem hiding this comment.
This looks awesome, can you quickly verify race manually with go -test race on the new test once you have added a goroutine. I just want to make sure we don't add new races, we don't have anything to verify and this part of the code is pretty much entitled to races.
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
To be run with -race. Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
|
I've added slightly bigger test for concurrent pushes. Perhaps it can be used as basis for benchmark. I think that global (per-user) |
|
@sandlis pointed out issue with this code... it may pass different fingerprint from different ingesters to the chunk storage code. |
Mapped fingerprint is used in streams map.
Original ("rawFP") is preserved when saving chunks.
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
|
I've addressed problem where mapping series to a different fingerprint would also be reflected in chunk name, which is not what we want. Fixed now. Thanks @sandlis for spotting this! |
|
I now have my doubts about whether using original fingerprint for chunk ID is a good idea. By deduping chunks on original fingerprints, we may accidentally drop valid data that way, as we already know that there was fingerprint collision. I cannot find the code doing this deduping in Loki though. In Cortex, it's here: https://github.com/cortexproject/cortex/blob/77a09cc7c953ce5bc4938f042ccb588503ce8eb5/pkg/chunk/chunk_store_utils.go#L55 and with my extra fix, it would definitely drop chunk from different series than expected. |
Preserving raw fingerprints could lead to data loss when doing chunk deduplication while storing chunks. We don't want that. Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
In the |
| for _, label := range chunk.Metric { | ||
| if label.Value == "" { | ||
| return fmt.Errorf("Chunk has blank label %q", label.Name) | ||
| } | ||
| } | ||
|
|
||
| // remove __name__ label | ||
| if chunk.Metric.Has("__name__") { |
There was a problem hiding this comment.
you could use labels.MetricName (https://github.com/prometheus/prometheus/blob/master/pkg/labels/labels.go#L30)
What this PR does / why we need it:
This PR fixes problem in ingester with labels mapped to the same fast fingerprint, which then causes out of order errors.
Which issue(s) this PR fixes:
Fixes #898
Special notes for your reviewer:
Uses slightly adapted fpMapper code from Cortex.
Instead of using
[]client.LabelAdapter, this PR changes it tolabels.Labelsin ingester stream. It also avoids conversion viaclient.FromLabelAdaptersToLabels(which breaks contract oflabels.Labelsbecause it doesn't check for input being sorted. cortexproject/cortex#1813) Note that fpMapper code relies onlabels.Labelsbeing sorted in the equality check.This PR doesn't change similar problem present in promtail Client.
Checklist