Fix `LaterGauge` metrics to collect from all servers #18751

MadLittleMods · 2025-07-30T17:06:24Z

Fix LaterGauge metrics to collect from all servers

Follow-up to #18714

Previously, our LaterGauge metrics did include the server_name label as expected but we were only seeing the last server being reported in some cases. Any LaterGauge that we were creating multiple times was only reporting the last instance.

This PR updates all LaterGauge to be created once and then we use LaterGauge.register_hook(...) to add in the metric callback as before. This works now because we store a list of callbacks instead of just one.

I noticed this problem thanks to some tests in the Synapse Pro for Small Hosts repo that sanity check all metrics to ensure that we can see each metric includes data from multiple servers.

Testing strategy

This is only noticeable when you run multiple Synapse instances in the same process.
TODO

(see test that was added)

Dev notes

Previous non-global LaterGauge:

synapse_federation_send_queue_xxx
synapse_federation_transaction_queue_pending_destinations
synapse_federation_transaction_queue_pending_pdus
synapse_federation_transaction_queue_pending_edus
synapse_handlers_presence_user_to_current_state_size
synapse_handlers_presence_wheel_timer_size
synapse_notifier_listeners
synapse_notifier_rooms
synapse_notifier_users
synapse_replication_tcp_resource_total_connections
synapse_replication_tcp_command_queue
synapse_background_update_status
synapse_federation_known_servers
synapse_scheduler_running_tasks

Pull Request Checklist

Pull request is based on the develop branch
Pull request includes a changelog file. The entry should:
- Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
- Use markdown where necessary, mostly for code blocks.
- End with either a period (.) or an exclamation mark (!).
- Start with a capital letter.
- Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
Code style is correct (run the linters)

…-tenant

Same changelog as #18714 so they merge

MadLittleMods · 2025-07-30T22:51:04Z

changelog.d/18751.misc

@@ -0,0 +1 @@
+Refactor `LaterGauge` metrics to be homeserver-scoped.


Same changelog as #18714 so they merge

…-tenant

synapse/federation/send_queue.py

See #18751 (comment) ``` ValueError: Invalid metric name: synapse_federation_send_queue_QueueNames.PRESENCE_MAP_size ```

synapse/metrics/__init__.py

…-tenant Conflicts: tests/metrics/test_metrics.py

synapse/metrics/__init__.py

…-tenant

devonh · 2025-08-01T22:19:05Z

synapse/metrics/__init__.py

-    def _register(self) -> None:
-        if self.name in all_gauges.keys():
-            logger.warning("%s already registered, reregistering", self.name)
-            REGISTRY.unregister(all_gauges.pop(self.name))
-
        REGISTRY.register(self)
-        all_gauges[self.name] = self


Huh - that also solves it.

@erikjohnston it looks like you last touched this "reregistering" stuff back 7 years ago.
Do you know if it's safe to blow it away like this? Or is there a reason for it being here that I'm unaware of?

Logically, it makes sense to me that we shouldn't be reregistering the same metrics multiple times within Synapse. Anything that does must surely be a bug.

I think it's fine. I wonder if we should explicitly error if we try and reregister the same metric name then?

REGISTRY.register(...) will complain about duplicate metrics already ⏩

Based on our experience after merging this PR (see #18789 where this PR was reverted), I'm guessing this might have been added because we create many many homeservers in our trial tests (HomeserverTestCase). All of these metrics/hooks can stack up significantly enough to cause havoc in CI.

…-tenant

Changelog can no longer merge with the other one

MadLittleMods · 2025-08-05T15:12:46Z

changelog.d/18751.misc

@@ -0,0 +1 @@
+Fix `LaterGauge` metrics to collect from all servers.


Given the changelog is already underway in https://github.com/element-hq/synapse/blob/release-v1.136/CHANGES.md, I'm going to leave this as-is as it will require manual tinkering anyway to pull into v1.136.0 and we don't need to re-run CI again.

The desire is for this changelog to merge with #18714

Sounds good to me. I'm already manually tinkering away in there

MadLittleMods · 2025-08-05T16:41:12Z

Thank you for the review and release wrangling @devonh 🦟

This reverts commit 076db0a.

@reivilibre

#18789) This PR reverts #18751 ### Why revert? @reivilibre [found](https://matrix.to/#/!vcyiEtMVHIhWXcJAfl:sw1v.org/$u9OEmMxaFYUzWHhCk1A_r50Y0aGrtKEhepF7WxWJkUA?via=matrix.org&via=node.marinchik.ink&via=element.io) that our CI was failing in bizarre ways (thanks for stepping up to dive into this 🙇). Examples: - `twisted.internet.error.ProcessTerminated: A process has ended with a probable error condition: process ended by signal 9.` - `twisted.internet.error.ProcessTerminated: A process has ended with a probable error condition: process ended by signal 15.` <details> <summary>More detailed part of the log</summary> https://github.com/element-hq/synapse/actions/runs/16758038107/job/47500520633#step:9:6809 ``` tests.util.test_wheel_timer.WheelTimerTestCase.test_single_insert_fetch =============================================================================== Error: Traceback (most recent call last): File "/home/runner/.cache/pypoetry/virtualenvs/matrix-synapse-pswDeSvb-py3.9/lib/python3.9/site-packages/twisted/trial/_dist/disttrial.py", line 371, in task await worker.run(case, result) File "/home/runner/.cache/pypoetry/virtualenvs/matrix-synapse-pswDeSvb-py3.9/lib/python3.9/site-packages/twisted/trial/_dist/worker.py", line 305, in run return await self.callRemote(workercommands.Run, testCase=testCaseId) # type: ignore[no-any-return] File "/home/runner/.cache/pypoetry/virtualenvs/matrix-synapse-pswDeSvb-py3.9/lib/python3.9/site-packages/twisted/internet/defer.py", line 1187, in __iter__ yield self File "/home/runner/.cache/pypoetry/virtualenvs/matrix-synapse-pswDeSvb-py3.9/lib/python3.9/site-packages/twisted/internet/defer.py", line 1092, in _runCallbacks current.result = callback( # type: ignore[misc] File "/home/runner/.cache/pypoetry/virtualenvs/matrix-synapse-pswDeSvb-py3.9/lib/python3.9/site-packages/twisted/protocols/amp.py", line 1968, in _massageError error.trap(RemoteAmpError) File "/home/runner/.cache/pypoetry/virtualenvs/matrix-synapse-pswDeSvb-py3.9/lib/python3.9/site-packages/twisted/python/failure.py", line 431, in trap self.raiseException() File "/home/runner/.cache/pypoetry/virtualenvs/matrix-synapse-pswDeSvb-py3.9/lib/python3.9/site-packages/twisted/python/failure.py", line 455, in raiseException raise self.value.with_traceback(self.tb) twisted.internet.error.ProcessTerminated: A process has ended with a probable error condition: process ended by signal 9. tests.util.test_macaroons.MacaroonGeneratorTestCase.test_guest_access_token ------------------------------------------------------------------------------- Ran 4325 tests in 669.321s FAILED (skips=159, errors=62, successes=4108) while calling from thread Traceback (most recent call last): File "/home/runner/.cache/pypoetry/virtualenvs/matrix-synapse-pswDeSvb-py3.9/lib/python3.9/site-packages/twisted/internet/base.py", line 1064, in runUntilCurrent f(*a, **kw) File "/home/runner/.cache/pypoetry/virtualenvs/matrix-synapse-pswDeSvb-py3.9/lib/python3.9/site-packages/twisted/internet/base.py", line 790, in stop raise error.ReactorNotRunning("Can't stop reactor that isn't running.") twisted.internet.error.ReactorNotRunning: Can't stop reactor that isn't running. joining disttrial worker #0 failed Traceback (most recent call last): File "/home/runner/.cache/pypoetry/virtualenvs/matrix-synapse-pswDeSvb-py3.9/lib/python3.9/site-packages/twisted/internet/defer.py", line 1853, in _inlineCallbacks result = context.run( File "/home/runner/.cache/pypoetry/virtualenvs/matrix-synapse-pswDeSvb-py3.9/lib/python3.9/site-packages/twisted/python/failure.py", line 467, in throwExceptionIntoGenerator return g.throw(self.value.with_traceback(self.tb)) File "/home/runner/.cache/pypoetry/virtualenvs/matrix-synapse-pswDeSvb-py3.9/lib/python3.9/site-packages/twisted/trial/_dist/worker.py", line 406, in exit await endDeferred File "/home/runner/.cache/pypoetry/virtualenvs/matrix-synapse-pswDeSvb-py3.9/lib/python3.9/site-packages/twisted/internet/defer.py", line 1187, in __iter__ yield self twisted.internet.error.ProcessTerminated: A process has ended with a probable error condition: process ended by signal 15. ``` </details> With more debugging (thanks @devonh for also stepping in as maintainer), we were finding that the CI was consistently failing at `test_exposed_to_prometheus` which was a bit of smoke because of all of the [metrics changes](#18592) that were merged recently. Locally, although I wasn't able to reproduce the bizarre errors, I could easily see increased memory usage (~20GB vs ~2GB) and the `test_exposed_to_prometheus` test taking a while to complete when running a full test run (`SYNAPSE_TEST_LOG_LEVEL=INFO poetry run trial tests`). <img width="1485" height="78" alt="Lots of memory usage" src="https://github.com/user-attachments/assets/811e2a96-75e5-4a3c-966c-00dc0512cea9" /> After updating `test_exposed_to_prometheus` to dump the `latest_metrics_response = generate_latest(REGISTRY)`, I could see that it's a massive 3.2GB response. Inspecting the contents, we can see 4.1M (4,137,123) entries for just `synapse_background_update_status{server_name="test"} 3.0` which is a `LaterGauge`. I don't think we have 4.1M test cases so it's also unclear why we end up with so many samples but it does make sense that we do see a lot of duplicates because each `HomeserverTestCase` will create a homeserver for each test case that will `LaterGauge.register_hook(...)` (part of the #18751 changes). `tests/storage/databases/main/test_metrics.py` ```python latest_metrics_response = generate_latest(REGISTRY) with open("/tmp/synapse-test-metrics", "wb") as f: f.write(latest_metrics_response) ``` After reverting the #18751 changes, running the full test suite locally doesn't result in memory spikes and seems to run normally. ### Dev notes Discussion in the [`#synapse-dev:matrix.org`](https://matrix.to/#/!vcyiEtMVHIhWXcJAfl:sw1v.org/$vkMATs04yqZggVVd6Noop5nU8M2DVoTkrAWshw7u1-w?via=matrix.org&via=node.marinchik.ink&via=element.io) room. ### Pull Request Checklist  * [x] Pull request is based on the develop branch * [ ] Pull request includes a [changelog file](https://element-hq.github.io/synapse/latest/development/contributing_guide.html#changelog). The entry should: - Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from `EventStore` to `EventWorkerStore`.". - Use markdown where necessary, mostly for `code blocks`. - End with either a period (.) or an exclamation mark (!). - Start with a capital letter. - Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry. * [ ] [Code style](https://element-hq.github.io/synapse/latest/code_style.html) is correct (run the [linters](https://element-hq.github.io/synapse/latest/development/contributing_guide.html#run-the-linters))

Fix `LaterGauge` metrics to collect from all servers Follow-up to #18714 Previously, our `LaterGauge` metrics did include the `server_name` label as expected but we were only seeing the last server being reported in some cases. Any `LaterGauge` that we were creating multiple times was only reporting the last instance. This PR updates all `LaterGauge` to be created once and then we use `LaterGauge.register_hook(...)` to add in the metric callback as before. This works now because we store a list of callbacks instead of just one. I noticed this problem thanks to some [tests in the Synapse Pro for Small Hosts](element-hq/synapse-small-hosts#173) repo that sanity check all metrics to ensure that we can see each metric includes data from multiple servers. ### Testing strategy 1. This is only noticeable when you run multiple Synapse instances in the same process. 1. TODO (see test that was added) ### Dev notes Previous non-global `LaterGauge`: ``` synapse_federation_send_queue_xxx synapse_federation_transaction_queue_pending_destinations synapse_federation_transaction_queue_pending_pdus synapse_federation_transaction_queue_pending_edus synapse_handlers_presence_user_to_current_state_size synapse_handlers_presence_wheel_timer_size synapse_notifier_listeners synapse_notifier_rooms synapse_notifier_users synapse_replication_tcp_resource_total_connections synapse_replication_tcp_command_queue synapse_background_update_status synapse_federation_known_servers synapse_scheduler_running_tasks ``` ### Pull Request Checklist  * [x] Pull request is based on the develop branch * [x] Pull request includes a [changelog file](https://element-hq.github.io/synapse/latest/development/contributing_guide.html#changelog). The entry should: - Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from `EventStore` to `EventWorkerStore`.". - Use markdown where necessary, mostly for `code blocks`. - End with either a period (.) or an exclamation mark (!). - Start with a capital letter. - Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry. * [x] [Code style](https://element-hq.github.io/synapse/latest/code_style.html) is correct (run the [linters](https://element-hq.github.io/synapse/latest/development/contributing_guide.html#run-the-linters))

…18791) Re-introduce: #18751 that was reverted in #18789 (explains why the PR was reverted in the first place). - Adds a `cleanup` pattern that cleans up metrics from each homeserver in the tests. Previously, the list of hooks built up until our CI machines couldn't operate properly, see #18789 - Fix long-standing issue with `synapse_background_update_status` metrics only tracking the last database listed in the config (see #18791 (comment))

