Fixes store duplicate label names and values. #1790
Conversation
cyriltovena
changed the title
|
see grafana/loki#1220. |
cyriltovena
force-pushed
the
dup-label
branch
from
32485df to
ec357b4
Compare
Have you read the PR description ? The composite store needs to sort and make labels or values unique. |
There was a problem hiding this comment.
Good job @cyriltovena. To my understanding of the problem, changes look good to me. I left a couple of last minor comments.
cyriltovena
force-pushed
the
dup-label
branch
from
ec357b4 to
a0b412f
Compare
cyriltovena
force-pushed
the
dup-label
branch
from
a0b412f to
e9676ca
Compare
pkg/chunk/chunk_store.go
Outdated
| for _, entry := range entries { | ||
| _, labelValue, _, _, err := parseChunkTimeRangeValue(entry.RangeValue, entry.Value) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| result = append(result, string(labelValue)) | ||
| result = appendUniqueStrings(result, values, string(labelValue)) |
There was a problem hiding this comment.
Since this change is (now) unrelated to the bugfix, I would prefer it goes in a different commit with its own explanation.
You may want to make the same change in parseIndexEntries(), which would let you remove uniqueStrings().
It's worth checking the benchmark numbers and/or making sure the benchmarks are using a realistic data set.
There was a problem hiding this comment.
good catch ! I'll remove it.
There was a problem hiding this comment.
before :
pkg: github.com/cortexproject/cortex/pkg/chunk
BenchmarkParseIndexEntries500-16 4441 259747 ns/op 96427 B/op 1503 allocs/op
BenchmarkParseIndexEntries2500-16 891 1363771 ns/op 482397 B/op 7503 allocs/op
BenchmarkParseIndexEntries10000-16 206 5654314 ns/op 1928897 B/op 30005 allocs/op
BenchmarkParseIndexEntries50000-16 39 30095557 ns/op 9607707 B/op 150006 allocs/op
PASS
ok github.com/cortexproject/cortex/pkg/chunk 8.054s
after:
pkg: github.com/cortexproject/cortex/pkg/chunk
BenchmarkParseIndexEntries500-16 3678 318019 ns/op 130810 B/op 1522 allocs/op
BenchmarkParseIndexEntries2500-16 747 1632040 ns/op 612466 B/op 7563 allocs/op
BenchmarkParseIndexEntries10000-16 172 6779048 ns/op 2441477 B/op 30219 allocs/op
BenchmarkParseIndexEntries50000-16 31 36258715 ns/op 11602340 B/op 151867 allocs/op
PASS
ok github.com/cortexproject/cortex/pkg/chunk 8.108s
There was a problem hiding this comment.
Adding half duplicate in the mix:
before:
pkg: github.com/cortexproject/cortex/pkg/chunk
BenchmarkParseIndexEntries500-16 4485 262828 ns/op 96346 B/op 1503 allocs/op
BenchmarkParseIndexEntries2500-16 838 1379892 ns/op 482316 B/op 7503 allocs/op
BenchmarkParseIndexEntries10000-16 201 5907672 ns/op 1928821 B/op 30005 allocs/op
BenchmarkParseIndexEntries50000-16 39 30373427 ns/op 9607658 B/op 150007 allocs/op
PASS
ok github.com/cortexproject/cortex/pkg/chunk 8.056s
after:
pkg: github.com/cortexproject/cortex/pkg/chunk
BenchmarkParseIndexEntries500-16 4454 265512 ns/op 109322 B/op 1514 allocs/op
BenchmarkParseIndexEntries2500-16 878 1356005 ns/op 526739 B/op 7536 allocs/op
BenchmarkParseIndexEntries10000-16 211 5586487 ns/op 2107155 B/op 30116 allocs/op
BenchmarkParseIndexEntries50000-16 37 29155700 ns/op 10205072 B/op 150868 allocs/op
PASS
ok github.com/cortexproject/cortex/pkg/chunk 7.917s
There was a problem hiding this comment.
So it looks like if there is duplicates the new append with the map unique values is faster, no surprise here. However if there is no duplicates the old version is faster.
Memory wise I think the increase is negligible. (25B per op).
I will submit the benchmark now.
There was a problem hiding this comment.
After some discussions with Goutham, let's not take any risk and leave the parseIndexEntries as is. Other functions (Fetching labels) are not used by Cortex and should be fine to change.
There was a problem hiding this comment.
done ! I think we should be good to go here.
cyriltovena
force-pushed
the
dup-label
branch
from
e9676ca to
96726b1
Compare
There was a problem hiding this comment.
Thanks @cyriltovena for addressing my feeback. Changes LGTM!
cyriltovena
force-pushed
the
dup-label
branch
from
b50d2cd to
a42d80c
Compare
Signed-off-by: Cyril Tovena <[email protected]>
Signed-off-by: Cyril Tovena <[email protected]>
Signed-off-by: Cyril Tovena <[email protected]>
cyriltovena
force-pushed
the
dup-label
branch
from
cc0f617 to
fe55572
Compare
When we query label names or values across multiple stores from different schema period we can lose the unicity or ordering.
This changes sort and filter label in the composite store. I've also added some test to verify the output.