mirror of
https://github.com/simonw/datasette.git
synced 2026-09-11 19:14:07 +02:00
Make registry entries deepcopy-able, so console metric dumps work
Attribute, SpanName and MetricName are str subclasses whose __new__ requires the metadata arguments, so copy.deepcopy could not reconstruct one - it falls back to cls.__new__(cls) and raises TypeError. That broke a real path rather than a theoretical one. The SDK's ConsoleMetricExporter renders data points through dataclasses.asdict(), which deepcopies mappings, and both core and kit-based plugins pass registry entries as metric attribute keys - so every console metrics dump crashed, core's own points included. Found by datasette-paper's dev harness running opentelemetry-instrument with console exporters. __reduce__ collapses copies to a plain str, which is what an entry is everywhere except the registry module itself: the description, values and buckets describe the single registered instance, and nothing reads them off a copy. Pickle is fixed by the same change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JwU7BcTnAxUGSJYhBrQaY7
This commit is contained in:
parent
71d382bbd4
commit
032943b866
2 changed files with 69 additions and 0 deletions
|
|
@ -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})"
|
||||
|
||||
|
|
|
|||
|
|
@ -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.
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue