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

Loki: Improve handling of empty responses #52397

Merged
merged 1 commit into from Jul 19, 2022
Merged

Loki: Improve handling of empty responses #52397

merged 1 commit into from Jul 19, 2022

Conversation

gabor
Copy link
Contributor

@gabor gabor commented Jul 18, 2022

helps with #52046

the loki http response parser calls converter.ReadPrometheusStyleResult (https://github.com/grafana/grafana/blob/main/pkg/util/converter/prom.go#L27), which can return a nil-pointer. the loki code did not handle this case.

the linked github-issue describes such a situation.

this pull-request adds code to handle the nil-pointer case.

how to test:

  • the new code is covered by a test, so it should be ok
  • try to run some generic loki queries, to make sure this did not break something else

(NOTE: this will not resolve the reported issue (why are we not getting back any data for the log-volume-query), but we should handle the nil-pointer-case.

@gabor gabor requested review from svennergr, ivanahuckova and a team and removed request for a team July 18, 2022 11:58
@gabor gabor added this to the 9.0.4 milestone Jul 18, 2022
@gabor gabor changed the title Loki: improve handling of empty responses Loki: Improve handling of empty responses Jul 18, 2022
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

// it's hard to say if this is an error-case or not.
// we know the http-response was a success-response
// (otherwise we wouldn't be here in the code),
// so we will go with a success, with no data.
Copy link
Contributor

Choose a reason for hiding this comment

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

I like the comment. However, since this seems like a rare occasion, should we add some logging here? But I am also fine without it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm.. i'd say let's go without logging for now. i'm planning to add some way to record this strange response in a separate PR.

@gabor gabor merged commit 46eec85 into main Jul 19, 2022
@gabor gabor deleted the gabor/loki-nil-fix branch July 19, 2022 06:13
grafanabot pushed a commit that referenced this pull request Jul 19, 2022
gabor added a commit that referenced this pull request Jul 19, 2022
(cherry picked from commit 46eec85)

Co-authored-by: Gábor Farkas <gabor.farkas@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants