Log when returning query-time limit #8938
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
salvacorts
commented
Mar 29, 2023
| return original | ||
| } | ||
| level.Debug(logutil.WithContext(ctx, l.logger)).Log("msg", "using request limit", "limit", requestLimits.MaxQueryLength) | ||
| level.Debug(logutil.WithContext(ctx, l.logger)).Log("msg", "using request limit", "limit", "MaxQueryLength", "tenant", userID, "query-limit", requestLimits.MaxQueryLength, "original-limit", original) |
There was a problem hiding this comment.
Do we need to print the limit name? I'd say we don't since the logger will print the line where we are calling level.Debug.
There was a problem hiding this comment.
I think it's easier if the limit name is printed as well.
salvacorts
changed the title
jeschkies
approved these changes
Mar 29, 2023
| return original | ||
| } | ||
| level.Debug(logutil.WithContext(ctx, l.logger)).Log("msg", "using request limit", "limit", requestLimits.MaxQueryLength) | ||
| level.Debug(logutil.WithContext(ctx, l.logger)).Log("msg", "using request limit", "limit", "MaxQueryLength", "tenant", userID, "query-limit", requestLimits.MaxQueryLength, "original-limit", original) |
There was a problem hiding this comment.
I think it's easier if the limit name is printed as well.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
At #8727 we introduced various limits that can now be configured at query time. We always compare the value of the limit configured at query time with the value set on the overrides for the tenant or the default if not configured (aka original); applying the most restrictive one.
If the most restrictive is the original value or the limit is not configured at query-time, we print the following debug message:
loki/pkg/util/querylimits/limiter.go
Line 43 in 9e1846c
It will be printed many times: every time the query is not configured at query-time, as well as every time the original value is more restrictive. Moreover, this log message lacks some useful information such as the original value, the query-time value, the tenant ID and the limit name.
This PR fixes this by removing the debug message above and printing a new debug message only if we return the query-time limit. This new log also prints the tenant ID, the limit name, as well as the original and the query-time limit values.
Which issue(s) this PR fixes:
Fixes #8932
Special notes for your reviewer:
Checklist
CONTRIBUTING.mdguide (required)CHANGELOG.mdupdateddocs/sources/upgrading/_index.md