From fd6bf7c4b1ca1a9c6effe64119d46f6bd40fc78b Mon Sep 17 00:00:00 2001 From: Alex Garcia Date: Tue, 22 Sep 2026 09:47:11 -0700 Subject: [PATCH] Drop the db.execute(table=) telemetry label Simon's review: `table=` was a new parameter on the public `execute()` signature that had no effect on execution - it existed only to set the `db.collection.name` span attribute. Removing it takes the whole attribute out of phase 1. `db.query` spans keep `db.namespace`, `db.query.text`, `db.operation.name` and the rest; `db.collection.name` returns in a later PR once there is a mechanism worth committing to in the public API. Side effect worth having: this PR no longer touches `views/table.py` or `views/row.py` at all - both files are now byte-identical to main - so it is purely the database layer it claims to be. The registry conformance tests already enforce the rest: an attribute left in `telemetry_registry` but never emitted fails `test_every_registered_attribute_is_emitted`, so the registry entry, the generated docs block and the workload that reached it all come out together. Co-Authored-By: Claude Opus 5 --- datasette/database.py | 13 +------- datasette/telemetry_registry.py | 9 ------ datasette/views/row.py | 7 ++--- datasette/views/table.py | 15 ++------- docs/changelog.rst | 1 - docs/internals.rst | 6 +--- tests/test_telemetry.py | 54 +------------------------------- tests/test_telemetry_registry.py | 4 --- 8 files changed, 8 insertions(+), 101 deletions(-) diff --git a/datasette/database.py b/datasette/database.py index e125c651..0ed7ef06 100644 --- a/datasette/database.py +++ b/datasette/database.py @@ -20,7 +20,6 @@ from .inspect import inspect_hash from .telemetry import callback_name, sql_attribute, sql_operation_name, tracer from .telemetry_registry import ( CALLBACK, - DB_COLLECTION_NAME, DB_NAMESPACE, DB_OPERATION_NAME, DB_QUERY, @@ -809,16 +808,8 @@ class Database: custom_time_limit=None, page_size=None, log_sql_errors=True, - table=None, ): - """Executes sql against db_name in a thread - - `table`, if passed, is recorded as the `db.collection.name` span - attribute. It exists for callers that already know which table the - query targets - the table and row views - and is never derived from - `sql` itself: deriving it would be a parse, and on an instance where - anyone can create a table the resulting value set has no ceiling. - """ + """Executes sql against db_name in a thread""" self._check_not_closed() page_size = page_size or self.ds.page_size time_limit_ms = self.ds.sql_time_limit_ms @@ -921,8 +912,6 @@ class Database: operation_name = sql_operation_name(sql) if operation_name: span.set_attribute(DB_OPERATION_NAME, operation_name) - if table: - span.set_attribute(DB_COLLECTION_NAME, table) if params: span.set_attribute(PARAM_COUNT, len(params)) try: diff --git a/datasette/telemetry_registry.py b/datasette/telemetry_registry.py index 183d514a..db3fcdbf 100644 --- a/datasette/telemetry_registry.py +++ b/datasette/telemetry_registry.py @@ -106,14 +106,6 @@ DB_OPERATION_NAME = Attribute( "form. Resolving it further would mean parsing.", optional=True, ) -DB_COLLECTION_NAME = Attribute( - "db.collection.name", - "The primary table, set only where the view already knows it - the table " - "and row pages. Omitted for arbitrary ``?sql=`` queries, where determining " - "the table would mean parsing the query.", - optional=True, -) - PARAM_COUNT = Attribute( "datasette.param_count", "Number of bound parameters. Recorded instead of the values themselves.", @@ -197,7 +189,6 @@ DB_QUERY = SpanName( DB_QUERY_TEXT, CALLBACK, DB_OPERATION_NAME, - DB_COLLECTION_NAME, PARAM_COUNT, PARAM_SETS, TIME_LIMIT_MS, diff --git a/datasette/views/row.py b/datasette/views/row.py index 69ab3997..92e75199 100644 --- a/datasette/views/row.py +++ b/datasette/views/row.py @@ -429,7 +429,7 @@ class RowView(BaseView): resolved = await self.ds.resolve_row(request) pk_values = resolved.pk_values results = await resolved.db.execute( - resolved.sql, resolved.params, truncate=True, table=table + resolved.sql, resolved.params, truncate=True ) columns = [r[0] for r in results.description] rows = list(results.rows) @@ -685,9 +685,6 @@ class RowView(BaseView): ] ) try: - # No table= here: this counts incoming references across every - # foreign key pointing at this row, so it spans many tables and - # there is no single value db.collection.name could take. rows = list(await db.execute(sql, {"id": pk_values[0]})) except QueryInterrupted: # Almost certainly hit the timeout @@ -906,7 +903,7 @@ class RowUpdateView(BaseView): actor=request.actor, ): results = await resolved.db.execute( - resolved.sql, resolved.params, truncate=True, table=resolved.table + resolved.sql, resolved.params, truncate=True ) returned_row = results.dicts()[0] result["rows"] = [returned_row] diff --git a/datasette/views/table.py b/datasette/views/table.py index 3d1f146d..26ce2acd 100644 --- a/datasette/views/table.py +++ b/datasette/views/table.py @@ -1188,7 +1188,6 @@ class TableInsertView(BaseView): where_clause, ), args, - table=table_name, ) result["rows"] = fetched_rows.dicts() else: @@ -1402,8 +1401,7 @@ class TableDropView(BaseView): "table": table_name, "row_count": ( await db.execute( - f"select count(*) from {escape_sqlite(table_name)}", - table=table_name, + f"select count(*) from {escape_sqlite(table_name)}" ) ).single_value(), "message": 'Pass "confirm": true to confirm', @@ -1634,10 +1632,7 @@ class TableAutocompleteView(BaseView): try: results = await db.execute( - sql, - params, - custom_time_limit=AUTOCOMPLETE_TIME_LIMIT_MS, - table=table_name, + sql, params, custom_time_limit=AUTOCOMPLETE_TIME_LIMIT_MS ) except QueryInterrupted: fallback_where = _autocomplete_prefix_like(pks[0]) @@ -1658,7 +1653,6 @@ class TableAutocompleteView(BaseView): fallback_sql, params, custom_time_limit=AUTOCOMPLETE_TIME_LIMIT_MS, - table=table_name, ) except QueryInterrupted: return Response.json({"ok": True, "rows": []}) @@ -2258,9 +2252,7 @@ async def table_view_data( # Execute the main query! try: - results = await db.execute( - sql, params, truncate=True, table=table_name, **extra_args - ) + results = await db.execute(sql, params, truncate=True, **extra_args) except (sqlite3.OperationalError, InvalidSql) as e: raise DatasetteError(str(e), title="Invalid SQL", status=400) @@ -2537,7 +2529,6 @@ async def _next_value_and_url( await db.execute( prefix_lookup_sql, {**{f"pk{i}": rows[-2][pk] for i, pk in enumerate(pks)}}, - table=table_name, ) ).single_value() if isinstance(prefix, dict) and "value" in prefix: diff --git a/docs/changelog.rst b/docs/changelog.rst index 8603619c..f2a904ba 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -10,7 +10,6 @@ Unreleased ---------- - Datasette's database layer now emits `OpenTelemetry `__ spans: one per query, covering the full round trip including time spent waiting for a SQL worker thread, plus separate child spans for the execution itself and for time spent in the write queue. Callback-style calls - :ref:`db.execute_fn() `, :ref:`db.execute_write_fn() ` and ``db.execute_isolated_fn()``, the documented way for plugins to run arbitrary SQL - are covered too, carrying ``datasette.callback`` in place of the SQL text. Datasette core depends on ``opentelemetry-api`` only and never installs an SDK provider, an exporter or a sampler, so there is no effect and no measurable overhead unless tracing is switched on externally - normally with the standard ``opentelemetry-instrument`` agent. See :ref:`internals_telemetry`. (:issue:`1730`) -- :ref:`db.execute(sql, ..., table=None) ` has a new optional ``table=`` parameter, naming the table a query is about so it can be recorded on that query's OpenTelemetry span. It has no effect on query execution, and Datasette never derives it from the SQL. (:issue:`1730`) Nothing is removed by the OpenTelemetry work: the ``?_trace=1`` query string parameter, the ``trace_debug`` setting and the :ref:`internals_tracer` module all continue to work as before. diff --git a/docs/internals.rst b/docs/internals.rst index 7123a313..42072fcd 100644 --- a/docs/internals.rst +++ b/docs/internals.rst @@ -2116,9 +2116,6 @@ Executes a SQL query against the database and returns the resulting rows (see :r ``log_sql_errors`` - boolean Should any SQL errors be logged to the console in addition to being raised as an error? Defaults to ``True``. -``table`` - string - The name of the table this query is about, if the caller already knows it. This has no effect on how the query executes - it is recorded as the ``db.collection.name`` attribute on the :ref:`OpenTelemetry span ` for the query. Datasette never derives this from the SQL, so leave it unset for queries that do not have one obvious table. - .. _database_results: Results @@ -2566,7 +2563,6 @@ Spans are ``SpanKind.INTERNAL`` unless a kind is listed below. Only ``db.query`` - ``db.query.text`` *(optional)* - The SQL, truncated to 2048 characters. Never the parameter values. Absent for a callback-style call (``execute_fn()`` and friends), where there is no SQL string to record - ``datasette.callback`` is set instead. - ``datasette.callback`` *(optional)* - The qualified name of the Python callable passed to ``execute_fn()``, ``execute_write_fn()`` or ``execute_isolated_fn()`` - for example ``TableInsertView.post..insert_or_upsert_rows``. Set instead of ``db.query.text``, which does not exist for a callback: the SQL is whatever the function chooses to run. A lambda reports ````, which is why callers wanting a recognisable span should pass a named function. Bounded cardinality: the set of callables is fixed by the installed code, not by request input. - ``db.operation.name`` *(optional)* - The statement's leading keyword - ``SELECT``, ``INSERT``, ``CREATE``, and so on - matched against a small fixed allowlist. Omitted rather than set to an arbitrary value: the attribute must stay safe to use as a metric dimension, and echoing an unrecognised first token from user-supplied SQL would be an unbounded-cardinality hazard. Also omitted for ``execute_write_script()``, which runs multiple statements - per semantic conventions, the operation name should not be extracted from query text that can contain more than one operation. Note that a statement beginning with a CTE reports ``WITH``, not the operation inside it - a substantial share of Datasette's own reads take that form. Resolving it further would mean parsing. - - ``db.collection.name`` *(optional)* - The primary table, set only where the view already knows it - the table and row pages. Omitted for arbitrary ``?sql=`` queries, where determining the table would mean parsing the query. - ``datasette.param_count`` *(optional)* - Number of bound parameters. Recorded instead of the values themselves. - ``datasette.param_sets`` *(optional)* - Number of parameter sets consumed by ``execute_write_many()``. Not a row count - ``executemany()`` returns no rows. The parameter values themselves are never recorded: that sequence can hold thousands of rows. - ``datasette.time_limit_ms`` *(optional)* - The :ref:`setting_sql_time_limit_ms` value this query ran under. Set on reads, which are the queries that time limit applies to. @@ -2612,7 +2608,6 @@ Spans leave your infrastructure whenever you configure an exporter, so what goes - **SQL text is truncated to 2048 characters.** On a public instance the SQL is supplied by visitors and is unbounded in length, so ``db.query.text`` is cut off - with a ``…[truncated]`` marker - rather than allowed to set the size of a span. - **SQL parameter values are never recorded.** Only ``datasette.param_count``, a count. Parameter values are the part of a query most likely to hold something sensitive, and separating them from the SQL is the reason bound parameters exist. - **No actor identifiers are recorded.** No actor ID, no actor JSON, no client IP address. Nothing on a span identifies who made the request. -- **Table names come only from an explicit** ``table=`` **argument.** ``db.collection.name`` is set by callers that already know which table they are working with, and is never derived from the SQL. Deriving it would mean parsing, and on an instance where visitors can create tables the set of possible values has no ceiling. The SQL itself, though, *is* recorded, and on a public instance that means anything a visitor types into the query editor or passes as ``?sql=`` will be exported along with the span. That is the trade-off tracing a query engine makes. @@ -2624,6 +2619,7 @@ Known limitations - **Datasette does not create a span for the HTTP request itself.** Every span listed above is therefore a root span unless something above Datasette - an ASGI instrumentation layer, or the web framework embedding it - has already started one for the request, in which case Datasette's spans nest underneath it correctly. - **Two plugin hooks run outside the** ``datasette.startup`` **span.** ``register_output_renderer`` is dispatched from ``Datasette.__init__()`` and ``asgi_wrapper`` from ``Datasette.app()``, both of which happen before ``invoke_startup()``. Datasette itself queries no database in either, so a default install emits nothing there - but a plugin that does will produce a root trace. Covering these would mean holding a span open across object construction, which is worse than the orphan. - ``db.operation.name`` **reports** ``WITH`` **for a statement that opens with a common table expression**, rather than the operation inside it, and a substantial share of Datasette's own reads take that form. The attribute is a leading-keyword match against a fixed allowlist, deliberately not a parse. +- **Query spans do not carry** ``db.collection.name``. Nothing records which table a query is about. Datasette will not derive it from the SQL - that would mean parsing, and on an instance where visitors can create tables the set of possible values has no ceiling - so it can only come from callers that already know, which needs an API that does not exist yet. - **Spans emitted before a provider is installed are not recorded.** If you are embedding Datasette in a host application, install your ``TracerProvider`` before serving traffic. This is ordinary OpenTelemetry behaviour rather than anything Datasette controls; nothing is permanently affected, those particular spans are simply dropped. .. _internals_csrf: diff --git a/tests/test_telemetry.py b/tests/test_telemetry.py index 48c3a822..f1b2b06b 100644 --- a/tests/test_telemetry.py +++ b/tests/test_telemetry.py @@ -853,7 +853,7 @@ async def test_invoke_startup_produces_one_trace_not_dozens_of_orphans(otel_span ) -# --- Semantic conventions: span kind, scope, db.operation/collection ------- +# --- Semantic conventions: span kind, scope, db.operation.name ------------- @pytest.mark.asyncio @@ -999,58 +999,6 @@ async def test_execute_write_script_has_no_operation_name(otel_spans): assert "db.operation.name" not in script_spans[0].attributes -@pytest.mark.asyncio -async def test_db_collection_name_set_from_table_argument(ds_client, otel_spans): - db = ds_client.ds.get_database("fixtures") - await db.execute("select pk from facetable limit 1", table="facetable") - - spans = _spans_for_namespace(otel_spans, "fixtures") - assert spans - assert spans[-1].attributes["db.collection.name"] == "facetable" - - -@pytest.mark.asyncio -async def test_db_collection_name_absent_without_table_argument(ds_client, otel_spans): - """ - db.collection.name comes only from an explicit table= argument and is - never derived from the SQL. - - Deriving it would be a parse, and on an instance where anybody can create - a table the value set has no ceiling. Without this test the one above - would still pass if the table name were being read out of the query text. - """ - db = ds_client.ds.get_database("fixtures") - await db.execute("select pk from facetable limit 1") - - spans = _spans_for_namespace(otel_spans, "fixtures") - assert spans - span = spans[-1] - assert span.attributes["db.query.text"] == "select pk from facetable limit 1" - assert "db.collection.name" not in span.attributes - - -@pytest.mark.parametrize( - "path,table", - ( - ("/fixtures/facetable.json", "facetable"), - ("/fixtures/simple_primary_key/1.json", "simple_primary_key"), - ), -) -@pytest.mark.asyncio -async def test_table_and_row_pages_set_db_collection_name( - ds_client, otel_spans, path, table -): - "The table and row views know their table, so their queries carry it." - response = await ds_client.get(path) - assert response.status_code == 200 - - spans = _spans_for_namespace(otel_spans, "fixtures") - assert spans - assert any( - span.attributes.get("db.collection.name") == table for span in spans - ), f"expected a db.query span from {path} carrying db.collection.name" - - # --- Callback-style calls: execute_fn / execute_write_fn / execute_isolated_fn diff --git a/tests/test_telemetry_registry.py b/tests/test_telemetry_registry.py index ef7c93a3..0a5b6d44 100644 --- a/tests/test_telemetry_registry.py +++ b/tests/test_telemetry_registry.py @@ -46,7 +46,6 @@ EXPECTED_ATTRIBUTES = { "db.query.text", "datasette.callback", "db.operation.name", - "db.collection.name", "datasette.param_count", "datasette.param_sets", "datasette.time_limit_ms", @@ -140,9 +139,6 @@ async def exercise(): custom_time_limit=1, ) - # db.collection.name - set only by views that already know their table - assert (await ds.client.get(f"/{name}/t?_facet=v")).status_code == 200 - assert (await ds.client.get(f"/{name}/t/1.json")).status_code == 200 return ds