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(remote-read): return err instead of nil #3053

Merged
merged 1 commit into from
Sep 27, 2022

Conversation

Dentrax
Copy link
Contributor

@Dentrax Dentrax commented Sep 27, 2022

Signed-off-by: Furkan [email protected]

What this PR does

For example, an invalid URL http://10.11.12.13:30090/prometheus/api/v1/read returns 404.

I'd expect an exit 1 and fatal log here, but it returns success instead.

$ go run cmd/mimirtool/main.go remote-read stats --address http://10.11.12.13:30090 --selector '{__name__!=""}'

INFO[0000] Created remote read client using endpoint 'http://10.11.12.13:30090/prometheus/api/v1/read'
INFO[0000] Querying time from=2022-09-27T08:48:54+03:00 to=2022-09-27T09:48:54+03:00 with selector={__name__!=""}
main: error: remote server http://10.11.12.13:30090/prometheus/api/v1/read returned HTTP status 404 Not Found: 404 page not found, try --help
exit status 1

Screen Shot 2022-01-18 at 17 40 19

Actual Behavior

$ go run cmd/mimirtool/main.go remote-read stats --address http://10.10.10.10:30090 --selector '{__name__=~"istio.*"}' --remote-read-path /api/v1/read --log.level debug --read-timeout 60s

INFO[0000] log level set to debug
INFO[0000] Created remote read client using endpoint 'http://10.10.10.10:30090/api/v1/read'
INFO[0000] Querying time from=2022-01-18T16:01:52+03:00 to=2022-01-18T17:01:52+03:00 with selector={__name__=~"istio.*"}

$ echo $?
0

Expected Behavior

INFO[0000] log level set to debug                       
INFO[0000] Created remote read client using endpoint 'http://10.10.10.10:30090/api/v1/read' 
INFO[0000] Querying time from=2022-01-18T16:39:20+03:00 to=2022-01-18T17:39:20+03:00 with selector={__name__=~"istio.*"} 
___2remote_read_stats: error: remote server http://10.10.10.10:30090/api/v1/read returned HTTP status 400 Bad Request: exceeded sample limit (50000000), try --help

Process finished with the exit code 1

Which issue(s) this PR fixes or relates to

Fixes grafana/cortex-tools#260

Checklist

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

@Dentrax Dentrax requested a review from a team as a code owner September 27, 2022 07:11
@aknuds1
Copy link
Contributor

aknuds1 commented Sep 27, 2022

Please re-write the PR description so it clearly states what the PR does, and why (for example, it doesn't change the golangci-lint settings any longer).

@Dentrax Dentrax force-pushed the fix/return-err-nil branch 3 times, most recently from 659e7b7 to 41d004c Compare September 27, 2022 07:25
@Dentrax
Copy link
Contributor Author

Dentrax commented Sep 27, 2022

Please re-write the PR description so it clearly states what the PR does, and why (for example, it doesn't change the golangci-lint settings any longer).

Sorry, a bit confused between two PRs since submitted both at the same time. It should be fine now.

Copy link
Contributor

@56quarters 56quarters 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, just one comment about the changelog.

@56quarters 56quarters enabled auto-merge (squash) September 27, 2022 14:23
@56quarters 56quarters merged commit 44bd7bc into grafana:main Sep 27, 2022
sysedwinistrator pushed a commit to sysedwinistrator/mimir that referenced this pull request Nov 21, 2022
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.

Exit if --remote-read-path incorrect
3 participants