New flag to disable detailed metrics for path #841
Conversation
main.go
Outdated
| @@ -97,6 +98,7 @@ func main() { | |||
| flag.BoolVar(&watchAllNamespaces, "watch-all-namespaces", true, | |||
| "Watch for custom resources in all namespaces, if set to false it will only watch the runtime namespace.") | |||
| flag.DurationVar(&rateLimitInterval, "rate-limit-interval", 5*time.Minute, "Interval in which rate limit has effect.") | |||
| flag.BoolVar(&detailedMetrics, "detailed-metrics", true, "Count metrics for every requested path") | |||
There was a problem hiding this comment.
I would name the flag --export-http-path-metrics and have it set to false by default. The description could be: When enabled, the requests full path is included in the HTTP server metrics (risk as high cardinality).
There was a problem hiding this comment.
it is the other way around.
maybe something like:
--set-metrics-handlerID and have it set to false by default. The description: When enabled, the requests full path is included in the HTTP server metrics as handler label (this will keep cardinality low because all urls path will have same handler label on the metrics).
Like this we keep the actual behavior by default.
There was a problem hiding this comment.
But I don't think we should keep the default behaviour, given that people expose the receiver on the internet, best is to ship with a secure default.
There was a problem hiding this comment.
I agree with @stefanprodan, but the only concern is if somebody have already build alerts system based on path labels that they have in their systems. Probability is low, but still this change would silently break metrics and affect monitoring for those who rely on this label. Probably we could keep current behaviour and switch to more secure on next major release with proper notification in release notes ?
There was a problem hiding this comment.
This PR will not go into a patch release anyway. It will be part of Flux v2.4 and we'll document the flag and the behaviour change accordingly.
|
@Kuzbekov can you please rebase and force push, we're getting close to Flux 2.4 release and this PR can now be merged. |
Flag detailed-metrics added to provide a way to disable exposing all accessed paths to the metrics and prevent potential metrics cardinality explosion Signed-off-by: Alexey Kuzbekov <[email protected]>
Signed-off-by: Alexey Kuzbekov <[email protected]>
done |
Flag detailed-metrics added to provide a way to disable exposing all accessed paths to the metrics and prevent potential metrics cardinality explosion
Issue ref: #828