atecontroller: the :8080 metrics listener follows OTEL_METRICS_EXPORTER by ericcurtin · Pull Request #1976 · agent-substrate/substrate

Eric Curtin (ericcurtin) · 2026-09-29T10:11:21Z

The :8080 listener follows OTEL_METRICS_EXPORTER, per the policy in #2067. Unset keeps push and :8080, otlp closes :8080, prometheus serves only, none does neither.

Fixes #933

Tests pass
Appropriate changes to documentation are included in the PR

Its registry leaves over OTLP, so nothing scrapes :8080. Drop the Service and port too.

Eric Curtin (ericcurtin) · 2026-10-01T02:32:07Z

Jeff Luo (@JeffLuoo) Lior Lieberman (@LiorLieberman) PTAL when you get a chance. Thank you!

Jeff Luo (JeffLuoo) · 2026-10-02T20:30:08Z

Thank you for this PR, and for the cleanup in managerOptions.

I suggest that we hold this PR until #2067 decides the metrics policy.

Eric Curtin (ericcurtin) · 2026-10-02T21:36:50Z

Sounds good, holding until #2067 is decided. Will rebase after.

Eric Curtin (ericcurtin) · 2026-10-03T09:14:49Z

Merged main and resolved the conflict. Still holding for #2067.

Jeff Luo (JeffLuoo) · 2026-10-06T14:25:13Z

Thank you for holding this. #2067 has decided the policy, and task 3 there replaces this PR. :8080 must follow OTEL_METRICS_EXPORTER, not always turn off. There are multiple PRs at the moment trying to add a function to parse OTel standard environment variables. I will suggest still hold until the env var parsing part is done, and then rework this PR.

Eric Curtin (ericcurtin) · 2026-10-08T01:21:59Z

Jeff Luo (@JeffLuoo) Understood, thanks. I will hold until the OTel env var parsing lands and then rework this.

Jeff Luo (JeffLuoo) · 2026-10-08T13:51:40Z

Eric Curtin (@ericcurtin)

Thank you for waiting. Here is the rework, based on the policy in #2067. Please rebase after #2190 merges. That PR moves OTEL_METRICS_EXPORTER to the shared parser and makes prometheus a known name.

Jeff Luo (JeffLuoo) · 2026-10-08T15:30:48Z

The change I mentioned is merged.

Eric Curtin (ericcurtin) · 2026-10-08T21:32:39Z

Jeff Luo (@JeffLuoo) Reworked on top of #2190, PTAL.

Jeff Luo (JeffLuoo)

The implementation and tests look good. Two comments on docs/observability.md inline.

Jeff Luo (JeffLuoo) · 2026-10-08T22:11:08Z

 ### Scraping instead of pushing

-ateapi, atelet, atenet-router and the credential provider also serve every instrument on their Prometheus `/metrics` endpoint. A cluster that scrapes those endpoints sets `OTEL_METRICS_EXPORTER=none` on the components, so each series reaches the backend once. With `none` the components install no OTLP metric reader and keep the Prometheus one; traces and logs are unaffected. atecontroller then registers its instruments (`ate.workerpool.*`) on controller-runtime's registry, so the manager's `:8080` serves them next to the controller-runtime families. ateom serves no endpoint of its own, so leave the variable unset on the worker pods, or it exports no metrics at all. The variable takes a comma-separated list, with the same rules as `OTEL_LOGS_EXPORTER`. `otlp` (the default) keeps the push, and `prometheus` or `none` stops it. `/metrics` stays on for each value. ateom serves no `/metrics`, so there `prometheus` is skipped as unknown with a warning, and the push stays on unless the variable is `none`.
+ateapi, atelet, atenet-router and the credential provider also serve every instrument on their Prometheus `/metrics` endpoint. A cluster that scrapes those endpoints sets `OTEL_METRICS_EXPORTER=prometheus` on the components, so each series reaches the backend once. With `prometheus` the components install no OTLP metric reader and keep the Prometheus one; traces and logs are unaffected. `none` stops the push too, and leaves their `/metrics` on. ateom serves no endpoint of its own, so leave the variable unset on the worker pods, or it exports no metrics at all. The variable takes a comma-separated list, with the same rules as `OTEL_LOGS_EXPORTER`. `otlp` (the default) keeps the push, and `prometheus` or `none` stops it. ateom serves no `/metrics`, so there `prometheus` is skipped as unknown with a warning, and the push stays on unless the variable is `none`.


Two stale details in this paragraph:

