Skip to content

Widen RequestObservation: it is too narrow for an access log, and consumers bypass it #202

Description

@aaylward

The evidence

RequestObservation carries six fields: method, target, operation, trace_parent, status, duration. That is enough for metrics — which is what it was built for — and not enough for the other thing every server does with a per-request hook, which is emit one line about the request.

MoonBase's aura says so directly. Its AccessLog middleware carries this comment:

One access-log line per request. Kept separate from Observe because the log line needs X-Forwarded-For and the response body size, which RequestObservation doesn't carry; it measures its own duration for the log line only.

Two costs land there. The chain runs two clocks per request measuring the same interval, and the numbers can disagree — the log's duration brackets a different span than Observe's, since the two middleware sit at different depths. And a consumer that wants both metrics and a log line composes two observability middleware where one hook should serve.

There is a sharper gap a few lines further down in the same file:

X-Forwarded-For= is the raw header, which since ADR-0012 is NOT the identity the rate limiter keys on — a 429's actual bucket (the derived client address) is not on this line.

So "which client got rate limited" is unanswerable from the access log today. The service computed the answer — PerClientRateLimit keyed on it — and then dropped it.

What to add

  • response_bytesHttpResponse::body.size(). Trivial; Observe already holds the response when it builds the observation.
  • request_bytesHttpRequest::body.size(). Same.
  • The ADR-0012 derived clientDerivedClient (address + Source provenance) from smithy/http/forwarded.h, not the raw header. This is the one that closes the 429 gap, and the Source is worth carrying too: the distribution of sources is the documented misconfiguration signal in the production guide.
  • Something for the thrown-handler path. Observe already reports status 500 with an empty operation when dispatch throws, but the observation cannot distinguish that from a handler that returned 500 deliberately. A bool or an exception-kind field separates "we crashed" from "we said no", which is the first question asked about a 500 spike.

The design question this needs answered first

Observe cannot derive the client on its own: DeriveClient needs a TrustedProxies, and Observe has no configuration. Three options, and picking one is the actual work:

  1. Observe takes a TrustedProxies. Honest but duplicative — PerClientRateLimit already takes one, and two middleware in a chain would each need the same trust boundary passed in, which is exactly the kind of "configure it twice, get it wrong once" that ADR-0012 exists to prevent.
  2. The transport stamps a resolved client onto the request during ingress, alongside the traceparent it already mints (ADR-0011). One derivation per request, every middleware reads it, the trust boundary is configured once at the transport. This is my preference — it matches how trace_parent already works, and trace_parent is the precedent that made correlation free.
  3. A separate optional enricher that middleware can opt into. Most flexible, most surface.

Option 2 changes HttpRequest, which is a wider blast radius than changing RequestObservation, so it deserves the argument before anyone writes it.

Why it is worth doing regardless of what comes next

This is the prerequisite for #(structured access log, filed separately) and for any future event pipeline, but it pays on its own: aura deletes its duplicate clock and its bypass, and the 429 question becomes answerable. It does not commit anyone to a new backend, a new dependency, or a new format.

Not in scope

A logging facility in the runtime. runtime/include/smithy/core/ has no logger and should not grow one — the observability posture is SDK-free hooks (docs/production-guide.md), and consumers bring their own sink. This issue widens what the hook reports; it does not decide where it goes.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions