Add a separate flag for 'start' parameter#136
Add a separate flag for 'start' parameter#136DirectXMan12 merged 2 commits intokubernetes-sigs:masterfrom
Conversation
nilebox
force-pushed
the
start-flag
branch
2 times, most recently
from
5ba0ad0 to
73cc357
Compare
|
Can you add something like the description of #135 to the description of this PR? Someone doing |
|
@DirectXMan12 done, please review. |
DirectXMan12
left a comment
DirectXMan12
left a comment
There was a problem hiding this comment.
generally on board with this, couple comments inline
cmd/adapter/adapter.go
Outdated
| // MetricsRelistInterval is the interval at which to relist the set of available metrics | ||
| MetricsRelistInterval time.Duration | ||
| // MetricsStartDuration is the period to query available metrics for | ||
| MetricsStartDuration time.Duration |
There was a problem hiding this comment.
nit: this name is not immediately obvious. Perhaps max-metrics-age or something
There was a problem hiding this comment.
Renamed to metrics-max-age to be consistent with existing metrics-relist-interval flag name.
cmd/adapter/adapter.go
Outdated
| "and custom metrics API resources") | ||
| cmd.Flags().DurationVar(&cmd.MetricsRelistInterval, "metrics-relist-interval", cmd.MetricsRelistInterval, ""+ | ||
| "interval at which to re-list the set of all available metrics from Prometheus") | ||
| cmd.Flags().DurationVar(&cmd.MetricsStartDuration, "metrics-start-duration", cmd.MetricsStartDuration, ""+ |
There was a problem hiding this comment.
can we default this to something sane based the metrics-relist-interval so that we don't confuse people who are expecting the existing behavior? e.g. if someone sets metrics-relist-interval to 20 minutes, they probably don't expect the max metrics age to be 10 minutes, and if someone sets it to 30 seconds, they probably also don't expect the age to be 10 minutes. Maybe 2 * relist interval by default or something?
There was a problem hiding this comment.
Agreed that I can make it longer than metrics-relist-interval by default, i.e. 20m. That would suggest that max age should be at least twice as high as relist interval.
Why setting max age to 10m with relist interval 30s is bad though? I think it's perfectly fine actually :) e.g. even if Prometheus is configured to be super slow and refreshes metrics once every 10m, I still don't want to have a potential delay of 9m:59s once a new metric is discovered by Prometheus, so I will set it to 30s.
There was a problem hiding this comment.
@DirectXMan12 I can probably add a validation code that would require the max-age >= 2 * relist-interval?
There was a problem hiding this comment.
no validation needed. My main concern was that it means metrics that are no longer valid will show up in discovery for a lot longer. If you have things coming and going quickly, this could lead to a lot more metrics to process during discovery than you intend. It's not a big deal here, though, and we can always follow up later.
| } | ||
|
|
||
| if cmd.MetricsMaxAge < cmd.MetricsRelistInterval { | ||
| return nil, fmt.Errorf("max age must not be less than relist interval") |
There was a problem hiding this comment.
@DirectXMan12 added a validation, but without the 2 * multiplier, as I think it's a bit too opinionated.
e.g. if someone sets a relist interval to 5s, while Prometheus itself (internally) has an interval of 1m, even setting max age to 10s won't help, so users need to understand what they are doing.
|
This PR should be good to merge now. |
|
LGTM 👍 |
2 participants
Make a separate flag
metrics-max-agefor duration to be used forstartparameter sent to Prometheus, determining the set of available metrics appeared during a specified period. This will allow us to update a list of available metrics frequently (e.g. 30s or less) without running into flakiness of appearing/disappearing metrics.Fixes #135