…18791) Re-introduce: element-hq/synapse#18751 that was reverted in element-hq/synapse#18789 (explains why the PR was reverted in the first place). - Adds a `cleanup` pattern that cleans up metrics from each homeserver in the tests. Previously, the list of hooks built up until our CI machines couldn't operate properly, see element-hq/synapse#18789 - Fix long-standing issue with `synapse_background_update_status` metrics only tracking the last database listed in the config (see element-hq/synapse#18791 (comment))

MadLittleMods added 3 commits July 29, 2025 18:53

Fix cache metrics to collect from all servers

33d6a3c

Update LaterGauge to work with multiple calls

c90b5f8

Bulk refactor LaterGauge register_hook

35be852

MadLittleMods added the A-Metrics label Jul 30, 2025

MadLittleMods added 4 commits July 30, 2025 17:26

Merge branch 'develop' into madlittlemods/fix-later-gauge-metrics-per…

2b92d11

…-tenant

Add test

acc5ecc

Clean up tests

5e4a0dc

Add changelog

2b9c84b

Same changelog as #18714 so they merge

MadLittleMods commented Jul 30, 2025

View reviewed changes

MadLittleMods added 2 commits July 30, 2025 17:58

Fix lints

193eba2

Merge branch 'develop' into madlittlemods/fix-later-gauge-metrics-per…

