feat(fetch): record outgoing fetch duration by external host - #1218
feat(fetch): record outgoing fetch duration by external host#1218nicacioliveira wants to merge 3 commits into
Conversation
This wrapper already measured the duration of every outgoing fetch — it just
discarded it unless a logger happened to be installed, and `logger` is null in
production:
const start = logger && performance.now();
So there was no metric answering "is the external API slow, or are we making
too many calls to it?". The only alternative was `otel_traces`, which is
tail-sampled at ~1.7% and carries no client spans for these calls at all. In
practice that meant a `load-data` span of 157s could not be attributed to
anything, and diagnosis fell back to guessing.
Adds an `outgoing_fetch_duration` histogram, dimensioned by external host and
status class.
Notes on the design, both copied from what already works in this repo:
- `unit: "ms"`. The meter provider in `observability/otel/metrics.ts` selects
bucket boundaries by unit, so "ms" picks up
`[10, 100, 500, 1000, 5000, 10000, 15000]` automatically. Recording seconds
would put every observation in the first bucket — which is exactly the bug
the @decocms/start runtime currently has on its four duration metrics
(99.8%-99.95% in bucket 1, measured).
- Low cardinality by construction: `server.address` is the host, never the path,
and status is bucketed into a class rather than the raw code. Measured on a
large VTEX storefront, a site talks to 6 distinct hosts, so this is ~6 x 5
series per site. For comparison, `loader_cache` reaches 3684 distinct label
values on a single site because it uses the full resolver chain as a label.
- Failures are recorded, not dropped. A call that hangs for 60s and then aborts
is the sample you most want and the one a success-only path loses. The error
is re-thrown untouched.
- `hostOf` returns null instead of throwing on a malformed input, and a null
host skips the sample. A metric must not be able to break the fetch path.
The logger behaviour is unchanged.
Verified: `deno check runtime/fetch/fetchLog.ts` clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Tagging OptionsShould a new tag be published when this PR is merged?
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe change instruments ChangesFetch Metrics Instrumentation
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant createFetch
participant Histogram
participant ExternalHost
Caller->>createFetch: invoke fetch(request)
createFetch->>createFetch: start timing
createFetch->>ExternalHost: send request
alt request succeeds
ExternalHost-->>createFetch: response with status
createFetch->>Histogram: record duration by host and status class
createFetch-->>Caller: return response
else request fails
ExternalHost-->>createFetch: transport error
createFetch->>Histogram: record duration as "error"
createFetch-->>Caller: rethrow original error
end
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@runtime/fetch/fetchLog.ts`:
- Around line 46-50: Update the hostOf function to return URL.hostname instead
of URL.host for string, URL, and Request inputs, preserving the existing parsing
and null-on-error behavior so port numbers do not affect the external host
metric label.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 98fafbff-04b2-4b53-ba4a-2df869cddc20
📒 Files selected for processing (1)
runtime/fetch/fetchLog.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 1 file
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
`URL.host` appends a non-default port, so `example.com:8080` and `example.com` would become two distinct `server.address` label values for the same host — inflating exactly the cardinality this metric is careful about, and undercutting the "6 hosts x 5 classes per site" claim in its own docstring. `hostname` also matches semconv, where `server.address` is the address alone and `server.port` is a separate attribute. Caught by CodeRabbit on #1218. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Good catch — applied in d24f22a.
Switched to |
Authority-less schemes — `data:`, `blob:`, `file:` — parse fine but have no hostname, so they returned "" and `record` only skipped on null. That would have created a meaningless `server.address=""` series for calls that never crossed the network. Also collapses the three-branch return into a single URL construction. Verified: https://a.com/x -> a.com https://a.com:8080/x -> a.com data:text/plain,hi -> null file:///tmp/x -> null nonsense -> null Caught by cubic on #1218. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Both cubic findings addressed.
Authority-less URLs produce an empty host (line 50) — valid and not covered by that fix. Verified behaviour:
|
2 participants
Problem
createFetchalready measures the duration of every outgoing fetch. It just throws the number away unless a logger happens to be installed — andloggerisnullin production:So there is no metric that answers "is the external API slow, or are we making too many calls to it?". The only alternative is
otel_traces, which is tail-sampled at ~1.7% and carries no client spans for these calls at all.That gap is not academic. Investigating a storefront yesterday,
load-dataspans of 157s could not be attributed to anything — we could measure VTEX by hand from inside the pod (TTFB 0.39–0.55s, healthy) and measure the page (fast TTFB), but nothing connected the two. Every hypothesis about where the time went was a guess.Change
An
outgoing_fetch_durationhistogram, dimensioned by external host and status class.Three design points, all copied from patterns already in this repo rather than invented:
unit: "ms". The meter provider inobservability/otel/metrics.tsselects bucket boundaries by unit — declaring"ms"automatically picks up[10, 100, 500, 1000, 5000, 10000, 15000]. That mechanism is worth calling out because the@decocms/startruntime lacks it and currently records seconds into millisecond buckets on all four of its duration metrics: 99.8%–99.95% of observations land in bucket 1 (measured on production ClickHouse), making every quantile there meaningless. This metric avoids that by construction.Low cardinality by construction.
server.addressis the host, never the path, and status is bucketed into a class rather than the raw code — roughly 6 hosts × 5 classes per site (measured: a large VTEX storefront talks to 6 distinct hosts). For contrast,loader_cachereaches 3,684 distinct label values on a single site and 21,849 fleet-wide, because it uses the full resolver chain (Categories@sections.variants.1.value.5.sections.0.section.page) as a label. That is a separate problem, but it is the reason this metric does not take a per-loader dimension.Failures are recorded, not dropped. A call that hangs and then aborts is the sample you most want, and a success-only path loses exactly those. The error is re-thrown untouched.
hostOfreturnsnullrather than throwing on malformed input, and a null host skips the sample — a metric must not be able to break the fetch path.Logger behaviour is unchanged.
What this does and does not give you
It attributes time spent inside a single outbound call, per host. It does not attribute time spent between calls — serial fan-out, block resolution, cache writes. If a 157s request turns out to be 200 sequential 700ms calls, this metric shows 200 healthy samples and the aggregate stays unexplained;
resolver_latencyis the signal for that, and it is currently emitted by only 5 tenants for reasons still unknown.So this closes one specific gap rather than the whole attribution problem, and I would rather say that plainly than oversell it.
Verification
deno check runtime/fetch/fetchLog.ts— clean.Not verified: the metric has not been observed end-to-end in ClickHouse, since that needs a release. Worth confirming the series count per tenant after the first deploy — the expectation is single digits.
🤖 Generated with Claude Code
Summary by cubic
Record duration of all outgoing fetches by external host and status class to surface third‑party API latency in production. Adds a low‑cardinality ms histogram and captures failures.
New Features
outgoing_fetch_durationhistogram (unit:ms).server.address(hostname only) andhttp.response.status_class(2xx/3xx/4xx/5xx/error).data:,blob:,file:).Bug Fixes
URL.hostnameforserver.addressto avoid port-based label splits and match semconv.nullto prevent aserver.address=""series.Written for commit 972ebe3. Summary will update on new commits.
Summary by CodeRabbit