Skip to content

Conversation

@MadLittleMods
Copy link
Contributor

@MadLittleMods MadLittleMods commented Jul 23, 2025

Bulk refactor Histogram metrics to be homeserver-scoped. We also add lints to make sure that new Histogram metrics don't sneak in without using the server_name label (SERVER_NAME_LABEL).

Part of #18592

Testing strategy

  1. Add the metrics listener in your homeserver.yaml
    listeners:
      # This is just showing how to configure metrics either way
      #
      # `http` `metrics` resource
      - port: 9322
        type: http
        bind_addresses: ['127.0.0.1']
        resources:
          - names: [metrics]
            compress: false
      # `metrics` listener
      - port: 9323
        type: metrics
        bind_addresses: ['127.0.0.1']
  2. Start the homeserver: poetry run synapse_homeserver --config-path homeserver.yaml
  3. Fetch http://localhost:9322/_synapse/metrics and/or http://localhost:9323/metrics
  4. Observe response includes the TODO metrics with the server_name label

Todo

Dev notes

LoggingDatabaseConnection
make_conn
make_pool
make_fake_db_pool

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)

Fix `synapse/federation/federation_server.py`
Wait for #18656 to be merged
so we have `self.server_name` available.
```
synapse/metrics/__init__.py:522: error: Expected the `labelnames` argument of Histogram to be a list of label names (including `SERVER_NAME_LABEL`), but got TupleExpr:528(
  StrExpr(type)
  StrExpr(reason)
  NameExpr(SERVER_NAME_LABEL [synapse.metrics.SERVER_NAME_LABEL])). If this is a process-level metric (vs homeserver-level), use a type ignore comment to disable this check.  [missing-server-name-label]
```
Wait for #18656 to merge
so we have access to `self.server_name`
@MadLittleMods MadLittleMods force-pushed the madlittlemods/18592-refactor-histogram branch from 75ead73 to 4b0a4bb Compare July 23, 2025 21:28
@MadLittleMods MadLittleMods marked this pull request as ready for review July 25, 2025 21:59
@MadLittleMods MadLittleMods requested a review from a team as a code owner July 25, 2025 21:59
Conflicts:
	scripts-dev/mypy_synapse_plugin.py
	synapse/metrics/_gc.py
Copy link
Member

@devonh devonh left a comment

Choose a reason for hiding this comment

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

These changes all look pretty straightforward and use the same patterns as the other metrics changes. 👍

...assuming CI passes of course :)

@MadLittleMods MadLittleMods merged commit d4af297 into develop Jul 29, 2025
106 of 112 checks passed
@MadLittleMods MadLittleMods deleted the madlittlemods/18592-refactor-histogram branch July 29, 2025 20:35
@MadLittleMods
Copy link
Contributor Author

Thanks for the review @devonh 🦩

MadLittleMods added a commit that referenced this pull request Jul 30, 2025
Same changelog as #18724 so they merge
MadLittleMods added a commit that referenced this pull request Jul 31, 2025
```
Failed to stop metrics: TypeError("prometheus_client.metrics.MetricWrapperBase.labels() got multiple values for keyword argument 'server_name'")
```

Noticed while running and debugging some tests.

This bug was introduced in
#18724
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants