(refactor) UpDownCounter RAII guards by BrynCooke · Pull Request #8379 · apollographql/router

BrynCooke · 2025-10-06T21:48:45Z

This PR changes the nature of UpDownCounters within the router and update a few metrics.

UpDownCounters are mostly good for tracking resources. They transmit an aggregate value rather than a delta, so each updown instrument sends the latest value.

Previously we were manually incrementing and decrementing updown counters, however there is nuance to this:

An increment that is paired with a decrement must be done via RAII guard as there is no guarantee that a code path will be called when using rust async.
A paired decrement must be applied to the SAME instrument that the increment was called on. The reason for this is that if a hot reload happens a new instrument may be created so if the decrement occurs on the new instrument it will be an immediate negative value. Note that instruments are normally contained within an Arc to allow callsite invalidation, but from within a guard the inner instrument must be cloned.

As we don't actually use UpDown counters for anything else (and there seems to be little reason to) the macro has been modified to return a RAII guard that will automatically decrement by the value that was incremented when it is dropped. It also stores the original instrument that was used to increment so that it is guaranteed to decrement on the same instrument that it incremented on.

Notes

Each commit can be reviewed in isolation.
There are various other gauges that can be removed and converted to UpDownCounters to simplify the codebase but these can be done separately.
I've had to make the lifecycle tests more lenient as series that are now old prom series that are zeroed out present in the results. See below for reasoning.

Issues

It has been noticed that the Promethues registry will NEVER drop a series. This means that over time cardinality will build up and consume memory. I've put a hacky workaround e1430e1. This will cause prom to be reinitialized 1 in 10 reloads. This may mean a minor number of metrics lost, but it's better than just accumulating forever.

Promethues is deprecated in the latest version of otel rust, and this isn't a problem for otlp. In the next major version of the router we should consider removing Prometheus support.

Checklist

Complete the checklist (and note appropriate exceptions) before the PR is marked ready-for-review.

Exceptions

Note any exceptions here

Notes

It may be appropriate to bring upcoming changes to the attention of other (impacted) groups. Please endeavour to do this before seeking PR approval. The mechanism for doing this will vary considerably, so use your judgement as to how and when to do this. ↩
Configuration is an important part of many changes. Where applicable please try to document configuration examples. ↩
A lot of (if not most) features benefit from built-in observability and debug-level logs. Please read this guidance on metrics best-practices. ↩
Tick whichever testing boxes are applicable. If you are adding Manual Tests, please document the manual testing (extensively) in the Exceptions. ↩

apollo-librarian · 2025-10-06T21:48:54Z

✅ Docs preview ready

The preview is ready to be viewed. View the preview

File Changes

2 new, 4 changed, 0 removed

+ graphos/routing/(latest)/self-hosted/managed-hosting/railway.mdx
+ graphos/routing/(latest)/self-hosted/managed-hosting/render.mdx
* graphos/routing/(latest)/customization/rhai/index.mdx
* graphos/routing/(latest)/observability/router-telemetry-otel/enabling-telemetry/selectors.mdx
* graphos/routing/(latest)/observability/router-telemetry-otel/enabling-telemetry/spans.mdx
* graphos/routing/(latest)/_sidebar.yaml

Build ID: b00c2b4a87dac6ca3f7372a3
Build Logs: View logs

URL: https://www.apollographql.com/docs/deploy-preview/b00c2b4a87dac6ca3f7372a3

BrynCooke · 2025-10-07T10:02:30Z

apollo-router/src/notification.rs

@@ -832,26 +840,15 @@ where
        operation_name: Option<String>,
    ) -> broadcast::Receiver<()> {
        let (closing_signal_tx, closing_signal_rx) = broadcast::channel(1);


@goto-bus-stop I've not changed the logic here but I am uncertain as to why creating a topic should overwrite the subscription. Should it not keep the subscription if it already exists?

BrynCooke · 2025-10-07T11:34:36Z

Moving back to draft as there is some sort of issue with the pipelines metric.

… original instrument This refactor makes updown counters finally useful as in combination with the temporality fix to make them always aggregate and the fix to only reload metrics if needed they will not accurately increment and decrement reliably.

…pdown counter

…nters.

Moved tests for otlp into directory

BrynCooke · 2025-10-07T16:52:48Z

OK, tests passing, I think we're good to review.

apollo-router/src/axum_factory/tests.rs

…d by updown counter.

goto-bus-stop

Looks great overall, some questions/nits

apollo-router/src/axum_factory/tests.rs

apollo-router/src/compute_job/metrics.rs

apollo-router/src/axum_factory/axum_http_server_factory.rs

apollo-router/src/metrics/mod.rs

goto-bus-stop · 2025-10-10T13:27:13Z

apollo-router/src/metrics/mod.rs

+                    let instrument = create_instrument_fn(meter);
+                    let attrs : &[opentelemetry::KeyValue] = &$attrs;
+                    instrument.$mutation($value, attrs);
+                    $guard::new(std::sync::Arc::new(instrument.clone()), $value, attrs)


Can this Arc::new() ever happen with the NoopGuard? That would suck a bit (it wouldnt make or break performance I imagine but still an unnecessary heap allocation).

c1b06cb I've added an explanation for this. The code is actually not generally run for #cfg[not(test)], it's there to prevent our unit tests from interfering with each other. We do need a test for callsite caching though, so the other arm is run for a specific test hence why the entire block is not under cfg test.

It no longer adds value over just having a guard during the job

…s reload.

goto-bus-stop · 2025-10-13T12:16:56Z

apollo-router/src/plugins/telemetry/reload/builder.rs

+    /// with labels that are unique (like launch_id) can accumulate as zeroed
+    /// out series that will never be incremented again.
+    ///
+    /// The true solution for this is to drop Prometheus support as this has been dropped in upstream OTEL.


Users would probably not be happy about that, maybe we could use a third party prometheus exporter (eg https://lib.rs/crates/opentelemetry-prometheus-text-exporter) when the time comes, but 🤷🏻‍♀️ just because it isn't a first party piece of the SDK doesn't mean it can't be done ;)

Life finds a way....

goto-bus-stop · 2025-10-13T12:17:46Z

apollo-router/src/plugins/telemetry/reload/builder.rs

    fn setup_public_metrics(&mut self) -> Result<(), BoxError> {
        if self.is_metrics_config_changed::<metrics::prometheus::Config>()
            || self.is_metrics_config_changed::<otlp::Config>()
+            || self.prometheus_random_change()


I guess I'd like it on the record that I don't love this solution but I don't have a better idea, so let's just do ittt

I also do not like this. Theres no other way though.....

I changed this to at least be deterministic. It reloads every 10 times rather than randomly.

…s reload.

Previously this was random, but this is bad for deterministic testing.

This comment has been minimized.

Sign in to view

BrynCooke changed the title ~~Bryn/up down counters~~ (refactor) UpDownCounter RAII guards Oct 6, 2025

BrynCooke force-pushed the bryn/up-down-counters branch from e2d282d to 606b2e2 Compare October 7, 2025 09:36

BrynCooke marked this pull request as ready for review October 7, 2025 09:36

BrynCooke requested a review from a team October 7, 2025 09:36

BrynCooke requested a review from a team as a code owner October 7, 2025 09:36

BrynCooke requested a review from goto-bus-stop October 7, 2025 09:37

BrynCooke force-pushed the bryn/up-down-counters branch from 606b2e2 to 0f3090f Compare October 7, 2025 09:48

BrynCooke commented Oct 7, 2025

View reviewed changes

BrynCooke marked this pull request as draft October 7, 2025 11:34

BrynCooke removed the request for review from goto-bus-stop October 7, 2025 11:34

BrynCooke force-pushed the bryn/up-down-counters branch from e9fd533 to c528de6 Compare October 7, 2025 12:40

bryn added 4 commits October 7, 2025 14:12

(refactor) Convert apollo.router.session.count.active from gauge to u…

872c2d6

…pdown counter

(refactor) Convert pipeline and connection handles to use up down cou…

9c0b16e

…nters.

(refactor) Create integration tests for metrics

fcdd0e8

Moved tests for otlp into directory

BrynCooke force-pushed the bryn/up-down-counters branch 2 times, most recently from f1ddee4 to 168c1c7 Compare October 7, 2025 15:48

bryn added 2 commits October 7, 2025 16:50

(docs) Changeset

a01f83b

(fix) Drive by, fix prom initialization logging

697859e

BrynCooke force-pushed the bryn/up-down-counters branch from 168c1c7 to 11a3cd9 Compare October 7, 2025 15:50

(fix) Lifecycle tests need to be more lenient

d61025d

BrynCooke force-pushed the bryn/up-down-counters branch from 11a3cd9 to d61025d Compare October 7, 2025 16:32

BrynCooke marked this pull request as ready for review October 7, 2025 16:52

BrynCooke requested a review from goto-bus-stop October 7, 2025 16:52

(fix) Prometheus metrics have a 1 in 10 chance for reload

e1430e1

BrynCooke force-pushed the bryn/up-down-counters branch from 3132d40 to e1430e1 Compare October 8, 2025 08:53

BrynCooke commented Oct 8, 2025

View reviewed changes

apollo-router/src/axum_factory/tests.rs Outdated Show resolved Hide resolved

BrynCooke force-pushed the bryn/up-down-counters branch from 913afed to e1430e1 Compare October 8, 2025 13:49

bryn added 2 commits October 10, 2025 11:27

(refactor) Remove unused pipeline instruments as these are now handle…

8ee2a1b

…d by updown counter.

(lint) Remove left over comment

701e24b

goto-bus-stop reviewed Oct 10, 2025

View reviewed changes

bryn added 4 commits October 13, 2025 10:36

Add mut_use to updown counter guard

902b633

Add explanation for non-caching arm if logic

c1b06cb

(refactory) Remove ActiveComputeMetric

33b3b71

It no longer adds value over just having a guard during the job

(refactor) Clean up guarded imports

b178c50

BrynCooke requested a review from goto-bus-stop October 13, 2025 10:25

(refactor) make multi pipelines more lenient as there may be a metric…

7cb526e

…s reload.

goto-bus-stop reviewed Oct 13, 2025

View reviewed changes

goto-bus-stop approved these changes Oct 13, 2025

View reviewed changes

(refactor) make multi pipelines more lenient as there may be a metric…

9538a75

…s reload.

BrynCooke enabled auto-merge (squash) October 13, 2025 12:43

BrynCooke disabled auto-merge October 13, 2025 13:25

bryn added 4 commits October 13, 2025 15:07

(refactor) force prometheus metric reload every 20 reloads

e2285e4

Previously this was random, but this is bad for deterministic testing.

(refactor) Make prom not reload on first reload

c934c2e

(test) Make tests less lenient again

dc36be3

(test) Make tests less lenient again

c645147

BrynCooke merged commit 4c04c55 into dev Oct 14, 2025
15 checks passed

BrynCooke deleted the bryn/up-down-counters branch October 14, 2025 06:43

abernix mentioned this pull request Oct 27, 2025

prep release: v2.8.0 #8495

Merged

Conversation

BrynCooke commented Oct 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Notes

Issues

Footnotes

Uh oh!

apollo-librarian bot commented Oct 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Docs preview ready

Uh oh!

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

BrynCooke commented Oct 7, 2025

Uh oh!

BrynCooke commented Oct 7, 2025

Uh oh!

Uh oh!

goto-bus-stop left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

BrynCooke commented Oct 6, 2025 •

edited

Loading

apollo-librarian bot commented Oct 6, 2025 •

edited

Loading