770bb81

…-tenant

MadLittleMods commented Jul 31, 2025

View reviewed changes

synapse/federation/send_queue.py Outdated Show resolved Hide resolved

MadLittleMods added 2 commits July 31, 2025 10:42

Fix invalid metric name

f569e29

See #18751 (comment) ``` ValueError: Invalid metric name: synapse_federation_send_queue_QueueNames.PRESENCE_MAP_size ```

Extra protection

0a710cb

devonh reviewed Aug 1, 2025

View reviewed changes

synapse/metrics/__init__.py Outdated Show resolved Hide resolved

Merge branch 'develop' into madlittlemods/fix-later-gauge-metrics-per…

2da500f

…-tenant Conflicts: tests/metrics/test_metrics.py

MadLittleMods marked this pull request as ready for review August 1, 2025 18:02

MadLittleMods requested a review from a team as a code owner August 1, 2025 18:02

MadLittleMods removed the request for review from a team August 1, 2025 18:02

devonh requested changes Aug 1, 2025

View reviewed changes

synapse/metrics/__init__.py Show resolved Hide resolved

MadLittleMods added 2 commits August 1, 2025 15:37

Merge branch 'develop' into madlittlemods/fix-later-gauge-metrics-per…

5efae1e

…-tenant

Remove all_gauges and re-register clobber

