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

Handle query parsing errors on QFE range split middleware #6172

Merged
merged 3 commits into from Mar 2, 2023
Merged

Handle query parsing errors on QFE range split middleware #6172

merged 3 commits into from Mar 2, 2023

Conversation

pedro-stanaka
Copy link
Contributor

@pedro-stanaka pedro-stanaka commented Mar 1, 2023 •
edited

  • I added CHANGELOG entry for this change.
  • Change is not relevant to the end user.

Related/fixes: #6075

Changes

  • Return JSON error message from query range split middleware when parsing PromQL expression.

Verification

I have tested it by running QFE with query split enabled.

fpetkovski
fpetkovski reviewed Mar 1, 2023
View reviewed changes
Copy link
Contributor

@fpetkovski fpetkovski left a comment

Choose a reason for hiding this comment

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

Thanks, mind adding a simple test for this so we don't break it again in the future?

@pull-request-size pull-request-size bot added size/S and removed size/XS labels Mar 1, 2023
fpetkovski
fpetkovski previously approved these changes Mar 1, 2023
View reviewed changes
Copy link
Contributor

@fpetkovski fpetkovski left a comment

Choose a reason for hiding this comment

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

Thanks, lgtm.

matej-g
matej-g previously approved these changes Mar 2, 2023
View reviewed changes
Copy link
Collaborator

@matej-g matej-g 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! 👍

@pedro-stanaka
Handle query parsing errors on QFE range split middleware
1561d3b 
Signed-off-by: Pedro Tanaka <pedro.tanaka@shopify.com>
@pedro-stanaka
Added changelog
a0e7254 
Signed-off-by: Pedro Tanaka <pedro.tanaka@shopify.com>
@pedro-stanaka
Adding small test
6a6a3fd 
Signed-off-by: Pedro Tanaka <pedro.tanaka@shopify.com>
@pedro-stanaka pedro-stanaka dismissed stale reviews from matej-g and fpetkovski via 6a6a3fd March 2, 2023 15:21
@pedro-stanaka pedro-stanaka force-pushed the hotfix/qfe-query-split-handle-parse-errors branch from 1dd9597 to 6a6a3fd Compare March 2, 2023 15:21
@pedro-stanaka
Copy link
Contributor Author

Sorry folks. I rebased the branch and ended up dismissing your reviews. cc @fpetkovski @matej-g

matej-g
matej-g approved these changes Mar 2, 2023
View reviewed changes
Copy link
Collaborator

@matej-g matej-g left a comment

Choose a reason for hiding this comment

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

Looks good. I'm unsure why the CircleCi is not running again, but it ran before the force push, so I consider this to be green. We need to take a look why this keeps happening to some contributors, but no sense in blocking this.

@matej-g matej-g merged commit b1d083f into thanos-io:main Mar 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matej-g matej-g matej-g approved these changes

@fpetkovski fpetkovski fpetkovski left review comments

Assignees
No one assigned
Labels
size/S
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Error executing query: "Unexpected token '<','<html> <h'.. is not a valid json
3 participants
@pedro-stanaka @fpetkovski @matej-g