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

Better error reporting for missing query string parameters #4811

Closed
candlerb opened this Issue Oct 31, 2018 · 8 comments

Comments

Projects
None yet
8 participants
@candlerb
Copy link

candlerb commented Oct 31, 2018

Proposal

Use case. Why is this important?

Currently a missing query parameter is reported as if the query parameter was present with empty string. Furthermore, the error message does not include the name of the faulty parameter. This makes it hard to identify the problem with the query.

This affects people experimenting with the API for the first time.

Bug Report

What did you do?

curl -s 'http://localhost:9090/api/v1/query_range?query=ifHCInOctets%7bifDescr%3d"pppoe-out1"%7d' | python3 -mjson.tool

What did you expect to see?

Something like:

"error": "missing parameter 'start'"

What did you see instead? Under which circumstances?

{
    "status": "error",
    "errorType": "bad_data",
    "error": "cannot parse \"\" to a valid timestamp"
}

Environment

  • System information:

      Linux 4.4.0-138-generic x86_64
    
  • Prometheus version:

    prometheus, version 2.4.3 (branch: HEAD, revision: 167a4b4e73a8eca8df648d2d2043e21bdb9a7449)
      build user:       root@1e42b46043e9
      build date:       20181004-08:42:02
      go version:       go1.11.1
    
@BrianHa94

This comment has been minimized.

Copy link

BrianHa94 commented Nov 1, 2018

@brian-brazil I would like to take this one up

@codwu

This comment has been minimized.

Copy link

codwu commented Nov 7, 2018

@BrianHa94 Are you still working on it? I would like to open a PR for this issue.

@BrianHa94

This comment has been minimized.

Copy link

BrianHa94 commented Nov 7, 2018

@codwu Yes, I have a fix. I just need to test before opening a PR.

@gouthamve

This comment has been minimized.

Copy link
Member

gouthamve commented Feb 1, 2019

Hi @BrianHa94 could you submit the patch? This is still an issue on 2.7.1

@palash25

This comment has been minimized.

Copy link
Contributor

palash25 commented Feb 17, 2019

Hi @gouthamve I would like to work on this

Just to be sure this change would require using errors.New with appropriate messages and adding it to the apiError struct wherever params are checked like in this snippet start, err := parseTime(r.FormValue("start")) right? and ofcourse changing the error response in these test cases

@geekodour

This comment has been minimized.

Copy link
Contributor

geekodour commented Feb 18, 2019

@gouthamve @palash25 Would it be useful if we set start to minTime and end to time.Now or something because otherwise I think the error message is helpful only?

@codesome

This comment has been minimized.

Copy link
Member

codesome commented Feb 18, 2019

@palash25 you can go ahead if @BrianHa94 is not working on it. @BrianHa94 are you still on it?

@BrianHa94

This comment has been minimized.

Copy link

BrianHa94 commented Feb 18, 2019

@palash25 please go ahead.

palash25 added a commit to palash25/prometheus that referenced this issue Feb 18, 2019

palash25 added a commit to palash25/prometheus that referenced this issue Feb 19, 2019

queryRange: Add more descriptive error messages
Fixes: prometheus#4811

Signed-off-by: Palash Nigam <npalash25@gmail.com>

palash25 added a commit to palash25/prometheus that referenced this issue Feb 19, 2019

queryRange: Add more descriptive error messages
Fixes: prometheus#4811

Signed-off-by: Palash Nigam <npalash25@gmail.com>

juliusv added a commit that referenced this issue Feb 19, 2019

queryRange: Add more descriptive error messages (#5229)
Fixes: #4811

Signed-off-by: Palash Nigam <npalash25@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.