invoke_startup() runs before any request exists, so nothing it does has an
ambient span to nest under. Measured on a fresh instance: 19 distinct traces,
19 of them single- or few-span roots - the register_* hook dispatches, the
internal catalog's db.query reads and its db.write.* catalog writes. In a
trace UI that is nineteen pieces of noise sitting next to every real trace,
which for an operator opening Jaeger for the first time is the difference
between "this works" and "this is unusable".
Bracketing the whole method body in one datasette.startup span takes that to
1. This is not a propagation fix - ticket 04's context propagation was already
correct, it simply had nothing to propagate. The bulk of the app.py diff is
re-indentation; `git diff -w` shows the real change (plus one line-length
rewrap black applied to the StartupError raise).
register_output_renderer and asgi_wrapper stay orphans deliberately: both are
dispatched from Datasette.__init__ / .app(), before invoke_startup() exists to
be called, and wrapping them would mean holding a span open across object
construction in library code that may never serve a request.
Suppressing instrumentation during warm-up was rejected as an alternative: a
slow prepare_connection runs on every connection, not just at startup, and is
exactly what tracing should reveal.
Also corrects the stale write-thread warm-up comment in database.py. It is
still a root, but for a reason worth stating precisely: a raw
threading.Thread does not inherit the starting thread's context, so the
datasette.startup span current on the event loop does not reach it. Read
connections do warm up under copy_context() and nest correctly.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Spans created on a worker thread resolve their parent from that thread's
ambient context, so without this every span produced below Database came
back as an unparented root, disconnected from the request that caused it.
Carrying the caller's context across each boundary is also what makes the
thread-pool wait visible: db.query covers the full round trip, the new
db.query.execute covers only the work inside the worker, and the gap
between them is the queueing the old tracer folds invisibly into one
number.
- execute_fn()'s executor.submit() and execute_isolated_fn()'s
run_in_executor() (immutable databases) now run the callable inside a
contextvars.copy_context(). A *fresh* copy per submit is required:
concurrently entering one shared Context raises "RuntimeError: cannot
enter context ... already entered".
- WriteTask carries the otel Context captured on the event loop at enqueue
time plus an enqueued_at_ns timestamp (both need __slots__ entries, or
they fail with AttributeError at runtime). _execute_writes attaches that
context right after the _SHUTDOWN check and detaches it in a finally
spanning all three execution branches - the write thread is persistent
and shared, so a leaked token would grow its context stack for every
write processed afterwards, and a wrong-token detach only logs rather
than raising.
- New spans: db.query.execute (read worker thread), db.write.queue_wait
(explicit start/end timestamps, so its duration is the real enqueue ->
dequeue wait rather than the microseconds spent building the span) and
db.write.execute (skipped in the conn_exception branch, where fn never
runs). db.query.execute honours log_sql_errors for the same reason
db.query does: facet suggestion probes with log_sql_errors=False and
would otherwise paint two red spans per text column on every table page.
- The write-thread warm-up prepare_connection is left as a documented
orphan root - no caller context exists that early.
Tests assert actual parent/child span-id relationships in a shared trace,
not just that spans exist, since an unparented root looks identical to a
correct span if you only check presence.
Note that copy_context() copies every ContextVar, not just OTel's, so
Datasette's own context vars (_skip_permission_checks,
_permission_check_cache, _in_datasette_client) now flow into worker
threads where they previously did not.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
execute_write(), execute_write_script() and execute_write_many() were the
only Database methods that ran SQL without producing an OpenTelemetry
span, so any instance doing writes - which is every instance, since
Datasette builds its internal catalog through these methods at startup -
showed reads in a trace and nothing else. The same db.system,
db.namespace and db.query.text attributes the read path already sets now
appear here, with db.query.text going through sql_attribute() so
attacker-supplied SQL cannot put an unbounded string on a span.
execute_write_many() records the parameter-set count as
datasette.param_sets, not datasette.rows_returned. executemany() consumes
parameter sets and returns no rows at all, so a rows_returned name would
be describing something that does not exist - and a consumer building a
"rows written" dashboard on top of it would be charting the wrong number.
These spans only cover the event-loop side of a write. The time actually
spent waiting on the write queue and executing on the write thread is not
attributed yet; that needs context propagation across the thread
boundary and lands separately. Writes with block=False are worse still -
execute_write_fn returns before the write happens, so the span closes
early. Span links fix that later.
As with the read path, the existing `with trace(...)` wrappers stay put
and the new spans nest inside them, so ?_trace=1 keeps working
unchanged - including execute_write_many's `count`, which the old tracer
stashes through the context manager's return value.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Datasette's existing tracer times a "sql" block that wraps a good deal
more than the query itself - queueing onto the thread pool, the pool
wait, and result marshalling all disappear into one number. That is
simonw/datasette#1730, "SQL tracing should much more closely track the
SQL query execution", open since 2022. A db.query span here is the outer
half of the answer; a later change adds the inner span drawn around the
sqlite3 call itself, and the gap between the two is exactly the thread
pool wait the current tracer folds away.
The span carries OTel semantic-convention attributes (db.system,
db.namespace, db.query.text) plus a few datasette.* ones. db.query.text
goes through sql_attribute(), which caps it at 2048 characters, because
on a public instance the SQL is attacker-supplied and unbounded. Only
len(params) is recorded, never a parameter value.
The existing `with trace(...)` wrapper stays exactly where it is and the
new span nests inside it. This change removes nothing: ?_trace=1 and the
trace_debug setting keep working unchanged. The two systems are
independent code paths.
Exception handling on the span is explicit rather than inherited from
start_as_current_span's defaults, which would record the exception and
set StatusCode.ERROR on anything passing through. That is wrong here
because some SQL failures are the expected answer. ArrayFacet.suggest()
runs json_type(<column>) against every column precisely to discover
which ones raise "malformed JSON", and passes log_sql_errors=False to
say so. Left to the defaults, a table with N text columns marks N
queries per page as failed - burying genuine failures and tripping any
alerting keyed on span status. Measured on a plain table page before
this: 4 error spans out of 225, all expected. Suppressed errors now
leave the status UNSET and set datasette.sql_error_suppressed instead,
so they stay discoverable without reading as failures.
QueryInterrupted still sets ERROR unconditionally. That is not quite
right either - facet suggestion is designed to time out - but the fix
needs its own reasoning and lands separately.
Behaviour change worth calling out: time_limit_ms is hoisted out of
sql_operation_in_thread so the span can record it on the event loop. It
is therefore read at call time rather than at thread-execution time.
Benign in practice, since ds.sql_time_limit_ms is set at startup, but it
is a real change.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Datasette core is gaining OpenTelemetry spans alongside the existing
hand-rolled tracer. This commit only lays the groundwork - no span is
emitted yet.
Core takes a runtime dependency on opentelemetry-api and nothing more.
It deliberately never creates a TracerProvider, configures an exporter,
or touches sampling: that belongs to whoever runs Datasette, normally
via an opentelemetry-instrument agent. Owning a provider in core was
tried in an earlier design and produced a cross-request span leak, a
process-global provider that tests could not tear down, and a sampling
env var that silently blanked output. With no provider installed every
span is a NonRecordingSpan and costs approximately nothing.
datasette/telemetry.py exposes the module-level tracer plus
sql_attribute(), which truncates SQL to 2048 characters. On a public
instance the SQL is attacker-controlled and unbounded - someone can
paste a 10MB query into ?sql= - so it must never reach a telemetry
pipeline verbatim.
opentelemetry-sdk goes in the dev dependency group only, because the
test suite needs it to assert on spans while the package itself must
not import it. tests/test_telemetry.py enforces that by importing
datasette in a fresh interpreter and inspecting sys.modules, which
catches a lazy import inside a function body that a grep would miss.
conftest.py gains a session-scoped autouse fixture installing an SDK
provider with an InMemorySpanExporter. It has to be session-scoped
because set_tracer_provider() is effectively once-per-process - a
second call logs a warning and is ignored. SimpleSpanProcessor rather
than BatchSpanProcessor, so assertions made right after a request never
race a background export thread. The otel_spans fixture that later
tickets assert against is added here too.
test_datasette_package_never_imports_the_sdk is moved to the front of
the run. Late in a serial run the pytest process holds enough threads
that the fork half of subprocess' fork+exec segfaults the interpreter
on macOS/CPython 3.13. That reproduces with any subprocess call in that
position on an unmodified tree, so it is a pre-existing hazard rather
than something this commit introduces; the repo already moves its other
subprocess-spawning tests to the front for related reasons.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