"ateom serves no endpoint of its own, so leave the variable unset on the worker pods, or it exports no metrics at all" was written when none was the scrape-only setting. Two sentences later, this paragraph notes that ateom skips prometheus as unknown and keeps the push on unless the variable is none (and atecontroller does not propagate OTEL_METRICS_EXPORTER to worker pods). Please remove that sentence or scope it to none.

"otlp (the default) keeps the push" — with metricsExporters setting def = Exporters{ExporterOTLP: true, exporterPrometheus: true} for components that serve /metrics, unset defaults to otlp,prometheus (otlp only on ateom), which matters because the next paragraph contrasts Unset with otlp.

Fixed in a9ea0d7, thanks.

Jeff Luo (JeffLuoo) · 2026-10-08T22:11:08Z

-ateapi, atelet, atenet-router and the credential provider also serve every instrument on their Prometheus `/metrics` endpoint. A cluster that scrapes those endpoints sets `OTEL_METRICS_EXPORTER=none` on the components, so each series reaches the backend once. With `none` the components install no OTLP metric reader and keep the Prometheus one; traces and logs are unaffected. atecontroller then registers its instruments (`ate.workerpool.*`) on controller-runtime's registry, so the manager's `:8080` serves them next to the controller-runtime families. ateom serves no endpoint of its own, so leave the variable unset on the worker pods, or it exports no metrics at all. The variable takes a comma-separated list, with the same rules as `OTEL_LOGS_EXPORTER`. `otlp` (the default) keeps the push, and `prometheus` or `none` stops it. `/metrics` stays on for each value. ateom serves no `/metrics`, so there `prometheus` is skipped as unknown with a warning, and the push stays on unless the variable is `none`.
+ateapi, atelet, atenet-router and the credential provider also serve every instrument on their Prometheus `/metrics` endpoint. A cluster that scrapes those endpoints sets `OTEL_METRICS_EXPORTER=prometheus` on the components, so each series reaches the backend once. With `prometheus` the components install no OTLP metric reader and keep the Prometheus one; traces and logs are unaffected. `none` stops the push too, and leaves their `/metrics` on. ateom serves no endpoint of its own, so leave the variable unset on the worker pods, or it exports no metrics at all. The variable takes a comma-separated list, with the same rules as `OTEL_LOGS_EXPORTER`. `otlp` (the default) keeps the push, and `prometheus` or `none` stops it. ateom serves no `/metrics`, so there `prometheus` is skipped as unknown with a warning, and the push stays on unless the variable is `none`.
+
+atecontroller's `:8080` listener follows the variable. Unset, or a list with both, it pushes and serves; `otlp` pushes and closes the listener; `prometheus` serves only, and registers its instruments (`ate.workerpool.*`) on controller-runtime's registry, so `:8080` serves them next to the controller-runtime families; `none` does neither.


When the variable is unset or otlp,prometheus, :8080 is open but does not serve the ate.workerpool.* instruments: because OTLP push is enabled, InitMetricsBridged keeps the OTel instruments off ctrlmetrics.Registry to avoid pushing a second copy through the bridge, and only registers them on :8080 under prometheus alone. Saying "it pushes and serves" makes a reader expect the full set on :8080 in the default mode. Please state which metrics :8080 serves in each mode:

Suggested change

atecontroller's `:8080` listener follows the variable. Unset, or a list with both, it pushes and serves; `otlp` pushes and closes the listener; `prometheus` serves only, and registers its instruments (`ate.workerpool.*`) on controller-runtime's registry, so `:8080` serves them next to the controller-runtime families; `none` does neither.

atecontroller's `:8080` listener follows the variable. When the variable is unset or `otlp,prometheus`, atecontroller pushes every metric over OTLP, and `:8080` serves only the controller-runtime families. With `otlp`, atecontroller pushes every metric and closes `:8080`. With `prometheus`, atecontroller does not push, and `:8080` serves the controller-runtime families and the `ate.workerpool.*` instruments. With `none`, atecontroller does not push and closes `:8080`.

Fixed in a9ea0d7, thanks.

Krisztian F (krisztianfekete) · 2026-10-09T08:57:24Z

 }

