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

Add split_queries to query stats #2641

Merged
merged 4 commits into from
Aug 4, 2022
Merged

Conversation

ssncferreira
Copy link
Contributor

@ssncferreira ssncferreira commented Aug 3, 2022

What this PR does

Add split_queries parameter describing the number of partial queries executed in parallel to both range and instant queries.

Which issue(s) this PR fixes or relates to

Fixes #

Checklist

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

@ssncferreira ssncferreira marked this pull request as ready for review August 3, 2022 11:16
Copy link
Collaborator

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

Thanks! Overall LGTM, but look at my comment, please.

Can we also add an assertion to 1 on the existing tests to check it's actually added to the query stats?

pkg/frontend/querymiddleware/split_and_cache.go Outdated Show resolved Hide resolved
@ssncferreira
Copy link
Contributor Author

Can we also add an assertion to 1 on the existing tests to check it's actually added to the query stats?

@pracucci added tests for stats: 34835d5

However, I don't see any tests that assert the written log line. Let me know if I'm missing something, otherwise I can create a unit test to assert this 🙂

@ssncferreira
Copy link
Contributor Author

Noticed that the ruler also logs query's stats: https://github.com/grafana/mimir/blob/main/pkg/ruler/compat.go#L195

Since the split by range feature also affects the ruler, maybe it is also worthwhile adding it here 🤔

@pracucci
Copy link
Collaborator

pracucci commented Aug 4, 2022

Noticed that the ruler also logs query's stats: https://github.com/grafana/mimir/blob/main/pkg/ruler/compat.go#L195

Since the split by range feature also affects the ruler, maybe it is also worthwhile adding it here 🤔

The ruler is not affected by the splitting. Splitting is done only by query-frontend. The ruler has two evaluations modes:

  • Internal: no splitting, requests don't go through query-frontend
  • Remote: requests go though query-frontend, there's splitting supported but provided by the query-frontend so it will be logged by the query-frontend middleware

@pracucci
Copy link
Collaborator

pracucci commented Aug 4, 2022

Can we also add an assertion to 1 on the existing tests to check it's actually added to the query stats?

@pracucci added tests for stats: 34835d5

However, I don't see any tests that assert the written log line. Let me know if I'm missing something, otherwise I can create a unit test to assert this 🙂

I actually meant to check the stats from the context, not the log line. It's not a blocker to get this PR merged tho.

@ssncferreira
Copy link
Contributor Author

Noticed that the ruler also logs query's stats: https://github.com/grafana/mimir/blob/main/pkg/ruler/compat.go#L195
Since the split by range feature also affects the ruler, maybe it is also worthwhile adding it here 🤔

The ruler is not affected by the splitting. Splitting is done only by query-frontend. The ruler has two evaluations modes:

  • Internal: no splitting, requests don't go through query-frontend
  • Remote: requests go though query-frontend, there's splitting supported but provided by the query-frontend so it will be logged by the query-frontend middleware

Ah, yes I meant the ruler will receive the split requests from the query-frontend (remote point). But already understood that that will be logged in the query-frontend. Thanks for the explanation 🙂

@pracucci pracucci merged commit e9f80b0 into main Aug 4, 2022
@pracucci pracucci deleted the query_stats_add_split_queries branch August 4, 2022 12:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants