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

Fix for ruler not honoring user error with local querier #7567

Merged
merged 4 commits into from
Mar 11, 2024

Conversation

krajorama
Copy link
Contributor

@krajorama krajorama commented Mar 7, 2024

What this PR does

Added new test case in TestRulerMetricsForInvalidQueriesAndNoFetchedSeries
with only the invalid query scenario where the user has entered an
invalid regex, but without selecting all chunks so the chunks limit
doesn't apply.

Since only one error happens, the error is of type errors.errorString
and not fmt.wrapError, which means the code to detect user errors and
cannot type assert to type QueryableError which in turn means it's treated
as a GRPC error, which it is not.
See

origErr := qerr.Unwrap()

Fix is restricted to local querier execution when GRPC errors are not exposed directly to the ruler.
Fix considers unwrapped errors as panics coming from the PromQL engine, most likely due to user error. Although it's hard to know exactly what errors the engine can return now and in the future. Would be nice if Prometheus had set rules on the types of error interfaces that it returns.

Which issue(s) this PR fixes or relates to

Related to CI run of #7558
https://github.com/grafana/mimir/actions/runs/8189224340/job/22393779459?pr=7558

Checklist

  • Tests updated.
  • N/A Documentation added.
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX].
  • N/A about-versioning.md updated with experimental features.

@krajorama krajorama requested a review from a team as a code owner March 7, 2024 19:19
@krajorama krajorama marked this pull request as draft March 7, 2024 19:19
Added new test case in TestRulerMetricsForInvalidQueriesAndNoFetchedSeries
with only the invalid query scenario where the user has entered an
invalid regex, but without selecting all chunks so the chunks limit
doesn't apply.

Since only one error happens, the error is of type errors.errorString
and not fmt.wrapError, which means the code to detect user errors and
cannot type assert to type QueryableError which in turn means it's treated
as a GRPC error, which it is not.
See https://github.com/grafana/mimir/blob/fdf381290fba868d18762c9790ebf543f49d4cc7/pkg/ruler/compat.go#L164

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
@krajorama krajorama force-pushed the krajo/fix-ruler-error-handling branch from 7c171d1 to 3ad0c9f Compare March 8, 2024 12:42
integration/ruler_test.go Outdated Show resolved Hide resolved
krajorama and others added 2 commits March 8, 2024 13:43
Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
@krajorama krajorama marked this pull request as ready for review March 11, 2024 10:42
@krajorama krajorama requested a review from a team as a code owner March 11, 2024 10:42
@krajorama krajorama changed the title Unit test for ruler not honoring user error Fix for ruler not honoring user error with local querier Mar 11, 2024
Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
Copy link
Member

@pstibrany pstibrany left a comment

Choose a reason for hiding this comment

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

Thank you!

@krajorama krajorama merged commit bf37da3 into main Mar 11, 2024
29 checks passed
@krajorama krajorama deleted the krajo/fix-ruler-error-handling branch March 11, 2024 11:31
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