Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use the default time retention value only when no size retention is set #5216

Merged
merged 4 commits into from Feb 19, 2019

Conversation

Projects
None yet
5 participants
@krasi-georgiev
Copy link
Member

krasi-georgiev commented Feb 14, 2019

fixes #5213

Now that we have time and size base retention time bases should not have a default value. A default is set only when both - time and size flags are not set.

This change will not affect current installations that rely on the default time based value, and will avoid confusions when only the size retention is set and it is expected that the default time based setting would be no longer in place.

Signed-off-by: Krasi Georgiev kgeorgie@redhat.com

@krasi-georgiev krasi-georgiev force-pushed the krasi-georgiev:time-size-flags branch from fb48d66 to aac8724 Feb 14, 2019

use the default time retention value only when no size retention is set
Now that we have time and size base retention it makes no sence to have
a default time based retention so the default is only used when time and
size flags are not set.

This change will not affect current installations that rely on the
default time based value, and will avoid confusions when people set only
time based retention and expect that the default time based setting
would be no longer in place.

Signed-off-by: Krasi Georgiev <kgeorgie@redhat.com>

@krasi-georgiev krasi-georgiev force-pushed the krasi-georgiev:time-size-flags branch from aac8724 to b0de281 Feb 14, 2019

Merge remote-tracking branch 'upstream/master' into time-size-flags
Signed-off-by: Krasi Georgiev <kgeorgie@redhat.com>
@gouthamve

This comment has been minimized.

Copy link
Member

gouthamve commented Feb 14, 2019

Should we release this with 2.7.2?

@brian-brazil

This comment has been minimized.

Copy link
Member

brian-brazil commented Feb 14, 2019

This level of behaviour change feels more like a minor version thing to me.


a.Flag("storage.tsdb.retention.time", "How long to retain samples in storage. Overrides \"storage.tsdb.retention\" if this flag is set to anything other than default.").
Default(defaultRetentionString).SetValue(&newFlagRetentionDuration)
SetValue(&newFlagRetentionDuration)

This comment has been minimized.

Copy link
@beorn7

beorn7 Feb 14, 2019

Member

"other than default" in the previous line doesn't make sense anymore from the user's perspective as the flag has no default value anymore.

Internally, the value will be the zero value of time.Duration, but that has no meaning to the user.

Suggestion:

Overrides \"storage.tsdb.retention\" if this flag is set to any non-zero value. If neither this flag nor \"storage.tsdb.retention\" nor "\storage.tsdb.retention.size\" is set, the retention time defaults to 15d.

This comment has been minimized.

Copy link
@krasi-georgiev

krasi-georgiev Feb 15, 2019

Author Member

updated

cfg.tsdb.MaxBlockDuration = cfg.tsdb.RetentionDuration / 10
if cfg.tsdb.RetentionDuration == 0 && cfg.tsdb.MaxBytes == 0 {
cfg.tsdb.RetentionDuration = defaultRetentionDuration
level.Warn(logger).Log("msg", "no time or size retention was set so using the default time retention", "duration", defaultRetentionDuration)

This comment has been minimized.

Copy link
@simonpasquier

simonpasquier Feb 14, 2019

Member

IMO it shouldn't a warning. A debug message is probably enough.

This comment has been minimized.

Copy link
@brian-brazil

brian-brazil Feb 14, 2019

Member

I'd suggest Info, and mentioning what the default is. We've had quite a few user questions around this over the years.

This comment has been minimized.

Copy link
@krasi-georgiev

krasi-georgiev Feb 15, 2019

Author Member

This will only appear when no time or size retention is set and I prefer to be more distinct than just an info message so I would rather keep it as a warning.

@brian-brazil the default value is already in the message.

This comment has been minimized.

Copy link
@simonpasquier

simonpasquier Feb 15, 2019

Member

As a user, I find it confusing to get a warning log while I just run ./prometheus

This comment has been minimized.

Copy link
@krasi-georgiev

krasi-georgiev Feb 15, 2019

Author Member

Confusing in what way?

for me a warn is something out of the ordinary. Something you should pay attention to.
In this case since no explicit flag or a setting was set a warning is raised to notify that a default of 15d retention is automatically applied. Without this it is easy to miss along the other info messages.

This comment has been minimized.

Copy link
@simonpasquier

simonpasquier Feb 15, 2019

Member

Because the warning wasn't present in previous versions and we aren't changing anything to the way retention settings work. I assume that a significant fraction of users run Prometheus without specifying the retention period and they might be confused seeing a warning log.

This comment has been minimized.

Copy link
@krasi-georgiev

krasi-georgiev Feb 15, 2019

Author Member

well there is a change in the behaviour. Before this was a default and now it is only applied when no time/size retention is set.

Anyway it is not that important so lets have one more person give an opinion and will leave or change accordingly.

This comment has been minimized.

Copy link
@beorn7

beorn7 Feb 15, 2019

Member

I vote for INFO.

This comment has been minimized.

Copy link
@krasi-georgiev

krasi-georgiev Feb 15, 2019

Author Member

updated, thanks for the feedback.

This comment has been minimized.

Copy link
@simonpasquier

simonpasquier Feb 15, 2019

Member

Thanks Krasi!

nits
Signed-off-by: Krasi Georgiev <kgeorgie@redhat.com>

@krasi-georgiev krasi-georgiev force-pushed the krasi-georgiev:time-size-flags branch from ea736ec to 10fbe04 Feb 15, 2019

reduced the log level to info.
Signed-off-by: Krasi Georgiev <kgeorgie@redhat.com>
@krasi-georgiev

This comment has been minimized.

Copy link
Member Author

krasi-georgiev commented Feb 16, 2019

anymore comments , changes?

@brian-brazil

This comment has been minimized.

Copy link
Member

brian-brazil commented Feb 19, 2019

👍

@krasi-georgiev krasi-georgiev merged commit a3c41f4 into prometheus:master Feb 19, 2019

3 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@krasi-georgiev krasi-georgiev deleted the krasi-georgiev:time-size-flags branch Feb 19, 2019

@krasi-georgiev

This comment has been minimized.

Copy link
Member Author

krasi-georgiev commented Feb 19, 2019

Thanks Brian.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.