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

Plugins: Ensure catching all appropriate 4xx api/ds/query scenarios #47565

Merged
merged 5 commits into from
Apr 29, 2022
Merged

Plugins: Ensure catching all appropriate 4xx api/ds/query scenarios #47565

merged 5 commits into from
Apr 29, 2022

Conversation

wbrowne
Copy link
Member

@wbrowne wbrowne commented Apr 11, 2022 •
edited
Loading

What this PR does / why we need it:

  • Add missing catch for valid data source not found case (currently returns 500, change to 404)
  • Add missing catch for plugin not actively registered (currently returns 500, change to 404)

@wbrowne wbrowne changed the title Plugins: Ensure catching all valid 4xx ds/query scenarios Plugins: Ensure catching all valid 4xx api/ds/query scenarios Apr 11, 2022
@wbrowne wbrowne changed the title Plugins: Ensure catching all valid 4xx api/ds/query scenarios Plugins: Ensure catching all appropriate 4xx+5xx api/ds/query scenarios Apr 11, 2022
marefr
marefr reviewed Apr 12, 2022
View reviewed changes
Copy link
Member

@marefr marefr left a comment

Choose a reason for hiding this comment

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

As a side note. One case we're not handling today is timeouts, e.g. 504 Gateway Timeout. Guess those ends up with a 500 currently.

pkg/api/metrics.go Outdated Show resolved Hide resolved
pkg/api/metrics.go Outdated Show resolved Hide resolved
@wbrowne wbrowne changed the title Plugins: Ensure catching all appropriate 4xx+5xx api/ds/query scenarios Plugins: Ensure catching all appropriate 4xx api/ds/query scenarios Apr 28, 2022
@wbrowne wbrowne marked this pull request as ready for review April 28, 2022 15:30
@wbrowne wbrowne requested a review from a team as a code owner April 28, 2022 15:30
@wbrowne wbrowne requested review from papagian, yangkb09 and ying-jeanne and removed request for a team April 28, 2022 15:30
@wbrowne wbrowne added the no-backport Skip backport of PR label Apr 28, 2022
@wbrowne wbrowne added this to the 8.5.2 milestone Apr 28, 2022
@wbrowne wbrowne self-assigned this Apr 28, 2022
papagian
papagian reviewed Apr 28, 2022
View reviewed changes
Copy link
Contributor

@papagian papagian left a comment

Choose a reason for hiding this comment

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

LGTM but if it's intentionally assigned to 8.5.2 milestone then needs to be back-ported.

@wbrowne wbrowne added backport v8.5.x Mark PR for automatic backport to v8.5.x and removed no-backport Skip backport of PR labels Apr 28, 2022
@wbrowne
Copy link
Member Author

wbrowne commented Apr 28, 2022

Thanks for the reminder @papagian! 😄

marefr
marefr approved these changes Apr 28, 2022
View reviewed changes
Copy link
Member

@marefr marefr left a comment

Choose a reason for hiding this comment

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

LGTM

@wbrowne wbrowne merged commit c8a71a2 into main Apr 29, 2022
@wbrowne wbrowne deleted the ds-query-query-errs branch April 29, 2022 10:08
@grafanabot grafanabot mentioned this pull request Apr 29, 2022
Merged
grafanabot pushed a commit that referenced this pull request Apr 29, 2022
@wbrowne @grafanabot
Plugins: Ensure catching all appropriate 4xx api/ds/query scenarios (#…
1a6dbc5 
…47565)

* catch ds 404s

* catch plugin errs

* go step back

(cherry picked from commit c8a71a2)
wbrowne added a commit that referenced this pull request Apr 29, 2022
@grafanabot @wbrowne
Plugins: Ensure catching all appropriate 4xx api/ds/query scenarios (#…
92c08f0 
…47565) (#48514)

* catch ds 404s

* catch plugin errs

* go step back

(cherry picked from commit c8a71a2)

Co-authored-by: Will Browne <wbrowne@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@papagian papagian papagian left review comments

@marefr marefr marefr approved these changes

@yangkb09 yangkb09 Awaiting requested review from yangkb09 yangkb09 was automatically assigned from grafana/backend-platform

@ying-jeanne ying-jeanne Awaiting requested review from ying-jeanne ying-jeanne was automatically assigned from grafana/backend-platform

Assignees

@wbrowne wbrowne

Labels
add to changelog area/backend/api area/backend/plugins area/backend area/plugins backport v8.5.x Mark PR for automatic backport to v8.5.x type/bug
Projects
Plugins Platform / Grafana Community
Archived in project
Milestone
8.5.2
Development

Successfully merging this pull request may close these issues.
4 participants
@wbrowne @papagian @marefr @grafanabot