Document and maybe consolidate duration parameter format

[grobie]

The prometheus server accepts a couple of time duration parameters. These currently only allow the golang time.Duration units, with h being the largest one. On the other side, promql also accepts d/w/y etc.

So far, the valid format is not document anywhere, neither in the usage nor on prometheus.io. We should at least document that.

I'd also vote to consolidate the formats and accept the same format everywhere. There are arguments that any unit larger than hour can be ambiguous due to leap seconds/days. Personally I'd argue a leap seconds and days don't matter when someone specifies large retention intervals.

[brian-brazil]

I was of the impression that we'd already done all of that.

[beorn7]

Flags are still using vanilla Go duration.

A problem is that PromQL and config durations are not a superset of the Go durations. The former supports d, w, y. The latter can do mixed units like 5m30s.

[grobie]

Do you think we actually need mixed units in flags? I can't think of a
reasonable use case immediately, and one can always drop down to the next
lower unit.

On Mon, Aug 15, 2016 at 2:50 AM Björn Rabenstein notifications@github.com
wrote:

Flags are still using vanilla Go duration.

A problem is that PromQL and config durations are not a superset of the Go
durations. The former supports d, w, y. The latter can do mixed units like
5m30s.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#1883 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAANaJA63j_dGFhToeS92OW0W1vJP469ks5qgAwkgaJpZM4Jhaar
.

[beorn7]

I'm fine with not allowing mixed units. But it could break existing setups, and that's why we didn't just flip the switch.

If we allowed mixed units, we would have a non-breaking upgrade path.

[brian-brazil]

I think we're okay with switching to not having mixed units for the sake of consistency, and such a breaking change is permitted within 1.0.

[grobie]

I don't think we can break the existing usage. We explicitly promised
"Exact flag meanings may change as necessary. However, changes will never
cause the server to not start with previous flag configurations.". I'd
print a warning log line in that case and convert them.

On Mon, Aug 15, 2016 at 2:05 PM Brian Brazil notifications@github.com
wrote:

I think we're okay with switching to not having mixed units for the sake
of consistency, and such a breaking change is permitted within 1.0.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#1883 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAANaMVxYWAx8eFfMMEcKl2dtg3p1dJsks5qgKp-gaJpZM4Jhaar
.

[brian-brazil]

That wording must have changed at some point, it used to be that we wouldn't remove flags.

It would be valid to ignore such invalid flags.

[fabxc]

Do we need to reconsider this for 2.0?

[grobie]

Yes, in case we still have time duration flags left. I didn't do it due to my concerns about compatibility.

[fabxc]

So the plan would to to switch to our single unit while supporting days, weeks, and years? Where does this need changes. So far I got:

• storage.tsdb.retention
• storage.tsdb.min-block-duration
• storage.tsdb.max-block-duration

Anything I missed?

[grobie]

@fabxc I'm counting 7 DurationVars.

[gouthamve]

So do we make this change only to those flags requiring long intervals or to all Duration flags for consistency?

I am talking about:

--query.lookback-delta
--query.timeout
--web.read-timeout
--alertmanager.timeout

[grobie]

+1 to all.

…

On Fri, Jun 16, 2017 at 3:01 PM Goutham Veeramachaneni < ***@***.***> wrote: So do we make this change only to those flags requiring long intervals or to all Duration flags for consistency? I am talking about: --query.lookback-delta --query.timeout --web.read-timeout --alertmanager.timeout — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1883 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAANaE68_A8X2lwoyLBZokZP9v5pTwslks5sEnydgaJpZM4Jhaar> .

[fabxc]

Yea, that came to my mind as well. But all seems saner and I cannot really see a use for mixed units.

[gouthamve]

@brian-brazil @grobie Is only documenting it left as we moved all the flags? Or do we need to change anything in promql?

[grobie]

It's not about promql, it's about flags. I don't know what the current state is in 2.0, but in 1.x we have for example -storage.local.retention 360h0m0s. This is the standard golang time format. The request was to consolidate all of these and use our PromQL duration format, allowing to set -storage.local.retention 15d.

[gouthamve]

Then I think this can be closed as it is now in 2.0:

➜  prometheus git:(dev-2.0) ✗ ./prometheus --storage.tsdb.retention=15d
INFO[0000] Starting prometheus (version=, branch=, revision=)  source="main.go:202"
INFO[0000] Build context (go=go1.8.3, user=, date=)      source="main.go:203"
INFO[0000] Host details (darwin)                         source="main.go:204"
INFO[0000] Starting tsdb                                 source="main.go:216"

[grobie]

Cool, the acceptance criteria is basically: all duration flags should use our PromQL style duration format and not the golang style one. Feel free to close if that's the case! Thanks for checking.

…

On Thu, Jul 27, 2017 at 2:49 PM Goutham Veeramachaneni < ***@***.***> wrote: Then I think this can be closed as it is now in 2.0: ➜ prometheus git:(dev-2.0) ✗ ./prometheus --storage.tsdb.retention=15d INFO[0000] Starting prometheus (version=, branch=, revision=) source="main.go:202" INFO[0000] Build context (go=go1.8.3, user=, date=) source="main.go:203" INFO[0000] Host details (darwin) source="main.go:204" INFO[0000] Starting tsdb source="main.go:216" — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1883 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAANaDyBRT5IHfhURaH8OpmpnMgMATTLks5sSIdAgaJpZM4Jhaar> .

[gouthamve]

Fixed in #2844

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.