Parse the time arg as a float #65
Conversation
|
|
handlers.go
Outdated
| floatEnd, err := strconv.ParseFloat(ts[0], 64) | ||
| if err != nil { | ||
| level.Error(t.Logger).Log(lfEvent, "error parsing time", "params", params, lfDetail, err.Error()) | ||
| return []byte{}, &http.Response{} |
There was a problem hiding this comment.
Is this PR against an up-to-date master branch? The upstream version already allows returning errors from fetchPromQuery(), which we should do here too. https://github.com/Comcast/trickster/blob/master/handlers.go#L363
handlers.go
Outdated
| // As such we need to parse the value as a float64. For the sake of the | ||
| // rest of the code here we'll cast back to an int64 (effectively | ||
| // truncating the precision) | ||
| floatEnd, err := strconv.ParseFloat(ts[0], 64) |
There was a problem hiding this comment.
Actually it can also be an rfc3339 timestamp. Use this function instead to parse it (and then call Unix() on it to get back to seconds): https://github.com/Comcast/trickster/blob/master/handlers.go#L1066-L1077
|
@juliusv rebased on master, apparently I had an old checkout :) |
handlers.go
Outdated
| if t, ok := params[upTime]; ok { | ||
| end, err = strconv.ParseInt(t[0], 10, 64) | ||
| if ts, ok := params[upTime]; ok { | ||
| // time is actually a decimal value to include sub-second granularity. |
There was a problem hiding this comment.
This comment needs updating (or removal) now, as it can be rfc3339 as well.
handlers.go
Outdated
| // truncating the precision) | ||
| reqStart, err := parseTime(ts[0]) | ||
| if err != nil { | ||
| return []byte{}, nil, err |
There was a problem hiding this comment.
This can just be return nil, nil, err
The time arg in the params can contain decimal points for more precision. Before this patch that would cause an error on the time parse, which was ignored, causing the time sent downstream to be `0`. This patch (1) properly parses the time as a float64-- then casts to int64 and (2) checks for errors in the parsing
|
👍 |
|
Good from my side, but I can't merge :) |
|
Deferring to Julius's approval on this one and merging ;) Thanks to everyone! @juliusv I'm checking with Comcast as to whether I am permitted to make you an approver who can merge as well. When I hear back in the affirmative, I'll hook you up with that access. |
|
@jranson Uh oh, am I on the hook for reviews now? ;) But yeah, I won't say no to merge rights. |
The time arg in the params can contain decimal points for more
precision. Before this patch that would cause an error on the time
parse, which was ignored, causing the time sent downstream to be
0.This patch (1) properly parses the time as a float64-- then casts to
int64 and (2) checks for errors in the parsing