Make default search limit configurable #1022
Conversation
kvrhdn
requested review from
annanay25,
dgzlopes,
joe-elliott,
KMiller-Grafana,
mapno and
mdisibio
as code owners
modules/ingester/instance_search.go
Outdated
| maxResults := int(req.Limit) | ||
| // limit should always be set, if it's missing set a safe default | ||
| if maxResults == 0 { | ||
| maxResults = 100 |
There was a problem hiding this comment.
This line should never be reached correct? It's the main purpose of the PR
There was a problem hiding this comment.
Yeah, this line should never be reached. I wanted to add it as an extra safety check, just in case someone sends a query directly to the ingester. But this should be pretty much impossible as it's pretty hard to send a grpc request.
Protecting against 0 also doesn't make a lot of sense and it can never be negative 😅
There was a problem hiding this comment.
I opted to re-add the check again as req.Limit == 0 is most likely an indication that the caller didn't bother to set the limit. It's unlikely they explicitly wanted a limit of 0. If the limit is unset I think we should simply use a safe default limit.
What this PR does:
Adds a configuration parameter
querier.search_default_limitwhich allows you to set a server-side default.Which issue(s) this PR fixes:
Part of #932
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]