mirror of
https://github.com/simonw/datasette.git
synced 2026-09-15 21:14:07 +02:00
6 commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Alex Garcia
|
cf44993bef |
Make db.query spans match OpenTelemetry semantic conventions
Three corrections to the emitted data, bundled because changing what is on the wire after operators have built dashboards on it is a breaking change - so they belong in the first release that ships spans at all, not a later one. db.query is now SpanKind.CLIENT. Trace UIs key their database rendering off the span kind rather than off db.system, so the spans rendered as ordinary internal work despite carrying db.system and db.query.text. The three child spans stay INTERNAL on purpose: db.query.execute, db.write.execute and db.write.queue_wait are Datasette's decomposition of one logical query, not three database calls, and queue_wait touches no database at all - marking them CLIENT would make one query look like several to anything counting spans by kind. The instrumentation scope now carries the Datasette version and a schema URL, so a backend can tell which Datasette produced a span. The URL is 1.29.0 rather than the latest semconv release because that is the highest version at which every name emitted here is the current spelling: db.system was renamed to db.system.name in 1.30.0 and this code still emits the older form. Claiming a later schema would be false, and would stop a consumer translating that name forward, since the claim asserts the rename already happened. db.operation.name is the statement's leading keyword matched against a fixed allowlist, not a parse. On a public instance the SQL is attacker-controlled and this attribute is a candidate metric dimension in a later phase, so echoing back an arbitrary first token would let a visitor's typo mint a permanent series. Anything unrecognised gets no attribute rather than a wrong one. execute_write_script() does not set it at all, since semantic conventions say not to extract an operation name from query text that can hold several statements. db.collection.name comes only from a new table= argument on Database.execute(), and is never derived from the SQL: deriving it would be a parse, and on an instance where anyone can create a table the value set has no ceiling. It is passed from every query in the table and row views that targets exactly one user table. Internal-catalog reads and the row view's cross-table foreign key counts are deliberately left without it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
Alex Garcia
|
eac0fe76ea |
Give startup's ~20 orphan spans somewhere to belong
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> |
||
|
Alex Garcia
|
853984d175 |
Propagate otel context across the thread boundaries
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> |
||
|
Alex Garcia
|
be9baf665f |
Emit db.query spans from the three write entry points
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> |
||
|
Alex Garcia
|
2f84176d47 |
Emit a db.query span around Database.execute()
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> |
||
|
Alex Garcia
|
46961efee2 |
Add opentelemetry-api dependency and datasette/telemetry.py scaffolding
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> |