diff --git a/datasette/telemetry_registry.py b/datasette/telemetry_registry.py index cad5acb2..c437f9a9 100644 --- a/datasette/telemetry_registry.py +++ b/datasette/telemetry_registry.py @@ -50,6 +50,20 @@ class Attribute(str): self.values = frozenset(values) if values is not None else None return self + def __reduce__(self): + # Copies and pickles collapse to a plain str. Without this, `copy` has + # to reconstruct a str subclass through `cls.__new__(cls)`, which these + # classes reject - their `__new__` requires the metadata arguments. It + # is not a theoretical problem: the SDK's ConsoleMetricExporter renders + # data points with `dataclasses.asdict()`, which deepcopies mappings, + # and registry entries are used as metric attribute keys - so every + # console metrics dump would crash. Collapsing is also the honest + # answer, not a workaround. On the wire and in a copy an entry *is* + # its string; the description, values and buckets describe the single + # registered instance in this module, and nothing reads them off a + # copy. + return (str, (str(self),)) + def __repr__(self): return f"Attribute({str(self)!r})" @@ -95,6 +109,10 @@ class SpanName(str): self.kind = kind return self + def __reduce__(self): + # See Attribute.__reduce__. + return (str, (str(self),)) + def __repr__(self): return f"SpanName({str(self)!r})" @@ -117,6 +135,10 @@ class MetricName(str): self.buckets = tuple(buckets) if buckets is not None else None return self + def __reduce__(self): + # See Attribute.__reduce__. + return (str, (str(self),)) + def __repr__(self): return f"MetricName({str(self)!r})" diff --git a/tests/test_telemetry_registry.py b/tests/test_telemetry_registry.py index 0072b2ce..479e2994 100644 --- a/tests/test_telemetry_registry.py +++ b/tests/test_telemetry_registry.py @@ -23,7 +23,10 @@ registry and the wire. That is the one comparison in this file that is not made against a value derived from the registry itself. """ +import copy +import io import itertools +import pickle import pytest import pytest_asyncio @@ -396,6 +399,50 @@ def test_registry_entries_are_usable_as_plain_strings(): assert f"{reg.DB_QUERY}.execute" == "db.query.execute" +def test_registry_entries_survive_deepcopy_and_pickle(): + """ + A copy of an entry is a plain `str`. + + These are `str` subclasses whose `__new__` requires the metadata + arguments, so without `__reduce__` `copy` cannot reconstruct one and + raises. That is not academic: the SDK's `ConsoleMetricExporter` renders + data points with `dataclasses.asdict()`, which deepcopies mappings, and + core passes registry entries as metric attribute keys - see + `test_console_metric_exporter_renders_core_metric_points`. + """ + for entry in (reg.DB_NAMESPACE, reg.DB_QUERY, reg.M_OPERATION_DURATION): + assert copy.deepcopy({entry: 1}) == {str(entry): 1} + assert type(copy.deepcopy(entry)) is str + assert pickle.loads(pickle.dumps(entry)) == str(entry) + # The metadata still lives on the registered instance itself, which + # is the only place anything reads it. + assert entry.description.strip() + + +@pytest.mark.asyncio +async def test_console_metric_exporter_renders_core_metric_points(otel_metrics): + """ + The end-to-end shape of the bug above: a console metrics dump of + Datasette's own points has to survive `dataclasses.asdict()`. + """ + from opentelemetry.sdk.metrics.export import ConsoleMetricExporter + from opentelemetry.sdk.metrics.export import MetricExportResult + + name = _unique("registry_console_export") + ds = Datasette(memory=True) + ds.add_memory_database(name) + await ds.invoke_startup() + # One real query, so the dump contains a db.client.operation.duration + # point keyed by the DB_NAMESPACE registry entry. + await ds.get_database(name).execute("select 1") + + data = otel_metrics.reader.get_metrics_data() + assert data is not None, "no metrics captured - nothing to export" + exporter = ConsoleMetricExporter(out=io.StringIO()) + assert exporter.export(data) is MetricExportResult.SUCCESS + ds.close() + + def test_every_histogram_declares_bucket_boundaries(): """ Every histogram must carry explicit boundaries, and only histograms may.