Exact metric values, stricter fail-fast, and an opt-in client derivation

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.
 

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);
 ```
 

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

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<void(const RequestObservation&)> on_complete,
                    std::function<void(const RequestStart&)> on_start = nullptr,
                    std::function<std::chrono::steady_clock::time_point()> now = nullptr,
-                   http::TrustedProxies trusted = http::TrustedProxies::None());
+                   std::optional<http::TrustedProxies> trusted = std::nullopt);
 
 // 401 unless the request carries "authorization: Bearer <token>" (scheme
 // matched case-insensitively per RFC 6750) and validator(token) returns

runtime/src/server/metrics.cc

@@ -4,6 +4,7 @@
 #include <array>
 #include <cmath>
 #include <cstdio>
+#include <cstdlib>
 #include <string>
 #include <string_view>
 #include <utility>
@@ -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<char, 64> buffer{};
-  const int written = std::snprintf(buffer.data(), buffer.size(), "%.6f", value);
-  if (written <= 0) {
-    return "0";
-  }
-  std::string text(buffer.data(), static_cast<std::size_t>(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<std::size_t>(fixed) < buffer.size()) {
+    std::string text(buffer.data(), static_cast<std::size_t>(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<std::size_t>(written) < buffer.size() &&
+        std::strtod(buffer.data(), nullptr) == value) {
+      return std::string(buffer.data(), static_cast<std::size_t>(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<std::pair<std::string, std::string>> 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<std::mutex> 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<std::mutex> 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<std::mutex> lock(mutex);
   if (samples.contains(key)) {
     return;  // idempotent, and never disturbs a series already carrying events
@@ -414,9 +452,10 @@ std::shared_ptr<internal::MetricFamily> 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;
   }

runtime/src/server/middleware.cc

@@ -162,7 +162,7 @@ Middleware HealthEndpoint(std::string path, std::vector<ReadinessCheck> checks)
 Middleware Observe(std::function<void(const RequestObservation&)> on_complete,
                    std::function<void(const RequestStart&)> on_start,
                    std::function<std::chrono::steady_clock::time_point()> now,
-                   http::TrustedProxies trusted) {
+                   std::optional<http::TrustedProxies> 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<void(const RequestObservation&)> 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)

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();
 }