Bound the /metrics method label (closes #261) #264
Reference in New Issue
Block a user
Delete Branch "issue-261-metrics-method-label"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes #261.
methodwas recorded asr.Methodverbatim.net/httpaccepts any RFC 9110 token as a method and hands it through, so the label was bounded at nothing.The fix goes in the recorder seam #258 added for
handler— renamedroutePatternRecorder->boundedLabelRecorder, since it now bounds both request-controlled dimensions. A method the router can route is kept verbatim; anything else carries the existing(unmatched)sentinel, aliased (unmatchedMethod = unmatchedRoute) rather than respelled, so one idea does not read as two unmatched states in a scrape. Bound: 10 values.The retained set is the nine of chi's
methodMap, restated against thenet/httpconstants because chi does not export it. A token outside it can only ever produce chi's 405, so folding those together loses nothing a scrape could have used.chi.RegisterMethodwould extend the router's set; this service never calls it (verified:grep -rn RegisterMethodoverinternal/andcmd/returns nothing), and the code comment says a caller that started to must extend the switch with it.Reproduced first, then fixed
Both binaries built with
make build, run withMETRICS_USERNAME/METRICS_PASSWORDset, driven with 300 random 12-character method tokens against/, counting non-comment lines from an authenticated/metricsscrape:nextatfd5966f(unmodified)The
nextdelta of 7,525 lines matches the issue's measurement exactly (it reported 106 -> 7,631 against a warmer baseline). The branch's higher starting count is only because that instance had already been driven with real traffic for the audit below.The same reproduction at test scale,
TestMetrics_MethodSeriesCountIsFlatUnderAFloodrun against the unmodified recorder before the two-line wiring went in:Live label values from the fixed binary, showing the invented token
ZQXJ7BK2M4V9collapsed while the real methods stayed apart:Label audit
Enumerated from a live authenticated scrape of a running instance rather than from reading alone —
curl -u ... /metrics, every family and its label names extracted. That is the complete list of what/metricsexposes; nothing below was assumed present or absent.http_request_duration_seconds,http_response_size_byteshandlerroutePatternID, sentinel when unmatchedmethodmethodID(r.Method)codestrconv.Itoa(reporter.StatusCode())in go-http-metrics, whereStatusCode()reads the interceptor field written only byWriteHeadernet/httprejects anything outside 100-999http_requests_inflightserviceghmm.Config.Service, left unset inmetricsMiddlewarehttp_requests_inflighthandlerinflightHandlerconstant(all))http_request_duration_seconds,http_response_size_byteslewebhooker_*delivery metricstarget_typenormalizeTargetType, which folds anything outsideknownTargetTypestounknownwebhooker_events_received_total,webhooker_events_resubmitted_totalpromhttp_metric_handler_requests_totalcode200,500,503), observed in the scrapego_gc_duration_secondsquantilego_infoversiongo_*,process_*,promhttp_metric_handler_requests_in_flightHow each
nowas verified:code: traced the value tomiddleware.Measurein go-http-metrics, which formatsReporter.StatusCode();std.stdReporter.StatusCode()returnsresponseWriterInterceptor.statusCode, anintassigned only inWriteHeader. Nothing copies a request header or path into it. A request cannot name its own status.service:metricsMiddlewareconstructsghmm.New(ghmm.Config{Recorder: ...})and sets noService, so the field is""for the process lifetime. Confirmed empty in the live scrape above.target_type: every call site ininternal/metrics/metrics.gogoes throughnormalizeTargetType, andqueueDepthLabels/initSeriesenumerate the domain up front. Adatabase.TargetTypeoutside the four constants collapses tounknown.internal/deliveryandinternal/databasewere read for the call sites and not modified.go_*/process_*/promhttp_*families are registered byclient_golangon the default registry, which is the onepromhttp.Handler()gathers; their labels are compile-time constants of those collectors. Their presence and label sets are from the live scrape, not assumed.webhooker_delivery_duration_secondsdoes not appear in a scrape until the first delivery, becauseinitSeriesdoes not materialise it (unlike the counters and gauges). Its labels aretarget_type+le, both bounded as above. Noted as an observation, not a defect.No third unbounded dimension was found.
Tests
New, in
internal/middleware/metrics_method_test.go:TestMetrics_InventedMethodsMintOneLabelSet— 300 distinct invented tokens, exactly onemethodlabel, and no token echoed into any label. Asserts on the count rather than the set so a regression reports a number instead of dumping 300 tokens.TestMetrics_MethodSeriesCountIsFlatUnderAFlood— scrape, 300 tokens, scrape, 1,200 more, scrape; both series count and rendered exposition-line count must be flat. Lines are counted throughpromhttp.HandlerFor, i.e. the same handler/metricsmounts, because one histogram label set is ~25 lines and the line count is what the issue measured.TestMetrics_RealMethodsStayDistinct— the positive control. All nine routable methods (GET,POST,PUT,DELETE,HEAD,OPTIONS,PATCH,TRACE,CONNECT) keep their own duration series under the route pattern, one sample each.TestMetrics_MethodSentinelIsTheRouteSentinel— pins that the two sentinels are one string, so a future edit cannot introduce a second spelling.drivePathsinmetrics_test.gowas refactored onto a shareddrive/probehelper, since the probes now vary the method as well as the path; its signature and behaviour are unchanged.Verification
make checkgreen withGOFLAGS=-count=1: 580 tests pass across 20 packages,golangci-lintreports0 issues.in the Docker lint stage (summary line present, so the linter ran),fmt-checkclean. All probe processes stopped and scratch removed; ports 18961/18962 released.Out of scope and untouched: no new metrics, no changes under
internal/deliveryorinternal/database, noTODO.mdedit.The `method` label was recorded as `r.Method` verbatim. net/http accepts any RFC 9110 token as a method and passes it through, so the label was bounded at nothing: 300 requests to `/` carrying random 12-character method tokens took a live scrape from 81 lines to 7,606, unauthenticated and on a route with no rate limiter. This is the same remote memory-exhaustion vector the handler label carried, reached through a second dimension. The fix goes in the same recorder seam that bounds the handler label, which is renamed to reflect that it now bounds both. A method the router can route is kept verbatim, so real methods stay distinguishable; anything else carries the existing `(unmatched)` sentinel, deliberately the same spelling rather than a second one for the same idea. The bound is ten values. The retained set is restated against the net/http constants because chi's own methodMap is unexported. A token outside it can only ever produce chi's 405, so folding those together loses nothing a scrape could have used. README's Metrics section gains the inbound HTTP metrics, the bound on each of their labels, and the aggregate `http_requests_inflight{handler="(all)"}` semantics.PASS — independently reproduced and verified at
2ef52bb: on parentfd5966fmy own probe took a live scrape from 106 to 7,631 lines / 301methodvalues under 300 random 12-char tokens (601 after 600 tokens); this branch went 106 to 156 and stayed flat at 156 across a doubled flood. Positive control live: all nine chi methods keep distinct series with correctcode; 200 / 404 / 405 / 429 each landed with the rightcodeandmethod. Independent label enumeration from a live scrape found the complete label-name set to becode,handler,le,method,quantile,service,target_type,version— each traced to a bounded writer (promhttp_metric_handler_requests_totalobserved at exactly200/500/503); no third unbounded dimension.make checkgreen from a clean clone withGOFLAGS=-count=1in 1m25s: 580 tests / 20 packages / zero(cached)markers / 0 failures, lint0 issues.in the Docker stage under--no-cache-filter=lint(55.5s, not a cache hit),fmt-checkclean. CI green, mergeable, no attribution trailers,(closes #261)present.Anomalies and disclosures, none blocking:
get,PostandgEtall fold onto the sentinel. That is faithful to the router, not an accident: chi v1.5.5 upper-cases only at registration (Mux.Method), whilerouteHTTPdoes a baremethodMap[rctx.RouteMethod]lookup, so a lowercasegetcan only ever 405. Confirmed live.methodMapmatches the switch exactly — same ninenet/httpconstants in chi v1.5.5, andRegisterMethodis called nowhere in the repo or in any vendoredgo-chipackage. Residual risk of the duplicated source of truth:TestMetrics_RealMethodsStayDistinctwould catch chi dropping a method but not chi adding one, which would silently fold onto(unmatched). No clean guard exists while the map stays unexported, so I am waiving this rather than requiring it.handlerandmethodare separate label names and never merge series, and by constructionmethod="(unmatched)"implieshandler="(unmatched)"(chi 405s before it matches a pattern). So(unmatched)/(unmatched)reads as a bad method and(unmatched)/GETas a bad path — distinguishable, verified live.internal/middleware/metrics_method_test.go:189assert.Contains(t, keys(labels), ...)dumps all 300 tokens when it fails, which is the thing the comment three lines above says the count-based assertion was chosen to avoid.props.Method = methodID(...)lines in my own throwaway clone, ranmake test(both cardinality tests failed at 601 to 3,001 series / 7,501 to 37,501 lines, matching the PR body; the positive control correctly passed both before and after), then restored. Nothing was changed, committed or pushed here.deliveryDurationobservation: confirmed —webhooker_delivery_duration_secondsis genuinely absent from a fresh scrape. Real, out of scope for this PR, and worth a follow-up issue against #209. Not required here.