Detailed query plan errors #1996
Conversation
colega
force-pushed
the
detailed-plan-errors
branch
from
bbc38bd
to
e4d209c
Compare
Right now when we fail the plan we see an error message which is:
argument bad type. expected int - got etString
If the query is not trivial (with nested function calls, for instancer),
it becomes very complicated to understand which arg it's referring too.
This adds context to the errors returned by NewPlan() (specifically,
newplanFunc()), telling which function and which arg caused the error.
The original error is wrapped now so all the usages of NewPlan() now
check the wrapped error rather than specific type.
Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
colega
force-pushed
the
detailed-plan-errors
branch
from
e4d209c
to
a61b8a0
Compare
| var ( | ||
| argExp Arg | ||
| nextPos int | ||
| ) |
There was a problem hiding this comment.
what are all these pos vs nextPos changes about?
this is non-obvious. I would recommend to leave this PR for just the detailed errors, and if you want to do other unrelated cleanups, to do them separately, or at least in separate commits that explain the reasoning of the change. (also, this seems to make the code more complicated and i'm not sure why)
There was a problem hiding this comment.
Methods like e.consumeBasicArg(pos, argExp) return the next position to consume as their first param, but they return 0 for error cases. I need to know which was the position they were processing for the error messages so I split the advancing logic into two steps in each call: only update pos if the error was nil.
There was a problem hiding this comment.
ah i'm sorry. that makes perfect sense now. i'll blame it on being monday morning.
| // When graphite issues a request (and sets the request.NoProxy flag), | ||
| // it should be for raw data only without any function processing. | ||
| ctx.Error(err.HTTPStatusCode(), err.Error()) | ||
| return |
There was a problem hiding this comment.
why did you take out this clause? if this condition is triggered it would result in an infinite proxy loop.
There was a problem hiding this comment.
!request.NoProxy is still a condition to proxy the request below. All the logic for unknown function is the same except for the stats call, so I just left the stats call alone.
There was a problem hiding this comment.
ah i see. you are right! nice code simplification!
Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
See #1996 I missed the CHANGELOG.md there. Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
Right now when we fail the plan we see an error message which is:
If the query is not trivial (with nested function calls, for instancer), it becomes very complicated to understand which arg it's referring too.
This adds context to the errors returned by NewPlan() (specifically, newplanFunc()), telling which function and which arg caused the error. The error message is now:
The original error is wrapped now so all the usages of NewPlan() now check the wrapped error rather than specific type. Additionally, added wrapping/unwrapping logic to the response.WrapError() (we could skip this, but since it says it wraps it, why not wrapping it for real).