Add option to client_golang to *not* send the time on a Series() call

[jacksontj]

The series call doesn't require a time, it would be nice if we had some mechanism in the go API to not set a time. I had opened #615 towards this, but I'm not sure this is a great approach, maybe we instead make the time.Time pointers?

[beorn7]

Could the zero value of time.Time be used as a marker for “do not use”? (Cf. https://golang.org/pkg/time/#Time.IsZero .) I might be completely wrong. Not an expert of the API client. @krasi-georgiev to vet.

[jacksontj]

My only concern is that prom's minTime is actually significantly before time.Time{}

minTime = -292273086-05-16 16:47:06 +0000 UTC
time.Time{} = 0001-01-01 00:00:00 +0000 UTC

on go playground

So if you wanted to query a time before Zero time (since apparently thats valid?) it'd be a bit weird to have our "null" (don't send a value) be in the middle of that.

[beorn7]

Oh right, I now even remember that we had discussed this problem in a different context already (i.e. a valid Prometheus sample might happen to have the time.Time zero value as its actual timestamp).

[beorn7]

So yeah, disregard my comment, please.

[bboreham]

I just hit this on LabelValues(). Evidently somebody else though that time.Time{} should behave as 'not set':

https://github.com/cortexproject/cortex/blob/07d6cfc4eb848fc410262773255dfec750c0a64f/integration/getting_started_single_process_config_test.go#L62

How about creating a constant with a value of minTime, that would be taken as "don't send a value"?

[jacksontj]

Its been a while since this issue was touched last; but this came up again as an issue on promxy (jacksontj/promxy#528) -- I was wondering if we had any ideas/thoughts on fixing this?

[jacksontj]

The "clearest" API change (IMO) would be to change from a time.Time to a *time.Time (pointer) -- that way if it is null we just don't send it on. This is more accurate to what the underlying HTTP API is (since these are optional arguments).

[jacksontj]

I was looking to create a PR against this and noticed that the client already is VERY inconsistent. Some methods (such as Query) use IsZero and others do nothing. From a quick review of the prometheus server API -- all of the time values are optional (except for QueryRange) -- so it this seems like something we should definitely fix.

Practically speaking time.Time{} or *time.Time approaches both work (*time.Time is more accurate, but an API change).

[bboreham]

Pointers to Time here is not idiomatic Go, in my opinion.

Adding a new method like SeriesForAllTime would be non-breaking.

I think I’d be fine with the zero time meaning “not set”, since that is obvious. I doubt there are many people who have data at that instant.

(Note there is a bug with model.Earliest - #951)

[bwplotka]

Thanks everyone for great points.

Another not tested idea: We could replace time.Time in arguments with interface that time.Time implements and then passing nil will work (!) and is detectable. I don't think that would break compatibility. (?)

Also technically speaking we can break compatibility in api: https://github.com/prometheus/client_golang#important-note-about-releases-and-stability - I wouldn't change it to pointer, but rather optional argument e.g. WithStartTime. This would mean we need to adjust all API endpoints which would be pain to everybody.

Note that, if we aim for v2, I think we discussed API should be in form of OpenAPI spec, maintained outside of this repo - anybody could auto-generate code in their language. Any improvements ideally would go to OpenAPI client Go generator (last time I checked it wasn't great).

For now, I would indeed create var TimeNotSet time.Time (which returns true on IsZero) (that's stupid as we don't have immutable vars in Go) use empty time.Time until we have clear use case where somebody was upset or blocked by this. This is recommended widely - it means year 1 not 1970 as it is usually.

[jacksontj]

@bwplotka if we wanted to change to another "interface" I would do something like this:

-func (h *httpAPI) QueryExemplars(ctx context.Context, query string, startTime, endTime time.Time) ([]ExemplarQueryResult, error) {
+func (h *httpAPI) QueryExemplars(ctx context.Context, query string, startTime, endTime timeArg) ([]ExemplarQueryResult, error) {
        u := h.client.URL(epQueryExemplars, nil)
        q := u.Query()
 
        q.Set("query", query)
-       if !startTime.IsZero() {
+       if startTime != nil {
                q.Set("start", formatTime(startTime))
        }
-       if !endTime.IsZero() {
+       if endTime != nil {
                q.Set("end", formatTime(endTime))
        }
        u.RawQuery = q.Encode()
@@ -1455,6 +1455,11 @@ func (h *apiClientImpl) DoGetFallback(ctx context.Context, u *url.URL, args url.
        return resp, body, warnings, err
 }
 
-func formatTime(t time.Time) string {
+type timeArg interface {
+    Unix() int64
+    Nanosecond() int
+}
+
+func formatTime(t timeArg) string {
        return strconv.FormatFloat(float64(t.Unix())+float64(t.Nanosecond())/1e9, 'f', -1, 64)
 }

This way you aren't masking time.Time and it can be nil or something else entirely (more "go" as well, since those are the only 2 args we care about).

That being said, given that one API endpoint already uses time.IsZero (and thats not an uncommon usage) I think the PR I have open doing that is the way to go #1238

[bwplotka]

Agree and thanks for checking (and fixing)! We can always move to interface if lack of the possibility to specify first millisecond of year 0001 becomes a problem (: