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

Jump to bottom

feat: move detected field logic to query frontend #14212

Merged
merged 7 commits into from
Sep 24, 2024
Merged

feat: move detected field logic to query frontend #14212

merged 7 commits into from
Sep 24, 2024

Conversation

trevorwhitney
Copy link
Collaborator

@trevorwhitney trevorwhitney commented Sep 20, 2024 •
edited
Loading

What this PR does / why we need it:

/detected_fields queries in ops are failing over bigger time ranges in Explore Logs, whereas logs queries for the same time period are not. I think this is because of all the optimizations/protections we have in place for logs queries (result cache, sharding, limits, etc.) that have not been implemented for /detected_fields. However, since /detected_fields is just doing a SelectLogs under the hood in the querier, if we move the detected fields logic up to the Query Frontend we can use the existing limited and filter tripperwares to get the underlying logs. Due to the limits already in place in those tripperwares, they will never return too many lines for the detected fields logic to reasonably handle in the Query Frontend, which allows us to move this logic there without fear of knocking over QFs.

This replaces #14210 as it works much better. I have tested in in Ops and it solves the problem without overloading QFs.

Special notes for your reviewer:

Checklist

  • Reviewed the CONTRIBUTING.md guide (required)
  • Documentation added
  • Tests updated
  • Title matches the required conventional commits format, see here
    • Note that Promtail is considered to be feature complete, and future development for logs collection will be in Grafana Alloy. As such, feat PRs are unlikely to be accepted unless a case can be made for the feature actually being a bug fix to existing behavior.
  • Changes that require user attention or interaction to upgrade are documented in docs/sources/setup/upgrade/_index.md
  • For Helm chart changes bump the Helm chart version in production/helm/loki/Chart.yaml and update production/helm/loki/CHANGELOG.md and production/helm/loki/README.md. Example PR
  • If the change is deprecating or removing a configuration option, update the deprecated-config.yaml and deleted-config.yaml files respectively in the tools/deprecated-config-checker directory. Example PR

@trevorwhitney
feat: move detected field logic to query frontend
88e006d 
Squashed commit of the following:

commit 4d9af56
Author: Trevor Whitney <trevorjwhitney@gmail.com>
Date:   Fri Sep 20 16:38:03 2024 -0600

    feat: move detected fields logic to QF

commit a23ea8b
Author: Trevor Whitney <trevorjwhitney@gmail.com>
Date:   Fri Sep 20 14:37:35 2024 -0600

    fix: enforce query timeouts and limits

commit 2ef80ec
Author: Trevor Whitney <trevorjwhitney@gmail.com>
Date:   Fri Sep 20 13:24:35 2024 -0600

    chore: no retries for detected_fields

commit c7a471d
Author: Trevor Whitney <trevorjwhitney@gmail.com>
Date:   Fri Sep 20 13:05:33 2024 -0600

    feat: return empty result instead of error for failed detected fields queries

commit 7b3529a
Author: Trevor Whitney <trevorjwhitney@gmail.com>
Date:   Fri Sep 20 09:39:33 2024 -0600

    fix: guard against failed ingester requests for detected fields

    (cherry picked from commit fbf9018)
@trevorwhitney trevorwhitney mentioned this pull request Sep 20, 2024
Closed
7 tasks
trevorwhitney
trevorwhitney commented Sep 23, 2024
View reviewed changes
pkg/querier/querier.go
@@ -1134,33 +1135,6 @@ func (q *SingleTenantQuerier) DetectedFields(ctx context.Context, req *logproto.
}, nil
}

func getParsersFromExpr(expr syntax.LogSelectorExpr) []string {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this wasn't used by anything

@trevorwhitney trevorwhitney marked this pull request as ready for review September 23, 2024 23:33
@trevorwhitney trevorwhitney requested a review from a team as a code owner September 23, 2024 23:33
@shantanualsi
Copy link
Contributor

this is great! have a few comments.. thank you!

svennergr
svennergr approved these changes Sep 24, 2024
View reviewed changes
Copy link
Contributor

@svennergr svennergr left a comment

Choose a reason for hiding this comment

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

LGTM

pkg/querier/queryrange/detected_fields.go Show resolved Hide resolved
@loki-gh-app
Copy link
Contributor

loki-gh-app bot commented Sep 24, 2024

This PR must be merged before a backport PR will be created.

2 similar comments
@loki-gh-app
Copy link
Contributor

loki-gh-app bot commented Sep 24, 2024

This PR must be merged before a backport PR will be created.

@loki-gh-app
Copy link
Contributor

loki-gh-app bot commented Sep 24, 2024

This PR must be merged before a backport PR will be created.

@trevorwhitney trevorwhitney merged commit 36ace66 into main Sep 24, 2024
68 checks passed
@trevorwhitney trevorwhitney deleted the move-detected-fields-to-qf branch September 24, 2024 17:42
loki-gh-app bot pushed a commit that referenced this pull request Sep 24, 2024
@trevorwhitney @grafana-delivery-bot
feat: move detected field logic to query frontend (#14212)
071b04a 
(cherry picked from commit 36ace66)
@loki-gh-app loki-gh-app bot mentioned this pull request Sep 24, 2024
Merged
7 tasks
loki-gh-app bot pushed a commit that referenced this pull request Sep 24, 2024
@trevorwhitney @grafana-delivery-bot
feat: move detected field logic to query frontend (#14212)
1779f9e 
(cherry picked from commit 36ace66)
@loki-gh-app loki-gh-app bot mentioned this pull request Sep 24, 2024
Merged
7 tasks
@loki-gh-app loki-gh-app bot mentioned this pull request Sep 30, 2024
Open
jeschkies pushed a commit to jeschkies/loki that referenced this pull request Oct 1, 2024
@trevorwhitney @jeschkies
feat: move detected field logic to query frontend (grafana#14212)
d5724b8
@loki-gh-app loki-gh-app bot mentioned this pull request Oct 7, 2024
Closed
This was referenced Oct 14, 2024
Closed
Closed
This was referenced Oct 21, 2024
Closed
Closed
@loki-gh-app loki-gh-app bot mentioned this pull request Nov 4, 2024
Merged
@loki-gh-app loki-gh-app bot mentioned this pull request Nov 11, 2024
Closed
@loki-gh-app loki-gh-app bot mentioned this pull request Dec 20, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shantanualsi shantanualsi shantanualsi left review comments

@svennergr svennergr svennergr approved these changes

Assignees
No one assigned
Labels
backport k220 backport k221 product-approved size/XXL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@trevorwhitney @shantanualsi @svennergr