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

Loki: only log "executing query" once per query in the frontend #8337

Merged
merged 3 commits into from
Jan 30, 2023

Conversation

slim-bean
Copy link
Collaborator

Signed-off-by: Edward Welch [email protected]

What this PR does / why we need it:

This is a continuation of #7469

I had to add a way to disable logging of the "executing query" line from the Engine in the query frontend because this is called for every split by time and shard created by our roundtrippers leading to many "executing query" lines for a single query which is not desired.

Added the "executing query" line to the roundtripper and it now covers: range, instant, series, and labels queries.

It should now also only log one line per query making it possible to correlate this to the "metrics.go" line for a finished query to help debug queries which don't finish because of OOM crash.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Checklist

  • Reviewed the CONTRIBUTING.md guide (required)
  • Documentation added
  • Tests updated
  • CHANGELOG.md updated
  • Changes that require user attention or interaction to upgrade are documented in docs/sources/upgrading/_index.md

@slim-bean slim-bean requested a review from a team as a code owner January 30, 2023 15:32
… the frontend and instead log from the roundtripper so we only get one executing log line per query.

Signed-off-by: Edward Welch <[email protected]>
@slim-bean slim-bean force-pushed the fix-executing-query branch from d808580 to 4471335 Compare January 30, 2023 15:33
Copy link
Contributor

@dannykopping dannykopping left a comment

Choose a reason for hiding this comment

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

LGTM! One tiny nit

pkg/logql/engine.go Outdated Show resolved Hide resolved
slim-bean and others added 2 commits January 30, 2023 11:03
Co-authored-by: Danny Kopping <[email protected]>
Signed-off-by: Edward Welch <[email protected]>
@grafanabot
Copy link
Collaborator

./tools/diff_coverage.sh ../loki-target-branch/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

@slim-bean slim-bean merged commit 35510ba into main Jan 30, 2023
@slim-bean slim-bean deleted the fix-executing-query branch January 30, 2023 16:24
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.

3 participants