Skip to content
Merged
Show file tree
Hide file tree
Changes from 22 commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
0178776
feat(reexecution/c): decouple metrics server and collector
RodrigoVillar Oct 12, 2025
4ad3f2b
Merge branch 'master' into rodrigo/decouple-reexecution-metrics
RodrigoVillar Oct 13, 2025
fc83c89
Merge branch 'master' into rodrigo/decouple-reexecution-metrics
RodrigoVillar Oct 13, 2025
848c6ad
docs: improve collectRegistry
RodrigoVillar Oct 13, 2025
be761ac
chore: set default to empty string
RodrigoVillar Oct 13, 2025
ca0b993
docs: benchmark script
RodrigoVillar Oct 13, 2025
a7cb056
chore: simplify metricsMode
RodrigoVillar Oct 13, 2025
d36d4ca
Merge branch 'master' into rodrigo/decouple-reexecution-metrics
RodrigoVillar Oct 13, 2025
c7f3185
chore: unexport metricsMode
RodrigoVillar Oct 13, 2025
9e319d0
chore: clean up
RodrigoVillar Oct 13, 2025
2cc0a79
chore: self-review nits
RodrigoVillar Oct 14, 2025
feb1681
feat(reexecute/c): explicit metrics port
RodrigoVillar Oct 14, 2025
c7c83bb
Merge branch 'master' into rodrigo/support-explicit-metrics-port
RodrigoVillar Oct 20, 2025
a5d4392
chore: nits
RodrigoVillar Oct 20, 2025
4ff8126
Merge branch 'master' into rodrigo/support-explicit-metrics-port
RodrigoVillar Oct 20, 2025
c67da7f
chore: nits
RodrigoVillar Oct 20, 2025
f5e2e14
chore: unexport consts
RodrigoVillar Oct 20, 2025
d871c75
chore: README
RodrigoVillar Oct 20, 2025
540d60f
chore: nit
RodrigoVillar Oct 20, 2025
bca6d48
chore: nit
RodrigoVillar Oct 20, 2025
e381c2c
Merge branch 'master' into rodrigo/support-explicit-metrics-port
RodrigoVillar Oct 20, 2025
e5283df
Merge branch 'master' into rodrigo/support-explicit-metrics-port
RodrigoVillar Oct 28, 2025
ac02cbf
docs: func name
RodrigoVillar Oct 28, 2025
4654206
chore: unify server flags
RodrigoVillar Oct 28, 2025
d18bbb8
chore: nit
RodrigoVillar Oct 28, 2025
df721f1
fix: quote
RodrigoVillar Oct 28, 2025
527b652
feat: implicitly enable metrics server if collector is enabled
RodrigoVillar Oct 28, 2025
f2fc794
chore: nits
RodrigoVillar Oct 28, 2025
f156a76
docs: README
RodrigoVillar Oct 28, 2025
5bb04be
chore: nit
RodrigoVillar Oct 28, 2025
d0b0083
docs: flag usage
RodrigoVillar Oct 28, 2025
6796474
Merge branch 'master' into rodrigo/support-explicit-metrics-port
RodrigoVillar Oct 28, 2025
830e8c1
chore: nits
RodrigoVillar Oct 28, 2025
3651f47
chore: revert unification of metrics server param
RodrigoVillar Oct 29, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions Taskfile.yml
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,7 @@ tasks:
BENCHMARK_OUTPUT_FILE: '{{.BENCHMARK_OUTPUT_FILE | default ""}}'
METRICS_SERVER_ENABLED: '{{.METRICS_SERVER_ENABLED | default "false"}}'
METRICS_COLLECTOR_ENABLED: '{{.METRICS_COLLECTOR_ENABLED | default "false"}}'
METRICS_SERVER_PORT: '{{.METRICS_SERVER_PORT}}'
cmd: |
CURRENT_STATE_DIR={{.CURRENT_STATE_DIR}} \
BLOCK_DIR={{.BLOCK_DIR}} \
Expand All @@ -220,6 +221,7 @@ tasks:
BENCHMARK_OUTPUT_FILE={{.BENCHMARK_OUTPUT_FILE}} \
METRICS_SERVER_ENABLED={{.METRICS_SERVER_ENABLED}} \
METRICS_COLLECTOR_ENABLED={{.METRICS_COLLECTOR_ENABLED}} \
METRICS_SERVER_PORT={{.METRICS_SERVER_PORT}} \
bash -x ./scripts/benchmark_cchain_range.sh

reexecute-cchain-range-with-copied-data:
Expand All @@ -236,6 +238,7 @@ tasks:
BENCHMARK_OUTPUT_FILE: '{{.BENCHMARK_OUTPUT_FILE | default ""}}'
METRICS_SERVER_ENABLED: '{{.METRICS_SERVER_ENABLED | default "false"}}'
METRICS_COLLECTOR_ENABLED: '{{.METRICS_COLLECTOR_ENABLED | default "false"}}'
METRICS_SERVER_PORT: '{{.METRICS_SERVER_PORT}}'
cmds:
- task: import-cchain-reexecute-range
vars:
Expand All @@ -254,6 +257,7 @@ tasks:
BENCHMARK_OUTPUT_FILE: '{{.BENCHMARK_OUTPUT_FILE}}'
METRICS_SERVER_ENABLED: '{{.METRICS_SERVER_ENABLED}}'
METRICS_COLLECTOR_ENABLED: '{{.METRICS_COLLECTOR_ENABLED}}'
METRICS_SERVER_PORT: '{{.METRICS_SERVER_PORT}}'

