Review polish: changelog entry, comment dedupe, semconv note, 404 route test

- Add the missing changelog bullet for the HTTP request span - the most
  user-visible signal in the stack had no entry.
- State the deliberate deviation on server.address: it is the verbatim
  Host header including any :port, not the semconv address/port split.
- Deduplicate the is_recording()-vs-is_valid rationale: the middleware
  fast path keeps the full telling, request_span() now points at it.
- Trim the http.route regex rationale in the docs intro to a pointer at
  the attribute description, halve the httpx raw_path digression, drop
  a comment that restated the call below it, and leave client-IP policy
  to the privacy section instead of the user_agent description.
- Assert the routed 404 still carries http.route and an enriched span
  name - route enrichment must not be gated on a successful response.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012U7coQfVu8nK2R4q2mCULA

datasette/telemetry.py

@@ -194,13 +194,11 @@ def _url_path(scope):
     contain encoded slashes, which `scope["path"]` has already collapsed.
 
     The split on "?" is not decoration. The ASGI spec's `raw_path` excludes
-    the query string, and uvicorn honours that, but the name is used the
-    other way round elsewhere in this same dependency tree: httpx's
-    `URL.raw_path` is documented as "raw bytes of both the path and query".
-    A server that followed that reading would hand us `?sql=...` here, and
-    Datasette's query strings carry user-supplied SQL, which core never
-    records. A literal "?" cannot appear unencoded in a path, so the split
-    costs nothing when the server is well behaved.
+    the query string, but the name is read both ways in the wild - httpx's
+    own `raw_path` includes the query - and Datasette's query strings carry
+    user-supplied SQL, which core never records. A literal "?" cannot appear
+    unencoded in a path, so the defensive split costs nothing when the server
+    is well behaved.
     """
     raw_path = scope.get("raw_path")
     if raw_path:
@@ -235,10 +233,8 @@ def request_span(scope):
     span = scope.get(REQUEST_SPAN_SCOPE_KEY)
     if span is None:
         span = otel_trace.get_current_span()
-    # is_recording(), not `get_span_context().is_valid`: with no provider but
-    # an inbound `traceparent`, the API's NoOpTracer hands back a
-    # NonRecordingSpan carrying the *remote* context, which is perfectly valid
-    # and still records nothing.
+    # is_recording(), not `get_span_context().is_valid` - see the fast-path
+    # comment in TelemetryMiddleware for why valid is not the same as recording.
     return span if span.is_recording() else None
 
 
@@ -327,9 +323,6 @@ class TelemetryMiddleware:
 
             escaped = False
             try:
-                # Positional (scope, receive, send) throughout this codebase -
-                # `wrapped_send` is the third argument. `receive` is passed
-                # through unwrapped.
                 await self.app(scope, receive, wrapped_send)
             except BaseException as exception:
                 # BaseException, not Exception: `route_path` turns almost

datasette/telemetry_registry.py

@@ -125,15 +125,15 @@ URL_PATH = Attribute(
 URL_SCHEME = Attribute("url.scheme", "``http`` or ``https``.")
 SERVER_ADDRESS = Attribute(
     "server.address",
-    "The ``Host`` header. Client-controlled, so treat it as untrusted input "
+    "The ``Host`` header, verbatim - including any ``:port`` suffix, a "
+    "deliberate deviation from semantic conventions' ``server.address`` / "
+    "``server.port`` split. Client-controlled, so treat it as untrusted input "
     "rather than as the identity of the server.",
     optional=True,
 )
 USER_AGENT_ORIGINAL = Attribute(
     "user_agent.original",
-    "The ``User-Agent`` header, verbatim. Omitted if the client sent none. "
-    "The client's IP address is deliberately not recorded: core records no "
-    "identifier that would tie a span to a person.",
+    "The ``User-Agent`` header, verbatim. Omitted if the client sent none.",
     optional=True,
 )
 ERROR_TYPE = Attribute(

docs/changelog.rst

@@ -11,6 +11,7 @@ Unreleased
 
 - Datasette's database layer now emits `OpenTelemetry <https://opentelemetry.io/>`__ 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() <database_execute_fn>`, :ref:`db.execute_write_fn() <database_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) <database_execute>` 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`)
+- Every HTTP request now gets an OpenTelemetry ``SERVER`` span, named after the request method and matched route, carrying ``http.route``, the response status and W3C trace context extracted from inbound headers - so every database span has a request to belong to, and Datasette joins distributed traces started by a proxy or calling service. The query string is never recorded. See :ref:`internals_telemetry_requests`. (: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.
 

docs/internals.rst

@@ -2375,7 +2375,7 @@ The request span's name is the only one that is not a fixed string - it is compo
 
     GET /(?P<database>[^\/\.]+)/(?P<table>[^\/\.]+)(\.(?P<format>\w+))?$
 
-That is the route's compiled regular expression, not a prettified ``/{database}/{table}`` template. It is deliberate: Datasette routes with compiled patterns and the route table is fixed when the app is built, so the pattern is exact, bounded and needs no parsing, while transforming it into something prettier accretes edge cases. Django's own instrumentation ships regex-flavoured routes for the same reason.
+That is the route's compiled regular expression, not a prettified ``/{database}/{table}`` template - see the ``http.route`` attribute below for why. Django's own instrumentation ships regex-flavoured routes for the same reason.
 
 .. [[[cog
     from telemetry_doc import spans
@@ -2393,8 +2393,8 @@ That is the route's compiled regular expression, not a prettified ``/{database}/
     - ``http.route`` *(optional)* - The route the request matched, as the compiled regular expression pattern Datasette routes with - for example ``/(?P<database>[^\/\.]+)/(?P<table>[^\/\.]+)(\.(?P<format>\w+))?$`` for a table page. It is deliberately the pattern rather than a prettified ``/{database}/{table}`` template: the route table is fixed when the app is built, so the pattern is exact, bounded and needs no parsing, whereas the transform into something prettier accretes edge cases. Unlike ``url.path`` this is low cardinality, so it is the attribute to group by. Omitted when no route matched - a 404 - which is also when the span name falls back to the bare method.
     - ``url.path`` - The path portion of the URL. The query string is deliberately **not** recorded, on this or any other span: Datasette puts user-supplied SQL in ``?sql=`` and canned query parameters in the query string, so exporting it by default would export exactly the data the rest of this instrumentation is careful with.
     - ``url.scheme`` - ``http`` or ``https``.
-    - ``server.address`` *(optional)* - The ``Host`` header. Client-controlled, so treat it as untrusted input rather than as the identity of the server.
-    - ``user_agent.original`` *(optional)* - The ``User-Agent`` header, verbatim. Omitted if the client sent none. The client's IP address is deliberately not recorded: core records no identifier that would tie a span to a person.
+    - ``server.address`` *(optional)* - The ``Host`` header, verbatim - including any ``:port`` suffix, a deliberate deviation from semantic conventions' ``server.address`` / ``server.port`` split. Client-controlled, so treat it as untrusted input rather than as the identity of the server.
+    - ``user_agent.original`` *(optional)* - The ``User-Agent`` header, verbatim. Omitted if the client sent none.
     - ``http.response.status_code`` *(optional)* - The status of the response, read from the ASGI ``http.response.start`` message rather than from a :ref:`internals_response` object - several views, including static files, file downloads and streaming CSV, send that message themselves and never build one. Omitted if the connection closed before anything was sent.
     - ``error.type`` *(optional)* - Set when the request failed: the exception class name if one escaped the application, otherwise the status code as a string for a 5xx response. A 4xx does **not** set this and does not set an error status - per semantic conventions a client error is not a server span's failure.
 

tests/test_http_span.py

@@ -354,6 +354,9 @@ async def test_a_404_is_not_an_error(ds, otel_spans):
     assert server[0].attributes["http.response.status_code"] == 404
     assert "error.type" not in server[0].attributes
     assert server[0].status.status_code is StatusCode.UNSET
+    # Route enrichment must not be gated on a successful response.
+    assert "http.route" in server[0].attributes
+    assert server[0].name != "GET"
 
 
 @pytest.mark.asyncio