Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Query Frontend: Fix slow query logging if 'LogQueriesLongerThan' is set to < 0 #4633

Merged
merged 3 commits into from
Jan 31, 2022
Merged

Query Frontend: Fix slow query logging if 'LogQueriesLongerThan' is set to < 0 #4633

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
09383bf
Fix slow query log parameter if < 0
matej-g Jan 25, 2022
92ab793
Update CHANGELOG
matej-g Jan 25, 2022
0058e32
Extend the query frontend tests
matej-g Jan 25, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
* [BUGFIX] Set appropriate `Content-Type` header for /services endpoint, which previously hard-coded `text/plain`. #4596
* [BUGFIX] Querier: Disable query scheduler SRV DNS lookup, which removes noisy log messages about "failed DNS SRV record lookup". #4601
* [BUGFIX] Memberlist: fixed corrupted packets when sending compound messages with more than 255 messages or messages bigger than 64KB. #4601
* [BUGIX] Query Frontend: If 'LogQueriesLongerThan' is set to < 0, log all queries as described in the docs. #4633

## 1.11.0 2021-11-25

Expand Down
3 changes: 2 additions & 1 deletion pkg/frontend/frontend_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,8 @@ func TestFrontend_LogsSlowQueriesFormValues(t *testing.T) {

// Configure the query-frontend with the mocked downstream server.
config := defaultFrontendConfig()
config.Handler.LogQueriesLongerThan = 1 * time.Microsecond
// Setting to < 0 to ensure all queries are logged.
config.Handler.LogQueriesLongerThan = -1 * time.Microsecond
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was a bit surprised that this is changing what the test does, rather than testing the old and new behaviour.
However I think the test should have had both a slow and a fast query to test the old one properly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought the reason this was set so low was to in fact log every query for the purpose of this test. But I agree to have both cases tested separately would be nicer, I'll add it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added the test case, let me know what you think!

config.DownstreamURL = fmt.Sprintf("http://%s", downstreamListen.Addr())

var buf concurrency.SyncBuffer
Expand Down
2 changes: 1 addition & 1 deletion pkg/frontend/transport/handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ func (f *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
_, _ = io.Copy(w, resp.Body)

// Check whether we should parse the query string.
shouldReportSlowQuery := f.cfg.LogQueriesLongerThan > 0 && queryResponseTime > f.cfg.LogQueriesLongerThan
shouldReportSlowQuery := f.cfg.LogQueriesLongerThan != 0 && queryResponseTime > f.cfg.LogQueriesLongerThan
if shouldReportSlowQuery || f.cfg.QueryStatsEnabled {
queryString = f.parseRequestQueryString(r, buf)
}
Expand Down