feat: improve metrics - #1033
Conversation
| METRICS.duration.record(elapsed_secs, &attrs); | ||
| if is_failed_request(status) { | ||
| METRICS.errors.add(1, &attrs); | ||
| } |
There was a problem hiding this comment.
delete error counter, we have response code in labels that's enough
Greptile SummaryThe PR adds HTTP request and duration telemetry, introduces a Neo4j request counter, and splits reach-sensitive graph-query labels by reach and WoT depth.
Confidence Score: 4/5The missing HTTP error instrument should be restored or the stated telemetry contract revised before merging. Failed HTTP responses are classified by the middleware, but the implementation exports no Files Needing Attention: nexus-webapi/src/routes/middlewares/tracing.rs
|
| Filename | Overview |
|---|---|
| nexus-webapi/src/routes/middlewares/tracing.rs | Adds bounded HTTP request telemetry and safer trace attributes, but omits the promised http.server.errors counter. |
| nexus-webapi/src/routes/mod.rs | Places tracing outside timeout, body-limit, CORS, and compression layers so short-circuit responses remain observable. |
| nexus-common/src/db/graph/instrumented.rs | Adds a Neo4j request denominator consistently across run completion, execute failure, and stream finalization. |
| nexus-common/src/db/graph/queries/get.rs | Splits three reach-scoped query families into stable labels for each reach mode and validated WoT depth. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
Client[HTTP client] --> MW[Tracing middleware]
MW --> Router[Matched Axum route]
Router --> Graph[Instrumented Neo4j graph]
MW --> HTTPMetrics[HTTP request and duration metrics]
Graph --> NeoMetrics[Neo4j request, error, duration, and row metrics]
HTTPMetrics --> OTLP[OTLP metrics exporter]
NeoMetrics --> OTLP
Reviews (1): Last reviewed commit: "chore: drop http.server.errors; status o..." | Re-trigger Greptile
| struct HttpMetrics { | ||
| requests: Counter<u64>, | ||
| duration: Histogram<f64>, | ||
| } |
There was a problem hiding this comment.
When a request returns 408 or 5xx, the middleware classifies it as failed but records only http.server.requests and duration, so the promised http.server.errors series is never exported and dashboards cannot calculate errors / requests using that instrument.
Knowledge Base Used: Nexus observability stack
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dd58930995
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| METRICS.requests.add(1, &attrs); | ||
| METRICS.duration.record(elapsed_secs, &attrs); |
There was a problem hiding this comment.
When a request returns 408 or 5xx, is_failed_request only marks the tracing span as failed, while this function records only http.server.requests and the duration histogram. Consequently, the advertised http.server.errors metric is never created or incremented, so operators cannot use the intended first-class error total without reconstructing it from status-labeled request series.
Useful? React with 👍 / 👎.
8c484c2 to
e435f2e
Compare
Summary
Essential request/error totals were missing or hard to find: HTTP had traces only, Neo4j had
errorsbut no first-class request counter, and reach-scoped graph queries lumped WoT with following under one label.http.server.requests,http.server.errors(5xx + 408), andhttp.server.request.duration(semconv seconds). Routes use the matched template (unmatchedotherwise); unknown methods collapse to_OTHER. Incomingtraceparentis ignored on this public API. Query strings are not recorded.neo4j.query.requestsnext to the existingneo4j.query.errors, samequerylabel, so error rate iserrors / requestswithout using histogram_count.wot.post_stream.requests/errorsalready existed. Trusted-network tag queries were already distinct Neo4j labels.get_*_by_reachlabels are now split by reach and WoT depth (…_following,…_wot_3, …) because those queries have no companion meter — depth cannot live as an attribute the way it does onpost_stream_wot.Pre-submission Checklist
Footnotes
Sample prompt: "Can this be simplified? Can the code, comments, or logic introduced by these changes be simplfied, clarified or otherwise made more terse, concise and understandable, without affecting functionality?" ↩
cargo bench -p nexus-webapi↩