test-bootstrap-monitor-e2e:
desc: Runs bootstrap monitor e2e tests
Expand Down
4 changes: 3 additions & 1 deletion scripts/benchmark_cchain_range.sh
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ set -euo pipefail
# BENCHMARK_OUTPUT_FILE (optional): If set, benchmark output is also written to this file.
# METRICS_SERVER_ENABLED (optional): If set, enables the metrics server.
# METRICS_COLLECTOR_ENABLED (optional): If set, enables the metrics collector.
# METRICS_SERVER_PORT (optional): If set, determines the port the metrics server will listen to.

: "${BLOCK_DIR:?BLOCK_DIR must be set}"
: "${CURRENT_STATE_DIR:?CURRENT_STATE_DIR must be set}"
Expand All @@ -28,7 +29,8 @@ cmd="go test -timeout=0 -v -benchtime=1x -bench=BenchmarkReexecuteRange -run=^$
--end-block=\"${END_BLOCK}\" \
${LABELS:+--labels=\"${LABELS}\"} \
${METRICS_SERVER_ENABLED:+--metrics-server-enabled=\"${METRICS_SERVER_ENABLED}\"} \
${METRICS_COLLECTOR_ENABLED:+--metrics-collector-enabled=\"${METRICS_COLLECTOR_ENABLED}\"}"
${METRICS_COLLECTOR_ENABLED:+--metrics-collector-enabled=\"${METRICS_COLLECTOR_ENABLED}\"} \
${METRICS_SERVER_PORT:+--metrics-server-port=\"${METRICS_SERVER_PORT}\"}"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we change the ordering here and do a pass throughout to ensure that the metric server port param is next to metrics server enabled so that these are properly clustered together by the resource that they change?

Is it possible to use a single parameter rather than a metrics server port and metrics server enabled param? For example, we could use only the port param, treat an unset param as disabled, and then treat 0 or another port param as enabling it with the specified port.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to use a single parameter rather than a metrics server port and metrics server enabled param? For example, we could use only the port param, treat an unset param as disabled, and then treat 0 or another port param as enabling it with the specified port.

Yes, I've updated the PR to use a single parameter metrics-server-port as described above.

Could we change the ordering here and do a pass throughout to ensure that the metric server port param is next to metrics server enabled so that these are properly clustered together by the resource that they change?

Now that we have just one metrics server parameter, this comment is no longer applicable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update: as requested in #4418 (comment), I've reverted the unification of the metrics server flags: 3651f47


if [ -n "${BENCHMARK_OUTPUT_FILE:-}" ]; then
eval "$cmd" | tee "${BENCHMARK_OUTPUT_FILE}"
Expand Down
23 changes: 17 additions & 6 deletions tests/prometheus_server.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ package tests
import (
"context"
"errors"
"fmt"
"net"
"net/http"
"time"
Expand All @@ -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.
Expand All @@ -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)
}

// NewPrometheusServer 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) {
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{
Expand Down
1 change: 1 addition & 0 deletions tests/reexecute/c/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ If running locally, metrics collection can be customized via the following param

- `METRICS_SERVER_ENABLED`: starts a Prometheus server exporting VM metrics.
- `METRICS_COLLECTOR_ENABLED`: starts a Prometheus collector (if enabled, then `METRICS_SERVER_ENABLED` must be enabled as well).
- `METRICS_SERVER_PORT`: determines the port the metrics server will listen to (set to `0` by default)

When utilizing the metrics collector feature, follow the instructions in the e2e [README](../../e2e/README.md#monitoring) to set the required Prometheus environment variables.

Expand Down
9 changes: 7 additions & 2 deletions tests/reexecute/c/vm_reexecute_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ var (

metricsServerEnabledArg bool
metricsCollectorEnabledArg bool
metricsServerPort uint64

networkUUID string = uuid.NewString()
labels = map[string]string{
Expand Down Expand Up @@ -106,6 +107,7 @@ func TestMain(m *testing.M) {

flag.BoolVar(&metricsServerEnabledArg, "metrics-server-enabled", false, "Whether to enable the metrics server.")
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.Uint64Var(&metricsServerPort, "metrics-server-port", metricsServerPort, "Port which the metrics server will listen to")
flag.StringVar(&labelsArg, "labels", "", "Comma separated KV list of metric labels to attach to all exported metrics. Ex. \"owner=tim,runner=snoopy\"")

predefinedConfigKeys := slices.Collect(maps.Keys(predefinedConfigs))
Expand Down Expand Up @@ -159,6 +161,7 @@ func BenchmarkReexecuteRange(b *testing.B) {
chanSizeArg,
metricsServerEnabledArg,
metricsCollectorEnabledArg,
metricsServerPort,
)
})
}
Expand All @@ -173,6 +176,7 @@ func benchmarkReexecuteRange(
chanSize int,
metricsServerEnabled bool,
metricsCollectorEnabled bool,
metricsPort uint64,
) {
r := require.New(b)
ctx := b.Context()
Expand All @@ -192,7 +196,7 @@ func benchmarkReexecuteRange(
log := tests.NewDefaultLogger("c-chain-reexecution")

if metricsServerEnabled {
serverAddr := startServer(b, log, prefixGatherer)
serverAddr := startServer(b, log, prefixGatherer, metricsPort)

if metricsCollectorEnabled {
startCollector(b, log, "c-chain-reexecution", labels, serverAddr)
Expand Down Expand Up @@ -565,10 +569,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",
Expand Down
Loading