+// servePull follows the policy table: unset and the list keep both paths.
+func TestInitMetricsBridgedServePull(t *testing.T) {


Please add a name field. The empty value row currently shows up as subtest #00.

Fixed in a46d59d, thanks.

Krisztian F (krisztianfekete) · 2026-10-09T08:59:13Z

+	exporters := metricsExporters(ctx, true)
+	servePull = exporters.Has(exporterPrometheus)
+	switch {
+	case exporters.Has(ExporterOTLP):


In this branch :8080 misses ate.workerpool.*, so unset and otlp,prometheus are not following the policy. This can be a follow-up, but please open an issue to track this in that case.

Tracked in #2402, thanks. A fix needs a separate registry to avoid pushing them twice.

Krisztian F (krisztianfekete) · 2026-10-09T08:59:55Z

+	// own Prometheus registry. OTEL_METRICS_EXPORTER picks where they go: the
+	// OTLP push, which pads the bridged queue histograms so the Telemetry API
+	// accepts idle ones, and the manager's scrape listener.
+	mp, servePull, err := serverboot.InitMetricsBridged(ctx, serviceName, ctrlmetrics.Registry, padEmptyExponentialHistograms)


Let's log the resolved exporters and whether :8080 is on.

Fixed in a46d59d, thanks.

Krisztian F (krisztianfekete) · 2026-10-09T09:01:05Z

+func managerOptions(egressMITMCAPool types.NamespacedName, servePull bool) ctrl.Options {
+	metricsAddr := "0" // "0" disables the server.
+	if servePull {
+		metricsAddr = metricsserver.DefaultBindAddress


Nit: we could use 8080 as a const that we can reference in tests.

Fixed in a46d59d, thanks.

Krisztian F (krisztianfekete) · 2026-10-09T09:01:41Z

+	for _, servePull := range []bool{true, false} {
+		opts := managerOptions(types.NamespacedName{Namespace: "ate-system", Name: "pool"}, servePull)


Let's rewrite as a tests := []struct{...} table with t.Run, following the repo's convention.

Fixed in a46d59d, thanks.

Krisztian F (krisztianfekete) · 2026-10-09T09:04:32Z

 ### Scraping instead of pushing

-ateapi, atelet, atenet-router and the credential provider also serve every instrument on their Prometheus `/metrics` endpoint. A cluster that scrapes those endpoints sets `OTEL_METRICS_EXPORTER=none` on the components, so each series reaches the backend once. With `none` the components install no OTLP metric reader and keep the Prometheus one; traces and logs are unaffected. atecontroller then registers its instruments (`ate.workerpool.*`) on controller-runtime's registry, so the manager's `:8080` serves them next to the controller-runtime families. ateom serves no endpoint of its own, so leave the variable unset on the worker pods, or it exports no metrics at all. The variable takes a comma-separated list, with the same rules as `OTEL_LOGS_EXPORTER`. `otlp` (the default) keeps the push, and `prometheus` or `none` stops it. `/metrics` stays on for each value. ateom serves no `/metrics`, so there `prometheus` is skipped as unknown with a warning, and the push stays on unless the variable is `none`.
+ateapi, atelet, atenet-router and the credential provider also serve every instrument on their Prometheus `/metrics` endpoint. A cluster that scrapes those endpoints sets `OTEL_METRICS_EXPORTER=prometheus` on the components, so each series reaches the backend once. With `prometheus` the components install no OTLP metric reader and keep the Prometheus one; traces and logs are unaffected. `none` stops the push too, and leaves their `/metrics` on. The variable takes a comma-separated list, with the same rules as `OTEL_LOGS_EXPORTER`. Unset is `otlp,prometheus`, or `otlp` alone on ateom. `otlp` keeps the push, and `prometheus` or `none` stops it. ateom serves no `/metrics`, so there `prometheus` is skipped as unknown with a warning, and the push stays on unless the variable is `none`.


This conflicts with what we discussed over at #2067 where none means no /metrics. Please it as temporary until we move the probes off :9090, and link #2067, so readers don't take it as the policy.

Fixed in a46d59d, thanks.

Name the table rows, make :8080 a const, log the resolved exporters, and mark the /metrics note temporary (agent-substrate#2067).

Eric Curtin (ericcurtin) · 2026-10-09T10:39:43Z

Jeff Luo (@JeffLuoo) Krisztian F (@krisztianfekete) Addressed the review, PTAL when you get a chance. Thank you!

Jeff Luo (JeffLuoo)

Just a few more nits

Jeff Luo (JeffLuoo) · 2026-10-09T15:39:20Z

Here it still says "which the manager serves on an unscraped :8080". Now that :8080 is disabled under otlp/none and scraped under prometheus, could we update the text here to match line 338?

Fixed in 5529baa, thanks.

Jeff Luo (JeffLuoo) · 2026-10-09T15:40:32Z

 		serverboot.Fatal(ctx, "Failed to initialize metrics", err)
 	}
 	defer serverboot.ShutdownProvider("MeterProvider", mp.Shutdown)
+	slog.InfoContext(ctx, "Metrics listener", slog.Bool("enabled", servePull), slog.String("addr", metricsPullAddr))


When servePull is false, managerOptions binds to metricsOffAddr ("0"), so logging addr=":8080" alongside enabled=false is misleading. Consider logging the resolved opts.Metrics.BindAddress (or omitting addr when disabled).

Fixed in 5529baa, logs the resolved address.

Jeff Luo (JeffLuoo) · 2026-10-09T15:41:32Z

 	}
-	if metricsPushEnabled(ctx, true) {
+	exporters := metricsExporters(ctx, true)
+	slog.InfoContext(ctx, "Metrics exporters resolved", slog.String("exporters", exporters.String()))


nit - if we move this log into metricsExporters(ctx) (or newMeterProvider), InitMetrics and InitMetricsPushOnlyVia will also log the resolved exporters at startup, not just atecontroller.

Fixed in 5529baa, moved into metricsExporters.

…l components

Eric Curtin (ericcurtin) · 2026-10-09T20:32:43Z

Jeff Luo (@JeffLuoo) Krisztian F (@krisztianfekete) Addressed, PTAL. The e2e failure was httpbin.org returning EOF, unrelated; the new push reruns it.

atecontroller: disable the unused metrics listener

19344a9

Its registry leaves over OTLP, so nothing scrapes :8080. Drop the Service and port too.

Eric Curtin (ericcurtin) mentioned this pull request Sep 29, 2026

atecontroller metrics are never scraped #933

Open

Julian Gutierrez Oschmann (juli4n) requested a review from Jeff Luo (JeffLuoo) September 29, 2026 22:39

Maya Wang (mayawang) added area/observability kind/cleanup Small fixes that are not bugs, for example a typo in a code comment labels Sep 29, 2026

Jeff Luo (JeffLuoo) mentioned this pull request Oct 1, 2026

System components use different metrics paths #2067

Open

Merge main

8b4fcb8

Eric Curtin (ericcurtin) added 2 commits October 8, 2026 22:29

Merge remote-tracking branch 'origin/main' into ctrl-wt

76050b9

atecontroller: follow OTEL_METRICS_EXPORTER for the :8080 listener

024ce51

Eric Curtin (ericcurtin) changed the title ~~atecontroller: disable the unused metrics listener~~ atecontroller: the :8080 metrics listener follows OTEL_METRICS_EXPORTER Oct 8, 2026

Jeff Luo (JeffLuoo) reviewed Oct 8, 2026

View reviewed changes

docs: describe the :8080 listener per OTEL_METRICS_EXPORTER mode

a9ea0d7

Krisztian F (krisztianfekete) reviewed Oct 9, 2026

View reviewed changes

Eric Curtin (ericcurtin) mentioned this pull request Oct 9, 2026

atecontroller: serve ate.workerpool.* on :8080 when also pushing #2402

Open

atecontroller: address review on the :8080 listener

a46d59d

Name the table rows, make :8080 a const, log the resolved exporters, and mark the /metrics note temporary (agent-substrate#2067).

Jeff Luo (JeffLuoo) reviewed Oct 9, 2026

View reviewed changes

atecontroller: log the resolved metrics address, log exporters for al…

5529baa

…l components

		for _, servePull := range []bool{true, false} {
		opts := managerOptions(types.NamespacedName{Namespace: "ate-system", Name: "pool"}, servePull)

Conversation

Eric Curtin (ericcurtin) commented Sep 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Eric Curtin (ericcurtin) commented Oct 1, 2026

Uh oh!

Jeff Luo (JeffLuoo) commented Oct 2, 2026

Uh oh!

Eric Curtin (ericcurtin) commented Oct 2, 2026

Uh oh!

Eric Curtin (ericcurtin) commented Oct 3, 2026

Uh oh!

Jeff Luo (JeffLuoo) commented Oct 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Eric Curtin (ericcurtin) commented Oct 8, 2026

Uh oh!

Jeff Luo (JeffLuoo) commented Oct 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Jeff Luo (JeffLuoo) commented Oct 8, 2026

Uh oh!

Eric Curtin (ericcurtin) commented Oct 8, 2026

Uh oh!

Jeff Luo (JeffLuoo) left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Eric Curtin (ericcurtin) commented Oct 9, 2026

Uh oh!

Jeff Luo (JeffLuoo) left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Eric Curtin (ericcurtin) commented Oct 9, 2026

Uh oh!

Reviewers

Assignees

Eric Curtin (ericcurtin) commented Sep 29, 2026 •

edited

Loading

Jeff Luo (JeffLuoo) commented Oct 6, 2026 •

edited

Loading

Jeff Luo (JeffLuoo) commented Oct 8, 2026 •

edited

Loading