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.

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

Conversation

matej-g
Copy link
Contributor

@matej-g matej-g commented Jan 25, 2022

Signed-off-by: Matej Gera [email protected]

What this PR does:
Fixes the behavior of Query Frontend when it comes to logging slow queries - if the parameter is set to < 0, all queries should be logged, as is described in the documentation. However, presently, the opposite is true - if set to < 0, logging is disabled altogether.

Leaving the parameter at 0 should still retaing the same behavior, i.e. disabling slow query logging.

Which issue(s) this PR fixes:
Fixes #4610

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

Signed-off-by: Matej Gera <[email protected]>
Copy link
Contributor

@bboreham bboreham left a comment

Choose a reason for hiding this comment

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

OK to merge, but I had one comment on the test.

Comment on lines 114 to 115
// 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!

@alanprot alanprot merged commit 86d5f52 into cortexproject:master Jan 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Frontend: Parameter 'LogQueriesLongerThan' does not behave as described in the docs
3 participants