STORY-25143 - Add prometheus metrics to smokescreen #1
Conversation
…lag to prometheus config
README.md
Outdated
| @@ -59,6 +59,10 @@ Here are the options you can give Smokescreen: | |||
| --deny-address value Add IP[:PORT] to list of blocked IPs. Repeatable. | |||
| --allow-address value Add IP[:PORT] to list of allowed IPs. Repeatable. | |||
| --egress-acl-file FILE Validate egress traffic against FILE | |||
| --prometheus-endpoint ENDPOINT Expose Prometheus metrics on the given endpoint. | |||
There was a problem hiding this comment.
Is the default endpoint /metrics?
There was a problem hiding this comment.
I had gone for requiring this to be set to enable prometheus, so not having any default value.
I'd been on the fence with having an --expose-prometheus flag which doesn't take a value, and then keep --prometheus-endpoint with a default value of /metrics which may be cleaner?
* Don't export metrics list * Follow project sytlistic choices
README.md
Outdated
| @@ -59,6 +59,10 @@ Here are the options you can give Smokescreen: | |||
| --deny-address value Add IP[:PORT] to list of blocked IPs. Repeatable. | |||
| --allow-address value Add IP[:PORT] to list of allowed IPs. Repeatable. | |||
| --egress-acl-file FILE Validate egress traffic against FILE | |||
| --prometheus-endpoint ENDPOINT Expose Prometheus metrics on the given endpoint. | |||
There was a problem hiding this comment.
does ENDPOINT default to /metrics? I'm a bit confused by some of the wording here.
There was a problem hiding this comment.
@alex-kalinowski: #1 (comment) <- I'm happy to add a --enable-prometheus flag, and have the --prometheus-endpoint default to /metrics - it's probably the cleanest way to structure the config.
| cli.StringFlag{ | ||
| Name: "prometheus-endpoint", | ||
| Usage: "Expose metrics via prometheus on `ENDPOINT`.", | ||
| }, |
There was a problem hiding this comment.
Maybe this is just how my brain works with cli args but can we default this to /metrics and add an additional flag to the effect of metric-type where you would choose statsd and/or prometheus and then ride the defaults if needed?
As of now the default is always shipping statsd metrics. Might want to provide a toggle to flip those off.
There was a problem hiding this comment.
@alex-kalinowski, I've pushed a change to add the --expose-prometheus-metrics flag to toggle exposing prometheus metrics.
Currently the default isn't shipping statsd metrics? A default flag is set for statsd-address, but unless the flag is explicitly passed in, IsSet("statsd-address") will return false, and the SetupStatsd() method won't be called.
There was a problem hiding this comment.
I assumed IsSet() returned true for defaults as well.
…ggle exposing prometheus metrics
No description provided.