-
Couldn't load subscription status.
- Fork 813
feat(reexecute/c): explicit metrics port #4418
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
base: master
Are you sure you want to change the base?
Changes from all commits
0178776
4ad3f2b
fc83c89
848c6ad
be761ac
ca0b993
a7cb056
d36d4ca
c7f3185
9e319d0
2cc0a79
feb1681
c7c83bb
a5d4392
4ff8126
c67da7f
f5e2e14
d871c75
540d60f
bca6d48
e381c2c
e5283df
ac02cbf
4654206
d18bbb8
df721f1
527b652
f2fc794
f156a76
5bb04be
d0b0083
6796474
830e8c1
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 |
|---|---|---|
|
|
@@ -6,6 +6,7 @@ package tests | |
| import ( | ||
| "context" | ||
| "errors" | ||
| "fmt" | ||
| "net" | ||
| "net/http" | ||
| "time" | ||
|
|
@@ -14,7 +15,10 @@ import ( | |
| "github.com/prometheus/client_golang/prometheus/promhttp" | ||
| ) | ||
|
|
||
| const defaultPrometheusListenAddr = "127.0.0.1:0" | ||
| const ( | ||
| localhostAddr = "127.0.0.1" | ||
| defaultMetricsPort = 0 | ||
| ) | ||
|
|
||
| // PrometheusServer is a HTTP server that serves Prometheus metrics from the provided | ||
| // gahterer. | ||
|
|
@@ -28,25 +32,32 @@ type PrometheusServer struct { | |
| // NewPrometheusServer creates and starts a Prometheus server with the provided gatherer | ||
| // listening on 127.0.0.1:0 and serving /ext/metrics. | ||
| func NewPrometheusServer(gatherer prometheus.Gatherer) (*PrometheusServer, error) { | ||
| return NewPrometheusServerWithPort(gatherer, defaultMetricsPort) | ||
| } | ||
|
|
||
| // NewPrometheusServerWithPort creates and starts a Prometheus server with the provided gatherer | ||
| // listening on 127.0.0.1:port and serving /ext/metrics. | ||
| func NewPrometheusServerWithPort(gatherer prometheus.Gatherer, port uint64) (*PrometheusServer, error) { | ||
|
Comment on lines
+38
to
+40
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. Consider using functional options pattern here, but think this is fine since it's just adding one extra constructor. 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 would prefer the additional constructor for the following reasons:
|
||
| server := &PrometheusServer{ | ||
| gatherer: gatherer, | ||
| } | ||
|
|
||
| if err := server.start(); err != nil { | ||
| serverAddress := fmt.Sprintf("%s:%d", localhostAddr, port) | ||
| if err := server.start(serverAddress); err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return server, nil | ||
| } | ||
|
|
||
| // start the Prometheus server on a dynamic port. | ||
| func (s *PrometheusServer) start() error { | ||
| // start the Prometheus server on address. | ||
| func (s *PrometheusServer) start(address string) error { | ||
| mux := http.NewServeMux() | ||
| mux.Handle("/ext/metrics", promhttp.HandlerFor(s.gatherer, promhttp.HandlerOpts{})) | ||
|
|
||
| listener, err := net.Listen("tcp", defaultPrometheusListenAddr) | ||
| listener, err := net.Listen("tcp", address) | ||
| if err != nil { | ||
| return err | ||
| return fmt.Errorf("failed to listen on %s: %w", address, err) | ||
| } | ||
|
|
||
| s.server = http.Server{ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -63,7 +63,7 @@ var ( | |
| executionTimeout time.Duration | ||
| labelsArg string | ||
|
|
||
| metricsServerEnabledArg bool | ||
| metricsServerPort *uint64 | ||
| metricsCollectorEnabledArg bool | ||
|
|
||
| networkUUID string = uuid.NewString() | ||
|
|
@@ -104,7 +104,16 @@ func TestMain(m *testing.M) { | |
| flag.IntVar(&chanSizeArg, "chan-size", 100, "Size of the channel to use for block processing.") | ||
| flag.DurationVar(&executionTimeout, "execution-timeout", 0, "Benchmark execution timeout. After this timeout has elapsed, terminate the benchmark without error. If 0, no timeout is applied.") | ||
|
|
||
| flag.BoolVar(&metricsServerEnabledArg, "metrics-server-enabled", false, "Whether to enable the metrics server.") | ||
| flag.Func("metrics-server-port", "Starts a metrics server and sets the port it will listen to", func(s string) error { | ||
| port, err := strconv.ParseUint(s, 10, 64) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| metricsServerPort = new(uint64) | ||
| *metricsServerPort = port | ||
| return nil | ||
| }) | ||
|
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. Isn't there a way to simply check if a flag is set without using a Func like this? 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. If we keep Edit: as to the usage of flag.Uint64Var(metricsServerPort, "metrics-server-port", 0, "Starts a metrics server and sets the port it will listen to")results in a panic as |
||
| flag.BoolVar(&metricsCollectorEnabledArg, "metrics-collector-enabled", false, "Whether to enable the metrics collector (if true, then metrics-server-enabled must be true as well).") | ||
| flag.StringVar(&labelsArg, "labels", "", "Comma separated KV list of metric labels to attach to all exported metrics. Ex. \"owner=tim,runner=snoopy\"") | ||
|
|
||
|
|
@@ -119,9 +128,8 @@ func TestMain(m *testing.M) { | |
|
|
||
| flag.Parse() | ||
|
|
||
| if metricsCollectorEnabledArg && !metricsServerEnabledArg { | ||
| fmt.Fprint(os.Stderr, "metrics collector is enabled but metrics server is disabled.\n") | ||
| os.Exit(1) | ||
| if metricsCollectorEnabledArg && metricsServerPort == nil { | ||
| metricsServerPort = new(uint64) | ||
RodrigoVillar marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| customLabels, err := parseCustomLabels(labelsArg) | ||
|
|
@@ -157,7 +165,7 @@ func BenchmarkReexecuteRange(b *testing.B) { | |
| startBlockArg, | ||
| endBlockArg, | ||
| chanSizeArg, | ||
| metricsServerEnabledArg, | ||
| metricsServerPort, | ||
| metricsCollectorEnabledArg, | ||
| ) | ||
| }) | ||
|
|
@@ -171,7 +179,7 @@ func benchmarkReexecuteRange( | |
| startBlock uint64, | ||
| endBlock uint64, | ||
| chanSize int, | ||
| metricsServerEnabled bool, | ||
| metricsPort *uint64, | ||
| metricsCollectorEnabled bool, | ||
| ) { | ||
| r := require.New(b) | ||
|
|
@@ -191,8 +199,8 @@ func benchmarkReexecuteRange( | |
|
|
||
| log := tests.NewDefaultLogger("c-chain-reexecution") | ||
|
|
||
| if metricsServerEnabled { | ||
| serverAddr := startServer(b, log, prefixGatherer) | ||
| if metricsPort != nil { | ||
| serverAddr := startServer(b, log, prefixGatherer, *metricsPort) | ||
|
|
||
| if metricsCollectorEnabled { | ||
| startCollector(b, log, "c-chain-reexecution", labels, serverAddr) | ||
|
|
@@ -565,10 +573,11 @@ func startServer( | |
| tb testing.TB, | ||
| log logging.Logger, | ||
| gatherer prometheus.Gatherer, | ||
| port uint64, | ||
| ) string { | ||
| r := require.New(tb) | ||
|
|
||
| server, err := tests.NewPrometheusServer(gatherer) | ||
| server, err := tests.NewPrometheusServerWithPort(gatherer, port) | ||
| r.NoError(err) | ||
|
|
||
| log.Info("metrics endpoint available", | ||
|
|
||
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 would like to see
METRICS_SERVER_PORTbe in addition toMETRICS_SERVER_ENABLEDrather than its replacement. The default port should remain zero - dynamic - and that detail shouldn't be required knowledge for those who don't want to set a specific port.