Loki: Series API will return all series with no match or empty matcher #2254
Conversation
…return all series for the provided timeframe.
| dedupedSeries[key] = logproto.SeriesIdentifier{ | ||
| // If no matchers were supplied we include all streams. | ||
| if len(groups) == 0 { | ||
| series = make([]logproto.SeriesIdentifier, 0, len(i.streams)) |
There was a problem hiding this comment.
This allocation isn't protected by a mutex so it could be wrong but I think this would generally be ok, worst case a stream is added between the creation of this array and it's population causing Go to have to double its size. I'm thinking this is ok but am open to alternatives or different opinions.
There was a problem hiding this comment.
No problem here from my point of view. But if you're not sure you can write a test that does 2 query in different goroutine and run that test using -race.
go test -mod=vendor -race ./pkg/ingester.
There was a problem hiding this comment.
I think you could have kept the same function func(stream *stream) error for both cases.`
| from, through = util.RoundToMilliseconds(req.Start, req.End) | ||
| nameLabelMatcher, err := labels.NewMatcher(labels.MatchEqual, labels.MetricName, "logs") | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| matchers = []*labels.Matcher{nameLabelMatcher} |
There was a problem hiding this comment.
One concern I have here is this shortcut also skips the shard aware stuff in decodeReq, currently the series API is not shard aware so this is fine but I'm not sure if there is something else we could do here to protect future us from missing this. See #2167
There was a problem hiding this comment.
You would need to get the part that parses the string matcher out and pass the matchers as an array to decodeReq.
|
Should we not enforce some limits like |
This is a good thought @adityacs, I went through the code as written today and I think we are reasonably well protected from OOM, we download the chunks in batches for large ranges, and are only ever downloading one chunk per series. I think the only possibility of OOM would come from the actual result being built of the series labels, but it would have to be a pretty massive result for this to happen and I think is pretty unlikely. |
Codecov Report
@@ Coverage Diff @@
## master #2254 +/- ##
==========================================
- Coverage 62.32% 62.32% -0.01%
==========================================
Files 158 158
Lines 12718 12762 +44
==========================================
+ Hits 7927 7954 +27
- Misses 4183 4195 +12
- Partials 608 613 +5
|
|
@slim-bean Yeah, makes sense |
|
@slim-bean loki2.6.1 or loki 2.7.1 Series API will return all series with no match or empty matcher #8034 |
The series API can be an important tool for inspecting Loki usage and determining which series may be contributing to high cardinality problems.
In analytic uses cases such as this it's necessary to be able to return all series within a timeframe without supplying a matcher (or supply an empty matcher {})