-
Notifications
You must be signed in to change notification settings - Fork 0
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
STORY-25143 - Add prometheus metrics to smokescreen #1
base: master
Are you sure you want to change the base?
Changes from 10 commits
ad6735c
fd1048a
904e079
ed9fb19
7aabe5e
cda3fec
3f4e182
56d4547
e1c7ca9
aca8718
2ed11fb
c3379be
940217d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. does ENDPOINT default to /metrics? I'm a bit confused by some of the wording here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @alex-kalinowski: #1 (comment) <- I'm happy to add a |
||
If --prometheus-port isn't set, by default listens on 9810 | ||
--prometheus-port PORT Expose Prometheus metrics on the given port. | ||
Requires --prometheus-endpoint to be set. | ||
--resolver-address ADDRESS Make DNS requests to ADDRESS (IP:port). Repeatable. | ||
--statsd-address ADDRESS Send metrics to statsd at ADDRESS (IP:port). (default: "127.0.0.1:8200") | ||
--tls-server-bundle-file FILE Authenticate to clients using key and certs from FILE | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -85,6 +85,15 @@ func NewConfiguration(args []string, logger *log.Logger) (*smokescreen.Config, e | |
Name: "egress-acl-file", | ||
Usage: "Validate egress traffic against `FILE`", | ||
}, | ||
cli.StringFlag{ | ||
Name: "prometheus-endpoint", | ||
Usage: "Expose metrics via prometheus on `ENDPOINT`.", | ||
}, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @alex-kalinowski, I've pushed a change to add the Currently the default isn't shipping statsd metrics? A default flag is set for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I assumed IsSet() returned true for defaults as well. |
||
cli.StringFlag{ | ||
Name: "prometheus-port", | ||
Value: "9810", | ||
Usage: "Expose metrics via prometheus on `PORT`.", | ||
}, | ||
cli.StringSliceFlag{ | ||
Name: "resolver-address", | ||
Usage: "Make DNS requests to `ADDRESS` (IP:port). Repeatable.", | ||
|
@@ -229,6 +238,12 @@ func NewConfiguration(args []string, logger *log.Logger) (*smokescreen.Config, e | |
} | ||
} | ||
|
||
if c.IsSet("prometheus-endpoint") { | ||
if err := conf.SetupPrometheus(c.String("prometheus-endpoint"), c.String("prometheus-port")); err != nil { | ||
return err | ||
} | ||
} | ||
|
||
if c.IsSet("egress-acl-file") { | ||
if err := conf.SetupEgressAcl(c.String("egress-acl-file")); err != nil { | ||
return err | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,27 +1,36 @@ | ||
module github.com/stripe/smokescreen | ||
|
||
go 1.17 | ||
go 1.18 | ||
|
||
require ( | ||
github.com/DataDog/datadog-go v4.5.1+incompatible | ||
github.com/armon/go-proxyproto v0.0.0-20170620220930-48572f11356f | ||
github.com/carlmjohnson/versioninfo v0.22.4 | ||
github.com/hashicorp/go-cleanhttp v0.0.0-20171218145408-d5fe4b57a186 | ||
github.com/patrickmn/go-cache v2.1.0+incompatible | ||
github.com/prometheus/client_golang v1.13.0 | ||
github.com/rs/xid v1.2.1 | ||
github.com/sirupsen/logrus v1.7.0 | ||
github.com/stretchr/testify v1.8.0 | ||
github.com/stripe/goproxy v0.0.0-20220308202309-3f1dfba6d1a4 | ||
golang.org/x/net v0.0.0-20220812174116-3211cb980234 | ||
gopkg.in/urfave/cli.v1 v1.20.0 | ||
gopkg.in/yaml.v2 v2.2.8 | ||
gopkg.in/yaml.v2 v2.4.0 | ||
) | ||
|
||
require ( | ||
github.com/Microsoft/go-winio v0.4.17 // indirect | ||
github.com/beorn7/perks v1.0.1 // indirect | ||
github.com/cespare/xxhash/v2 v2.1.2 // indirect | ||
github.com/davecgh/go-spew v1.1.1 // indirect | ||
github.com/golang/protobuf v1.5.2 // indirect | ||
github.com/matttproud/golang_protobuf_extensions v1.0.1 // indirect | ||
github.com/pmezard/go-difflib v1.0.0 // indirect | ||
github.com/prometheus/client_model v0.2.0 // indirect | ||
github.com/prometheus/common v0.37.0 // indirect | ||
github.com/prometheus/procfs v0.8.0 // indirect | ||
golang.org/x/sys v0.0.0-20220728004956-3c1f35247d10 // indirect | ||
golang.org/x/text v0.3.7 // indirect | ||
google.golang.org/protobuf v1.28.1 // indirect | ||
gopkg.in/yaml.v3 v3.0.1 // indirect | ||
) |
Large diffs are not rendered by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the default endpoint
/metrics
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?