From 062dee023730e797f54d40b162145a3ffe3b23b2 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 9 Sep 2026 04:20:18 +0000 Subject: [PATCH 1/2] Make every rendered metric value exact, and abort the misuses that were slipping through MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four defects from a post-merge review of #199, all in the application-metrics path, all producing the failure mode that code's own comments name as the worst case: a scrape Prometheus rejects in full, with no in-process consumer to notice. FormatNumber built a string from snprintf's would-have-written length, an out-of-bounds read for any value whose "%.6f" form exceeds the 64-byte buffer — gauge.Set(1e60), or a bucket bound of 1e60, which the ladder validation happily admits. And "%.6f" is lossy below its granularity: {1e-8, 1e-7} passes the strictly-ascending check while both bounds render "0", two identical le labels, a duplicate series, the whole scrape refused. The rule now: the fixed six-decimal spelling is kept only when it parses back to exactly the value it claims — so every historical series string is byte-stable, which matters because those strings are series identity — and everything else gets a shortest round-trip form. One rule, both bugs. Three fail-fast gaps, closed to match the posture the file already has (ADR-0009): duplicate label names aborted nowhere and rendered {a="1",a="2"}; a user label named le on a histogram doubled the label the bucket lines append themselves (reserved only there — on a counter it is legal Prometheus, and a test pins the scoping); and re-registering a histogram compared kind and help but not buckets, beneath a comment promising "a mismatch aborts rather than picking one". Counter::Increment now aborts on a negative amount. The decrease fails nowhere visible — rate() reads it as a counter reset and extrapolates from zero, inflating exactly the panel someone is staring at. client_golang panics here for the same reason. Checked before the disabled-registry early-return, so enabling metrics in production is never the first time it runs. Also replaces a vacuous transport assertion: the scrape-does-not-count- itself check searched for operation=""/status= spellings that cannot appear in this exposition at all, so it passed under any composition. It now asserts route="/metrics" is absent, which the endpoint's self-label makes falsifiable. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_014C7WdBD99mUFWGxMvGSjSU --- runtime/include/smithy/server/metrics.h | 21 ++- runtime/src/server/metrics.cc | 75 +++++++--- runtime/tests/http/beast_transport_test.cc | 7 +- runtime/tests/server/metrics_test.cc | 153 +++++++++++++++++++++ 4 files changed, 232 insertions(+), 24 deletions(-) diff --git a/runtime/include/smithy/server/metrics.h b/runtime/include/smithy/server/metrics.h index ffb4c1b6..adae722c 100644 --- a/runtime/include/smithy/server/metrics.h +++ b/runtime/include/smithy/server/metrics.h @@ -144,6 +144,17 @@ class Counter { public: void Increment(double amount = 1.0) { Increment(MetricLabels{}, amount); } void Increment(const MetricLabels& labels, double amount = 1.0) { + // A counter only goes up. A negative amount does not fail anywhere + // visible — rate() and increase() read the decrease as a counter reset + // and extrapolate from zero, inflating exactly the panel someone is + // staring at. Fail-fast like every other scrape-corrupting misuse + // (ADR-0009; prometheus/client_golang panics here for the same reason), + // and deliberately BEFORE the disabled-registry check: enabling metrics + // in production must never be the first time this runs. + if (amount < 0) { + smithy::internal::Fatal( + "smithy::server::Counter: a counter may not be incremented by a negative amount"); + } // A handle from a disabled registry holds no family. The branch is what // makes an always-compiled call site free when metrics are off; the // argument is not, so guard a hot call site whose labels are themselves @@ -247,8 +258,9 @@ struct MetricsOptions { // stored, and invisible — the failure mode that looks like success. std::string service_name{}; - // Bounds the distinct {method,route,status} and {method,route} - // combinations retained, and separately the series of each application + // Bounds the distinct {method,route} combinations retained by the built-in + // families (which share one record per route, so one cap admits or refuses + // all of them together), and separately the series of each application // family; see the cardinality note on MetricsRegistry. std::size_t max_series = 4096; }; @@ -405,8 +417,9 @@ class MetricsRegistry { std::uint64_t duration_count = 0; }; - // Renders `{a="1",b="2"}` from the constant labels plus what is passed, - // dropping any pair whose name is empty. Callers hold mutex_. + // Renders the inner label text of a built-in series: service_name first, + // then the passed pairs in the order given (names here are code constants, + // never caller data). Callers hold mutex_. std::string BuiltInLabels(const MetricLabels& labels) const; // Finds or admits the stats for `key`, or returns nullptr when the cap diff --git a/runtime/src/server/metrics.cc b/runtime/src/server/metrics.cc index 1d5ce2c3..c8ada23c 100644 --- a/runtime/src/server/metrics.cc +++ b/runtime/src/server/metrics.cc @@ -4,6 +4,7 @@ #include #include #include +#include #include #include #include @@ -90,21 +91,44 @@ std::string FormatNumber(double value) { if (std::isinf(value)) { return value > 0 ? "+Inf" : "-Inf"; } + if (std::isnan(value)) { + return "NaN"; + } + // "%.6f" with trailing zeros trimmed is the stable spelling: the built-in + // microsecond families and every integral bound render as plain digits, and + // those strings are series identity — a le="2500000" that became le="2.5e+06" + // would be a different series to Prometheus. But "%.6f" is lossy outside its + // range: it cannot say anything smaller than 1e-6 (two distinct tiny bucket + // bounds would both render "0" — duplicate series, whole scrape rejected), + // and for huge values snprintf reports a length longer than any buffer it + // was given. So the fixed form is used only when it parses back to exactly + // the value it claims to be. std::array buffer{}; - const int written = std::snprintf(buffer.data(), buffer.size(), "%.6f", value); - if (written <= 0) { - return "0"; - } - std::string text(buffer.data(), static_cast(written)); - if (text.find('.') != std::string::npos) { - text.erase(text.find_last_not_of('0') + 1); - if (!text.empty() && text.back() == '.') { - text.pop_back(); + const int fixed = std::snprintf(buffer.data(), buffer.size(), "%.6f", value); + if (fixed > 0 && static_cast(fixed) < buffer.size()) { + std::string text(buffer.data(), static_cast(fixed)); + if (text.find('.') != std::string::npos) { + text.erase(text.find_last_not_of('0') + 1); + if (!text.empty() && text.back() == '.') { + text.pop_back(); + } + } + if (!text.empty() && std::strtod(text.c_str(), nullptr) == value) { + return text; + } + } + // Shortest round-trip form, fewest digits first. 17 significant digits are + // sufficient for any double, so the last format cannot fail the parse-back + // check, and its longest rendering (~24 characters) fits the buffer. + for (const char* format : {"%.15g", "%.16g", "%.17g"}) { + const int written = std::snprintf(buffer.data(), buffer.size(), format, value); + if (written > 0 && static_cast(written) < buffer.size() && + std::strtod(buffer.data(), nullptr) == value) { + return std::string(buffer.data(), static_cast(written)); } } - return text.empty() ? "0" : text; + return "0"; // unreachable: kept so the compiler sees every path return } - // Prometheus metric names are [a-zA-Z_:][a-zA-Z0-9_:]*, label names the same // without the colon. Both are code constants here, so an invalid one is a // programming error caught on the first run rather than data to sanitize — @@ -123,14 +147,28 @@ bool ValidName(std::string_view name, bool allow_colon) { // Renders a label set into the inner text of `{...}`, sorted by name so the // same labels in a different order address the same series instead of // silently minting a second one. -std::string RenderLabels(const MetricLabels& labels) { +std::string RenderLabels(const MetricLabels& labels, bool histogram) { std::vector> sorted(labels.begin(), labels.end()); std::ranges::sort(sorted, [](const auto& a, const auto& b) { return a.first < b.first; }); std::string out; - for (const auto& [name, value] : sorted) { + for (std::size_t i = 0; i < sorted.size(); ++i) { + const auto& [name, value] = sorted[i]; if (!ValidName(name, /*allow_colon=*/false)) { smithy::internal::Fatal("smithy::server::MetricsRegistry: invalid label name '" + name + "'"); } + // Both of these render a scrape Prometheus rejects whole, with no + // in-process consumer to notice — the same class the invalid-name abort + // above exists for (ADR-0009). Duplicates are adjacent after the sort; + // `le` is the label the histogram exposition appends itself, so a user + // copy would put it on every bucket line twice. + if (i > 0 && name == sorted[i - 1].first) { + smithy::internal::Fatal("smithy::server::MetricsRegistry: duplicate label name '" + name + + "'"); + } + if (histogram && name == "le") { + smithy::internal::Fatal( + "smithy::server::MetricsRegistry: 'le' is reserved on a histogram's series"); + } if (!out.empty()) out += ','; out += name; out += "=\""; @@ -197,7 +235,7 @@ void AppendFamilyHeader(std::string& out, std::string_view name, std::string_vie namespace internal { void MetricFamily::Add(const MetricLabels& labels, double amount, bool set) { - const std::string key = RenderLabels(labels); + const std::string key = RenderLabels(labels, kind == Kind::kHistogram); const std::lock_guard lock(mutex); if (auto found = samples.find(key); found != samples.end()) { if (set) { @@ -215,7 +253,7 @@ void MetricFamily::Add(const MetricLabels& labels, double amount, bool set) { } void MetricFamily::Observe(const MetricLabels& labels, double value) { - const std::string key = RenderLabels(labels); + const std::string key = RenderLabels(labels, kind == Kind::kHistogram); const std::lock_guard lock(mutex); auto found = samples.find(key); if (found == samples.end()) { @@ -237,7 +275,7 @@ void MetricFamily::Observe(const MetricLabels& labels, double value) { } void MetricFamily::Declare(const MetricLabels& labels) { - const std::string key = RenderLabels(labels); + const std::string key = RenderLabels(labels, kind == Kind::kHistogram); const std::lock_guard lock(mutex); if (samples.contains(key)) { return; // idempotent, and never disturbs a series already carrying events @@ -414,9 +452,10 @@ std::shared_ptr MetricsRegistry::Register(std::string na // Idempotent for an identical re-registration; a mismatch is the case // that would corrupt the scrape, so it aborts rather than picking one. const internal::MetricFamily& existing = *found->second; - if (existing.kind != kind || existing.help != help) { + if (existing.kind != kind || existing.help != help || existing.buckets != buckets) { smithy::internal::Fatal("smithy::server::MetricsRegistry: '" + name + - "' is already registered with a different type or help text"); + "' is already registered with a different type, help text, or " + "bucket ladder"); } return found->second; } diff --git a/runtime/tests/http/beast_transport_test.cc b/runtime/tests/http/beast_transport_test.cc index d72aedf7..b261d2fd 100644 --- a/runtime/tests/http/beast_transport_test.cc +++ b/runtime/tests/http/beast_transport_test.cc @@ -1306,8 +1306,11 @@ TEST(BeastTransportTest, TheMetricsEndpointScrapesOverTheRealTransport) { std::string::npos) << body; // The scrape itself went through MetricsEndpoint, which sits outside - // RecordMetrics — so it answered without counting itself. - EXPECT_EQ(body.find(R"(operation="",status="200")"), std::string::npos) << body; + // RecordMetrics — so it answered without counting itself. The endpoint + // stamps route="/metrics" on its response, so a counted scrape would be + // exactly that series (asserting the old operation=""/status= spellings + // here was vacuous: those labels cannot appear in this exposition at all). + EXPECT_EQ(body.find(R"(route="/metrics")"), std::string::npos) << body; server.Stop(); } diff --git a/runtime/tests/server/metrics_test.cc b/runtime/tests/server/metrics_test.cc index 7a7309b4..78a297e6 100644 --- a/runtime/tests/server/metrics_test.cc +++ b/runtime/tests/server/metrics_test.cc @@ -1337,5 +1337,158 @@ TEST(MetricsRegistryDeathTest, AnUnusableHistogramLadderAborts) { EXPECT_DEATH({ MetricsRegistry(Enabled()).NewHistogram("d_bytes", "D.", infinite); }, ""); } +// --------------------------------------------------------------------------- +// Value formatting: every rendered number is exact, and the historical +// spellings are stable. "%.6f" is the identity of every existing series +// (le="2500000" that became le="2.5e+06" would be a different series), but it +// is lossy outside its range — it cannot say anything smaller than 1e-6, and +// for huge values it used to read past its own buffer. The rule now: the +// fixed spelling only when it parses back to exactly the value it claims. +// --------------------------------------------------------------------------- + +TEST(ValueFormattingTest, HugeValuesRenderExactlyInsteadOfReadingPastTheBuffer) { + // 1e60 needs 67 characters as "%.6f" — more than the formatting buffer. + // The old code built a string from snprintf's would-have-written length, + // an out-of-bounds read on every scrape (the ASan job is what makes this + // test a proof rather than a hope). + MetricsRegistry registry(Enabled()); + auto gauge = registry.NewGauge("big_gauge", "Big."); + gauge.Set(1e60); + EXPECT_TRUE(HasLine(registry.Expose(), "big_gauge 1e+60")) << registry.Expose(); +} + +TEST(ValueFormattingTest, TinyBucketBoundsStayDistinctInsteadOfCollapsingToZero) { + // {1e-8, 1e-7} passes the strictly-ascending check, and under plain "%.6f" + // both rendered "0" — two identical le labels, a duplicate series, and a + // scrape Prometheus rejects whole. + MetricsRegistry registry(Enabled()); + auto tiny = registry.NewHistogram("tiny_seconds", "Tiny.", {1e-8, 1e-7}); + tiny.Observe(5e-8); + + const std::string exposition = registry.Expose(); + EXPECT_TRUE(HasLine(exposition, R"(tiny_seconds_bucket{le="1e-08"} 0)")) << exposition; + EXPECT_TRUE(HasLine(exposition, R"(tiny_seconds_bucket{le="1e-07"} 1)")) << exposition; + EXPECT_TRUE(HasLine(exposition, "tiny_seconds_sum 5e-08")) << exposition; +} + +TEST(ValueFormattingTest, ValuesDifferingPastSixDecimalsStayDistinct) { + // 1.1e-6 and 1.2e-6 both rendered "0.000001" under "%.6f" — same duplicate + // trap, one decimal place further in. The fixed form is kept only when it + // round-trips, so these fall through to the shortest exact spelling. + MetricsRegistry registry(Enabled()); + auto close_buckets = registry.NewHistogram("close_seconds", "Close.", {1.1e-6, 1.2e-6}); + close_buckets.Declare(); + + const std::string exposition = registry.Expose(); + EXPECT_TRUE(HasLine(exposition, R"(close_seconds_bucket{le="1.1e-06"} 0)")) << exposition; + EXPECT_TRUE(HasLine(exposition, R"(close_seconds_bucket{le="1.2e-06"} 0)")) << exposition; +} + +TEST(ValueFormattingTest, TheHistoricalSpellingsAreUntouched) { + // The stability half of the bargain: everything "%.6f" rendered exactly + // keeps its bytes, because those strings are series identity to whatever + // already scraped them. (The built-in microsecond ladder is pinned by the + // contract tests above; this pins the fractional and integral app cases.) + MetricsRegistry registry(Enabled()); + auto gauge = registry.NewGauge("plain_gauge", "Plain."); + gauge.Set(0.005); + EXPECT_TRUE(HasLine(registry.Expose(), "plain_gauge 0.005")) << registry.Expose(); + gauge.Set(2500000); + EXPECT_TRUE(HasLine(registry.Expose(), "plain_gauge 2500000")) << registry.Expose(); + gauge.Set(-0.25); + EXPECT_TRUE(HasLine(registry.Expose(), "plain_gauge -0.25")) << registry.Expose(); +} + +// --------------------------------------------------------------------------- +// The fail-fast gaps: misuses that render a scrape Prometheus rejects whole +// used to pass silently here while aborting everywhere else (ADR-0009). +// --------------------------------------------------------------------------- + +TEST(MetricsRegistryDeathTest, DuplicateLabelNamesAbortInsteadOfRenderingTwice) { + // {a="1",a="2"} is a parse error that fails the entire scrape, hours later, + // on a dashboard nobody is watching — the exact class the invalid-name + // abort next to it exists for. + EXPECT_DEATH( + { + MetricsRegistry registry(Enabled()); + auto counter = registry.NewCounter("dup_total", "Dup."); + counter.Increment({{"a", "1"}, {"a", "2"}}); + }, + "duplicate label name"); + // Equal values are no excuse: the rendered line is just as unparseable. + EXPECT_DEATH( + { + MetricsRegistry registry(Enabled()); + auto counter = registry.NewCounter("dup_total", "Dup."); + counter.Declare({{"a", "1"}, {"a", "1"}}); + }, + "duplicate label name"); +} + +TEST(MetricsRegistryDeathTest, TheHistogramsOwnLeLabelIsReserved) { + // The exposition appends le to every bucket line itself; a user copy would + // put it there twice. + EXPECT_DEATH( + { + MetricsRegistry registry(Enabled()); + auto histogram = registry.NewHistogram("h_seconds", "H.", {1.0}); + histogram.Observe({{"le", "oops"}}, 0.5); + }, + "reserved"); +} + +TEST(MetricsRegistryTest, LeIsOnlyReservedWhereTheExpositionUsesIt) { + // On a counter or gauge, a label named le is legal Prometheus — odd, but + // not ours to forbid. The reservation is scoped to where it collides. + MetricsRegistry registry(Enabled()); + auto counter = registry.NewCounter("odd_total", "Odd."); + counter.Increment({{"le", "fine"}}); + EXPECT_TRUE(HasLine(registry.Expose(), R"(odd_total{le="fine"} 1)")) << registry.Expose(); +} + +TEST(MetricsRegistryDeathTest, ReRegisteringAHistogramWithADifferentLadderAborts) { + // The mismatch abort next to this promised "a mismatch is the case that + // would corrupt the scrape" — but compared only kind and help, so a second + // caller's observations landed silently in the first caller's bins. + EXPECT_DEATH( + { + MetricsRegistry registry(Enabled()); + registry.NewHistogram("ladder_seconds", "L.", {1.0, 2.0}); + registry.NewHistogram("ladder_seconds", "L.", {1000.0, 2000.0}); + }, + "bucket ladder"); +} + +TEST(MetricsRegistryTest, ReRegisteringAHistogramWithTheSameLadderIsIdempotent) { + MetricsRegistry registry(Enabled()); + auto first = registry.NewHistogram("same_seconds", "S.", {1.0, 2.0}); + auto second = registry.NewHistogram("same_seconds", "S.", {1.0, 2.0}); + first.Observe(0.5); + second.Observe(0.5); + EXPECT_TRUE(HasLine(registry.Expose(), "same_seconds_count 2")) << registry.Expose(); +} + +TEST(MetricsRegistryDeathTest, ANegativeCounterIncrementAborts) { + // A decreasing counter reads to rate() as a reset, which extrapolates from + // zero and inflates exactly the panel someone is staring at, with no error + // anywhere. client_golang panics here for the same reason. + EXPECT_DEATH( + { + MetricsRegistry registry(Enabled()); + auto counter = registry.NewCounter("down_total", "Down."); + counter.Increment(-5.0); + }, + "negative"); + // And on a disabled registry too: enabling metrics in production must + // never be the first time the check runs. + EXPECT_DEATH( + { + MetricsRegistry registry; + auto counter = registry.NewCounter("down_total", "Down."); + counter.Increment(-5.0); + }, + "negative"); +} + } // namespace } // namespace smithy::server From eda328f1eba420b8d55979c1595265301fc1e9ba Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 9 Sep 2026 04:20:29 +0000 Subject: [PATCH 2/2] Skip the client derivation when nobody asked for it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Observe derived the ADR-0012 client on every request — a header walk, an address parse, and string building — even in chains whose sinks never read it, which is every metrics-only composition: RecordMetrics deliberately ignores observation.client, and the loopback paths this repo times are the ones that notice. The trust boundary is now optional, defaulting to none-supplied, which skips the derivation entirely and leaves client value-initialized at Source::kUnknown. Any supplied boundary derives, including TrustedProxies::None(), which keeps its meaning as the explicit direct-connect statement rather than doubling as "I didn't think about it". A sink that wants the client states the boundary, the way PerClientRateLimit already must — and both of the access-log formatter's call sites already do. Also from the same review: the guide's application-metrics example called NewHistogram without the buckets argument the API deliberately requires, so the documented snippet did not compile (and named its metric _seconds in a microseconds dialect); the CHANGELOG introduced the endpoint as "three families" and enumerated five in the same sentence; max_series was documented against the {method,route,status} keying an earlier commit removed; and BuiltInLabels promised an empty-name drop its rewrite no longer performs. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_014C7WdBD99mUFWGxMvGSjSU --- CHANGELOG.md | 21 ++++++++++---- docs/production-guide.md | 22 ++++++++++----- runtime/include/smithy/server/middleware.h | 27 +++++++++++------- runtime/src/server/middleware.cc | 14 ++++++---- runtime/tests/server/middleware_test.cc | 32 ++++++++++++++++++++-- 5 files changed, 85 insertions(+), 31 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8e4e07f9..9bc1fb47 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,9 +10,8 @@ policy in [docs/versioning.md](docs/versioning.md). - **A dependency-free Prometheus `/metrics` endpoint** (#91, first work item). `smithy::server::MetricsRegistry` aggregates the existing `Observe` - hooks into three families — - the five `http_server_*` families labeled by `service_name`, `http_method` - and `route` — and `MetricsEndpoint` serves them in the + hooks into the five `http_server_*` families labeled by `service_name`, + `http_method` and `route`, and `MetricsEndpoint` serves them in the text exposition format, which needs no client library and so costs zero new dependencies. `RecordMetrics` is `Observe` wired to a registry, so request timing keeps one implementation and the scraped numbers cannot drift from @@ -23,7 +22,15 @@ policy in [docs/versioning.md](docs/versioning.md). `http_method` outside the nine RFC 9110 verbs collapses to `CUSTOM`, and a series cap backstops anything unforeseen while counting what it refused in - `metrics_observations_dropped_total`. Application metrics join the + `metrics_observations_dropped_total`. Rendered values are exact: the + fixed six-decimal spelling is kept only where it parses back to the value + it claims (so every historical series string is stable), with a shortest + round-trip form for values it cannot represent — two distinct tiny bucket + bounds can no longer collapse into one `le` and fail the scrape. Misuse + that would corrupt a scrape aborts (ADR-0009): duplicate label names, a + user label named `le` on a histogram, re-registering a histogram under a + different bucket ladder, and incrementing a counter by a negative amount + (which `rate()` would read as a reset and inflate). Application metrics join the same scrape through `NewCounter` / `NewGauge` / `NewHistogram`, so one Prometheus target covers the service; the registry keeps owning escaping, label ordering, and the per-family cap. `Declare` exports a known series at @@ -73,8 +80,10 @@ policy in [docs/versioning.md](docs/versioning.md). unanswerable. The observation now adds `request_bytes`, `response_bytes`, `handler_threw`, and `client` — the derived client with its provenance, never the forgeable header. `Observe` takes the trust boundary as a fourth - defaulted parameter (`TrustedProxies::None()`, the direct-connect - statement); pass it the same one given to the limiter. `handler_threw` + optional parameter; pass it the same one given to the limiter. Left unset, + the derivation is skipped entirely and `client` stays at `Source::kUnknown` + — a metrics-only chain pays nothing for a field it never reads — while + `TrustedProxies::None()` remains the explicit direct-connect statement. `handler_threw` separates a contained crash from a deliberate 500, which report identically otherwise. diff --git a/docs/production-guide.md b/docs/production-guide.md index dfedea0f..28ab25df 100644 --- a/docs/production-guide.md +++ b/docs/production-guide.md @@ -364,11 +364,15 @@ and `client` — the ADR-0012 derived client (address plus provenance), **not** the raw `x-forwarded-for`, which a direct client can forge. That is the identity `PerClientRateLimit` keys on, so it is the one that answers "whose bucket did that 429 come from"; pass `Observe` the same `TrustedProxies` you -give the limiter or the two will disagree. Unset means -`TrustedProxies::None()` — the deliberate direct-connect statement, under -which the peer is the client and the header is ignored wholly. Watch the -distribution of `client.source`: every request reporting `kDirectPeer` with -one address means you are behind a proxy and did not say so. +give the limiter or the two will disagree. With no boundary supplied the +derivation is skipped entirely — `client` stays empty with +`Source::kUnknown`, and the request pays for no header walk — so a chain +whose sinks never read it (a metrics-only composition) costs nothing here. +A deployment with no proxy tier says so explicitly with +`TrustedProxies::None()`, under which the peer is the client and the header +is ignored wholly. Watch the distribution of `client.source`: every request +reporting `kDirectPeer` with one address means you are behind a proxy and +did not say so. `handler_threw` separates "we crashed" from "the handler deliberately answered 500" — both report status 500 with no operation, and an access log @@ -563,11 +567,15 @@ your domain numbers behind another. Mint a family once and keep the handle: ```cpp auto orders = metrics->NewCounter("orders_processed_total", "Orders processed."); -auto latency = metrics->NewHistogram("order_pipeline_seconds", "Pipeline time."); +// Buckets are required — there is no default, because inheriting a latency +// ladder for a histogram of bytes or queue depth yields meaningless bins. +// For a request-shaped duration, the shared ladder is the right one. +auto latency = metrics->NewHistogram("order_pipeline_duration_microseconds", "Pipeline time.", + smithy::server::HttpLatencyBuckets()); auto depth = metrics->NewGauge("queue_depth", "Pending jobs."); orders.Increment({{"region", "us-east"}}); -latency.Observe(elapsed.count()); +latency.Observe(elapsed_micros.count()); depth.Set(pending); ``` diff --git a/runtime/include/smithy/server/middleware.h b/runtime/include/smithy/server/middleware.h index ea19479d..80fa903e 100644 --- a/runtime/include/smithy/server/middleware.h +++ b/runtime/include/smithy/server/middleware.h @@ -140,12 +140,12 @@ struct RequestObservation { // The client as derived from the L4 peer and x-forwarded-for (ADR-0012), // with its provenance — NOT the raw header, which a client can forge. This // is the identity PerClientRateLimit keys on, so it is the one that answers - // "whose bucket did that 429 come from". Derived against the TrustedProxies - // passed to Observe: unset means TrustedProxies::None(), the deliberate - // direct-connect statement, under which the peer is the client and the - // header is ignored wholly. `source` is worth reporting alongside the - // address — the *distribution* of sources across requests is the - // misconfiguration signal docs/production-guide.md reads. + // "whose bucket did that 429 come from". Populated only when Observe was + // given a TrustedProxies to derive against; with none supplied it stays + // value-initialized (empty address, Source::kUnknown) and the request pays + // for no derivation — see Observe below. `source` is worth reporting + // alongside the address: the *distribution* of sources across requests is + // the misconfiguration signal docs/production-guide.md reads. http::DerivedClient client{}; // True when the handler threw and Observe reported the contained 500 on its // behalf. Distinguishes "we crashed" from "the handler deliberately @@ -177,13 +177,20 @@ struct RequestStart { // tests (null means steady_clock). // `trusted` is the ADR-0012 trust boundary used to derive // RequestObservation::client. Pass the same one given to PerClientRateLimit, -// or a 429's bucket and the client an observation reports will disagree. -// Unset means TrustedProxies::None() — the deliberate direct-connect -// statement, which reports the peer itself. +// or a 429's bucket and the client an observation reports will disagree; for +// a deployment with no proxy tier, pass TrustedProxies::None() — the +// deliberate direct-connect statement, under which the peer is the client +// and the header is ignored wholly. +// +// Left unset, the derivation is skipped entirely and client stays +// value-initialized (Source::kUnknown). Deriving means a header lookup, an +// address parse, and string building on every request, which is pure waste +// in a chain whose sinks never read client — RecordMetrics deliberately +// does not, so the metrics-only composition pays nothing here. Middleware Observe(std::function on_complete, std::function on_start = nullptr, std::function now = nullptr, - http::TrustedProxies trusted = http::TrustedProxies::None()); + std::optional trusted = std::nullopt); // 401 unless the request carries "authorization: Bearer " (scheme // matched case-insensitively per RFC 6750) and validator(token) returns diff --git a/runtime/src/server/middleware.cc b/runtime/src/server/middleware.cc index a04b1c10..2c17bfd5 100644 --- a/runtime/src/server/middleware.cc +++ b/runtime/src/server/middleware.cc @@ -162,7 +162,7 @@ Middleware HealthEndpoint(std::string path, std::vector checks) Middleware Observe(std::function on_complete, std::function on_start, std::function now, - http::TrustedProxies trusted) { + std::optional trusted) { if (on_complete == nullptr) { smithy::internal::Fatal("smithy::server::Observe: on_complete may not be null"); } @@ -181,10 +181,14 @@ Middleware Observe(std::function on_complete, observation.target = request.target; observation.trace_parent = request.headers.Get("traceparent").value_or(""); observation.request_bytes = request.body.size(); - // Derived once here rather than per sink: the walk parses - // x-forwarded-for, and two sinks deriving it independently could - // disagree if they were handed different trust boundaries. - observation.client = http::DeriveClient(request, trusted); + // Derived once here rather than per sink — two sinks deriving + // independently could disagree if handed different boundaries — and + // only when a boundary was supplied at all: the parse and the string + // building are per-request costs, and a chain whose sinks never read + // client (RecordMetrics) should not pay them. + if (trusted.has_value()) { + observation.client = http::DeriveClient(request, *trusted); + } const auto start = now(); http::HttpResponse response; #if defined(__cpp_exceptions) diff --git a/runtime/tests/server/middleware_test.cc b/runtime/tests/server/middleware_test.cc index 580147fd..c4f3458d 100644 --- a/runtime/tests/server/middleware_test.cc +++ b/runtime/tests/server/middleware_test.cc @@ -179,9 +179,11 @@ TEST(ObserveTest, TheClientIsTheDerivedOneNotTheForgeableHeader) { TEST(ObserveTest, AnUntrustedPeerIsTheClientAndItsHeaderIsIgnored) { // The forgery case, and the reason the raw header is the wrong thing to // log: a direct client claiming to be someone else must not be believed. - // Unset trust is TrustedProxies::None(), so every peer is untrusted. + // TrustedProxies::None() is the direct-connect statement, so every peer is + // untrusted and its header is ignored wholly. std::vector observations; - auto handler = Chain({Observe([&](const RequestObservation& o) { observations.push_back(o); })}, + auto handler = Chain({Observe([&](const RequestObservation& o) { observations.push_back(o); }, + nullptr, nullptr, http::TrustedProxies::None())}, [](const http::HttpRequest&) { return Ok("served"); }); http::HttpRequest request; @@ -200,7 +202,8 @@ TEST(ObserveTest, AnUntrustedPeerIsTheClientAndItsHeaderIsIgnored) { TEST(ObserveTest, TheClientIsReportedOnTheThrownPathToo) { // The 500s are exactly when someone wants to know who was calling. std::vector observations; - auto handler = Chain({Observe([&](const RequestObservation& o) { observations.push_back(o); })}, + auto handler = Chain({Observe([&](const RequestObservation& o) { observations.push_back(o); }, + nullptr, nullptr, http::TrustedProxies::None())}, [](const http::HttpRequest&) -> http::HttpResponse { throw std::runtime_error("handler exploded"); }); @@ -217,6 +220,29 @@ TEST(ObserveTest, TheClientIsReportedOnTheThrownPathToo) { EXPECT_EQ(observations[0].request_bytes, 7u); } +TEST(ObserveTest, NoTrustBoundaryMeansNoDerivationAtAll) { + // The default. Deriving costs a header lookup, an address parse, and + // string building on every request — pure waste in a chain whose sinks + // never read client, which is every metrics-only composition. So with no + // boundary supplied the field stays value-initialized rather than being + // derived against an assumed one; a sink that wants the client states the + // trust boundary, the way PerClientRateLimit already must. + std::vector observations; + auto handler = Chain({Observe([&](const RequestObservation& o) { observations.push_back(o); })}, + [](const http::HttpRequest&) { return Ok("served"); }); + + http::HttpRequest request; + request.method = "GET"; + request.target = "/things"; + request.peer_address = "198.51.100.9"; + request.headers.Set("x-forwarded-for", "203.0.113.7"); + (void)handler(request); + + ASSERT_EQ(observations.size(), 1u); + EXPECT_EQ(observations[0].client.address, ""); + EXPECT_EQ(observations[0].client.source, http::DerivedClient::Source::kUnknown); +} + TEST(ObserveTest, TracesAreNeverEmptyWhenServedThroughATransport) { // ADR-0011: the transport ingress (server_dispatch.h) mints a root // traceparent when the client sent none, so an Observe composed under any