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

Display correct values for the retention in the flags web gui. #5322

Merged
merged 4 commits into from Mar 11, 2019

Conversation

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

krasi-georgiev commented Mar 9, 2019

fixes #5321

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

Display correct values for the retention in the flags web gui.
Signed-off-by: Krasi Georgiev <kgeorgie@redhat.com>
@codesome

This comment has been minimized.

Copy link
Member

codesome commented Mar 9, 2019

This goes in 2.8.0

@roidelapluie

This comment has been minimized.

Copy link
Contributor

roidelapluie commented Mar 9, 2019

This will change the value in the flags. But it is confusing: if I add a maxbytes retention, the retention flag will change even if I do not change the retention flag.

I would propose to move that information to the Runtime and Build info page, by creating two metrics:

tsdb_retention_bytes
And
tsdb_retention_seconds

What do you think?

That could go in 2.9 ofc.

adding a log entry
Signed-off-by: Krasi Georgiev <kgeorgie@redhat.com>
@krasi-georgiev

This comment has been minimized.

Copy link
Member Author

krasi-georgiev commented Mar 9, 2019

This will change the value in the flags. But it is confusing: if I add a maxbytes retention, the retention flag will change even if I do not change the retention flag.

the flag will change only when no time or size retention is set and the default of 15d is applied. The behaviour hasn't changed since 2.7

if cfg.tsdb.RetentionDuration == 0 && cfg.tsdb.MaxBytes == 0 {

I would propose to move that information to the Runtime and Build info page, by creating two metrics:
tsdb_retention_bytes
And
tsdb_retention_seconds
What do you think?

I don't see why would we move it under Runtime and Build.
These are flags and looks logical to stay under the /flags web page.

@brian-brazil

This comment has been minimized.

Copy link
Member

brian-brazil commented Mar 9, 2019

I'd expect flags to be what the user passed in, and runtime to show current status (i.e. what actually applies).

@krasi-georgiev

This comment has been minimized.

Copy link
Member Author

krasi-georgiev commented Mar 9, 2019

I'd expect flags to be what the user passed in, and runtime to show current status (i.e. what actually applies).

aaah yeah that is a good way to think about it. otherwise if you miss the logs it is weird to see 15d of retention when you didn't pass anything as a flag.

I like the idea and will look into it.

@roidelapluie

This comment has been minimized.

Copy link
Contributor

roidelapluie commented Mar 9, 2019

Yes @bbrazil explained exactly what I had in mind.

added the retention info to the runtime status page
Signed-off-by: Krasi Georgiev <kgeorgie@redhat.com>
@krasi-georgiev

This comment has been minimized.

Copy link
Member Author

krasi-georgiev commented Mar 11, 2019

updated!

now the /flags page will show exactly what was passed where the /status will show what is the actual retention values.

Flags page
Screenshot_20190311_104324

Status Page
Screenshot_20190311_104254

<tr>
<th>Storage Size Retention</th>
<td>{{.RetentionSize}}</td>
</tr>

This comment has been minimized.

Copy link
@roidelapluie

roidelapluie Mar 11, 2019

Contributor

Can we simplify this and have just one line?

(exepmle values that would be. printed:

  • 15d
  • 15d or 10GB
  • 19GB
    )

This comment has been minimized.

Copy link
@krasi-georgiev

krasi-georgiev Mar 11, 2019

Author Member

yep , not a bad idea. I updated the code.

This comment has been minimized.

Copy link
@krasi-georgiev

krasi-georgiev Mar 11, 2019

Author Member

Screenshot_20190311_110834
Screenshot_20190311_110908

simplify the retention display
Signed-off-by: Krasi Georgiev <kgeorgie@redhat.com>

@codesome codesome merged commit 9d96ada into prometheus:master Mar 11, 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:flags-gui branch Mar 11, 2019

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.