11cf4e8

devonh reviewed Aug 1, 2025

View reviewed changes

MadLittleMods requested a review from erikjohnston August 4, 2025 16:06

MadLittleMods added 3 commits August 4, 2025 11:08

Merge branch 'develop' into madlittlemods/fix-later-gauge-metrics-per…

b4479cd

…-tenant

Merge branch 'develop' into madlittlemods/fix-later-gauge-metrics-per…

977fe9e

…-tenant

Update changelog since the Synapse release was cut

d967c9b

Changelog can no longer merge with the other one

MadLittleMods requested review from devonh and removed request for erikjohnston August 5, 2025 14:29

devonh approved these changes Aug 5, 2025

View reviewed changes

MadLittleMods commented Aug 5, 2025

View reviewed changes

devonh merged commit 076db0a into develop Aug 5, 2025
44 checks passed

devonh deleted the madlittlemods/fix-later-gauge-metrics-per-tenant branch August 5, 2025 15:28

MadLittleMods added a commit that referenced this pull request Aug 6, 2025

Revert "Fix LaterGauge metrics to collect from all servers (#18751)"

c012907

This reverts commit 076db0a.

MadLittleMods mentioned this pull request Aug 6, 2025

Revert "Fix LaterGauge metrics to collect from all servers (#18751)" #18789

Merged

3 tasks

devonh added a commit that referenced this pull request Aug 6, 2025

Revert "Fix LaterGauge metrics to collect from all servers (#18751)"

c9f212a

This reverts commit 076db0a.

MadLittleMods mentioned this pull request Aug 7, 2025

Re-introduce: Fix LaterGauge metrics to collect from all servers #18791

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Fix `LaterGauge` metrics to collect from all servers #18751

Fix `LaterGauge` metrics to collect from all servers #18751

Uh oh!

MadLittleMods commented Jul 30, 2025 •

edited

Loading

Uh oh!

MadLittleMods Jul 30, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

devonh Aug 1, 2025

Uh oh!

erikjohnston Aug 5, 2025

Uh oh!

MadLittleMods Aug 5, 2025 •

edited

Loading

Uh oh!

MadLittleMods Aug 6, 2025 •

edited

Loading

Uh oh!

MadLittleMods Aug 5, 2025 •

edited

Loading

Uh oh!

devonh Aug 5, 2025

Uh oh!

Uh oh!

MadLittleMods commented Aug 5, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		@@ -0,0 +1 @@
		Refactor `LaterGauge` metrics to be homeserver-scoped.

Uh oh!

Fix LaterGauge metrics to collect from all servers #18751

Fix LaterGauge metrics to collect from all servers #18751

Uh oh!

Conversation

MadLittleMods commented Jul 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Testing strategy

Dev notes

Pull Request Checklist

Uh oh!

MadLittleMods Jul 30, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

devonh Aug 1, 2025

Choose a reason for hiding this comment

Uh oh!

erikjohnston Aug 5, 2025

Choose a reason for hiding this comment

Uh oh!

MadLittleMods Aug 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MadLittleMods Aug 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devonh Aug 5, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MadLittleMods commented Aug 5, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Fix `LaterGauge` metrics to collect from all servers #18751

Fix `LaterGauge` metrics to collect from all servers #18751

MadLittleMods commented Jul 30, 2025 •

edited

Loading

MadLittleMods Aug 5, 2025 •

edited

Loading

MadLittleMods Aug 6, 2025 •

edited

Loading

MadLittleMods Aug 5, 2025 •

edited

Loading