Skip to content
This repository was archived by the owner on Aug 23, 2023. It is now read-only.

add min-age option to mt-index-cat #1064

Merged
merged 2 commits into from
Sep 28, 2018
Merged

add min-age option to mt-index-cat #1064

merged 2 commits into from
Sep 28, 2018

Conversation

Dieterbe
Copy link
Contributor

No description provided.

@Dieterbe Dieterbe requested review from woodsaj and replay September 26, 2018 13:10
@replay
Copy link
Contributor

replay commented Sep 26, 2018

That's great. Should we add such a parameter to /metrics/index.json as well?

if minAge != "0" {
minAgeInt, err := dur.ParseNDuration(minAge)
perror(err)
cutoffMin = int64(time.Now().Unix() - int64(minAgeInt))
Copy link
Contributor

@replay replay Sep 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would it make sense to only call time.Now().Unix() once and then reuse the returned value for the calculation of cutoff and cutoffMin? I know that in reality they're probably only going to be microseconds apart, but I think it's kind of unexpected that theoretically it would be possible that the time-range which is returned by this utility is not exactly cutoffMin - cutoff.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep

@Dieterbe
Copy link
Contributor Author

That's great. Should we add such a parameter to /metrics/index.json as well?

why? is there a use case for it?

@Dieterbe Dieterbe force-pushed the mt-index-cat-min-age branch from ac8fbb6 to c31344b Compare September 27, 2018 07:27
@Dieterbe
Copy link
Contributor Author

PTAL

@woodsaj
Copy link
Member

woodsaj commented Sep 27, 2018

i am confused by what these min/max age settings actually do. The descriptions of the options are not very clear. I feel that the term "age" is not appropriate for these settings. Age suggests that it is the time since the series was created, which is not what it does.

max-age seems to be the equivalent of max-stale setting in MT which i think is a better name and has a much clearer description.

So i think we should change these names and descriptions.

max-stale: exclude series that have not been seen for this much time.
min-stale: exclude series that have been seen in this much time

@Dieterbe
Copy link
Contributor Author

yes! something also didn't feel quite right to me, and you phrased it very well.

@Dieterbe
Copy link
Contributor Author

mergey mergey?

@Dieterbe Dieterbe merged commit 1bda4ea into master Sep 28, 2018
@woodsaj woodsaj deleted the mt-index-cat-min-age branch September 28, 2018 14:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants