API client Series() call can't handle minTime/maxTime set by prometheus

[jacksontj]

This is a bit of a convoluted error so I'll do my best to explain.

If an API call to /series is made without a start/end time prometheus sets minTime/maxTime as the value (https://github.com/prometheus/prometheus/blob/master/web/api/v1/api.go#L462). If I then turn around and use this time with the API client (to presumably send the same request) I get an error:

{
"status": "error",
"errorType": "execution",
"error": "bad_data: cannot parse \"292277025-08-17T23:12:54.999-08:00\" to a valid timestamp"
}

This is because the times used for minTime/maxTime can't be formatted to an RFC3339Nano timestamp. So for my use case in promxy I get a call from grafana (in this example) with no start/end time -- so prometheus sets the time to these min/max times but then I cannot pass that time down to another prometheus to fetch the data, as its not a valid timestamp.

Go playground showing the behavior: https://play.golang.org/p/hM_DWyv0JMK

So I'm not sure what we want to do exactly to handle this. Locally I have copied the min/max times into the client package and are optionally not setting the start/end times if they match. I'm not a major fan of the copy/paste of vars, but unfortunately those times are also private in the prometheus api/v1 package (https://github.com/prometheus/prometheus/blob/master/web/api/v1/api.go#L441). Alternatively we could make it so the client doesn't set the start/end if start/end are time.IsZero() which would allow the client/caller to pick when they don't send the timestamp (instead of us transparently doing it in the client).

[brian-brazil]

That sounds like a bug on the Prometheus end, however I also recall that Go has some limits in terms of what it can handle time wise.

[jacksontj]

@brian-brazil Yea, I could go either way on this TBH (client or server issue). If we were to tackle this server-side then IMO we'd want to pass some other time down the stack (presumably min/max that can be represented in what the API requires) -- right now the time we set is not expressible in the format the API requires.

[beorn7]

@krasi-georgiev assigning to you as the API expert. Perhaps you have an opinion/idea?

[brian-brazil]

Something else that occurs to me is that if this is something that could also affect other languages, then we should probably fix it on the server.

[krasi-georgiev]

+1 on fixing server side

[jacksontj]

What fix do we want server side? Should I just change the min/max time to the min/max that can be represented as the timestamp type we use?

…

On Wed, Jul 3, 2019, 5:22 AM Krasi Georgiev ***@***.***> wrote: +1 on fixing server side — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#614?email_source=notifications&email_token=AAMLPHMLHAGW5TD3X5JUI73P5SKY5A5CNFSM4H45Z6Y2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZEIQ7Q#issuecomment-508070014>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAMLPHMWF46DHX6YJVZGJILP5SKY5ANCNFSM4H45Z6YQ> .

[brian-brazil]

Hmm, looking a bit more this looks like a Go bug. The parser can only handle 4 digit years. golang/go#20555 is related, but different.

[jacksontj]

Well, as a workaround until it supports more than 4 digit year dates, we could change the min/max to:

minTime: "1970-01-01T00:00:00.0Z"
maxTime: "9999-12-31T23:59:59.999999999Z"

Which buys us 7980 years to sort that out :)

Alternatively we could implement parsing for the current min/max times

[jacksontj]

@brian-brazil friendly bump on #614 (comment) :)

[brian-brazil]

I'm not sure what the best way to deal with this is to be honest, as I don't think a Go fix is coming soon.

[jacksontj]

Our options for a server-side fix are (1) set min/max time to something the time package can parse or (2) implement the time parsing ourselves (there's already a parseTime method, so it wouldn't be too bad).

Either approach is fine IMO we just have to pick one :)

[brian-brazil]

The latter is probably better, I can imagine changing the guard value in future could cause issues. But I don't feel strongly on this.

[jacksontj]

I had another idea, instead of implementing all the parsing I just added special-case checks for min/max time (prometheus/prometheus@32c3b7c -- part of prometheus/prometheus#5734) This way we can delay implementing the actual time parsing (hopefully upstream sorts that out?) but still be able to unmarshal our own time range :)

[brian-brazil]

That's a good idea.

[jacksontj]

In addition to the server fix, IMO we should do #617 as a (1) simplification and (2) more complete workaround on the client side.

[krasi-georgiev]

Hm, why not leave as is so that we will know when the server is not behaving as it should?

[jacksontj]

The fix we put in on the server side is a hack really because of the shortcomings of stdlib time parse. The prom API already supports 2 timestamp formats, so it makes sense to change this client to the one that "just works"