Synapse creates metrics with name collisions

[devonh]

Problem

There are some metrics which are created in Synapse that use the same metric name. This leads to the metrics being overridden by the latest metric to be registered

synapse/synapse/util/metrics.py

Lines 313 to 337 in 1823287

	def register_hook(
	self, server_name: str, metric_name: str, hook: Callable[[], None]
	) -> None:
	"""
	Registers a hook that is called before metric collection.
	"""
	server_hooks = self._server_name_to_pre_update_hooks.setdefault(server_name, {})
	if server_hooks.get(metric_name) is not None:
	# TODO: This should be an `assert` since registering the same metric name
	# multiple times will clobber the old metric.
	#
	# We currently rely on this behaviour in a few places:
	# - We instantiate multiple `SyncRestServlet`, one per listener, and in the
	# `__init__` we setup a new `LruCache`.
	# - We instantiate multiple `ApplicationService` (one per configured
	# application service) which use the `@cached` decorator on some methods.
	#
	# Once the above behaviour is changed, this should be changed to an `assert`.
	logger.error(
	"Metric named %s already registered for server %s",
	metric_name,
	server_name,
	)
	server_hooks[metric_name] = hook

So far there are 2 known instances of this happening:

• We instantiate multiple SyncRestServlet, one per listener, and in the __init__ we setup a new LruCache.
• We instantiate multiple ApplicationService (one per configured application service) which use the @cached decorator on some methods.

Background

This is a long-standing issue and these logs are harmless. Previously, metrics were just being clobbered silently. Now with #18828, we have some error logs to call this out.