Skip to content

feat(browser): opt-in OpenTelemetry OTLP metrics export; v1.13.0 - #30

Open
harper-joseph wants to merge 2 commits into
mainfrom
feat/otel-metrics
Open

feat(browser): opt-in OpenTelemetry OTLP metrics export; v1.13.0#30
harper-joseph wants to merge 2 commits into
mainfrom
feat/otel-metrics

Conversation

@harper-joseph

Copy link
Copy Markdown
Contributor

Summary

Adds an opt-in OpenTelemetry metrics path to @harperfast/prerender-browser (v1.13.0). When
metrics.enabled + metrics.otlpEndpoint are set, the worker folds each stats window into OTel
counters/histograms/gauges and pushes them over OTLP/HTTP to a collector (Grafana Alloy). Off by
default — a disabled worker loads no OpenTelemetry code and behaves exactly as before.

Why

The render fleet already emits rich per-window stats via logStats, but only as pino logs. This
lets those numbers land in Grafana Cloud as real Prometheus metrics (via the Alloy stack already in
the cluster) so we get fleet-wide dashboards/alerting — including correct fleet-wide latency
quantiles, which per-worker pre-aggregated percentiles can't give.

Chosen over a logs→metrics or scrape approach because instrumenting the shared library (a) versions
the metric definitions with the code, (b) benefits every customer (kohls/stbhb/mcy…) on their next
bump, and (c) works with the multi-process fork model (each worker pushes independently). The fleet
is CPU-constrained (Guaranteed 14-core pods, saturated at peak), so the export is engineered to cost
effectively nothing on the render path (see below).

Design / where to look

  • src/metrics.ts (new) — lazy-imports the OTel SDK (dynamic import()), so nothing loads
    unless a worker enabled metrics. Counters + explicit-bucket histograms (render.duration,
    render.phase.duration) + observable gauges. Export is periodic and bounded/fire-and-forget:
    a slow or down collector can never block, delay, throw into record(), or fail a render.
  • src/Worker.ts — single integration point: logStats() calls metrics.record() once per
    window, reusing the aggregation it already computes (no new hot-path/per-render work). destroy()
    flushes on shutdown with a 2s cap so a down collector can't delay container exit.
  • src/index.ts — constructs the recorder only when enabled (the sole thing that loads OTel);
    init failure logs and continues without metrics. Redacts metrics.headers values from the config log.
  • src/settings.ts — new metrics option + resolution; defaults to disabled.

Attention / self-review notes (⚠️ cross-model CLI review couldn't run locally)

The Codex/Gemini CLIs weren't usable in my environment (Codex auth token expired; Gemini has no API
key), so the mandated two-model review did not run — please lean on the AI PR reviewer + a human
pass. Areas I'd focus a reviewer on:

  • OTel 2.x API usage (AggregationType views, resourceFromAttributes, PeriodicExportingMetricReader) — 2.0 changed these; verified against the installed type defs and unit tests.
  • Non-blocking guarantees — is there any path where a failed/slow export touches a render? (Intent: no; export runs on the reader's own timer, record() is guarded, shutdown flush is capped.)
  • New dependency footprint — adds @opentelemetry/{api,sdk-metrics,resources,exporter-metrics-otlp-proto}. The 3 npm audit highs (undici, ws, js-yaml) are pre-existing (undici direct dep; ws/js-yaml via mqtt/puppeteer), not introduced here.
  • Histogram bucket boundaries — tuned for seconds-scale render/settle times; sanity-check the ranges.

Testing

  • New test/metrics.test.ts (record + shutdown never throw, incl. collector-unreachable and
    all-zero/null-gauge windows) and settings coverage. node --test green (15/15).
  • tsc build, eslint, and prettier all clean. Verified the compiled dist/ keeps the imports
    dynamic (no static @opentelemetry in index.js/Worker.js) — disabled path loads nothing.
  • renderOnce.test.ts needs a real Chrome (not installed in my sandbox) and is unaffected by this change.

Follow-ups (not in this PR)

  1. render-service bump to pass metrics.enabled/otlpEndpoint from env (after this releases as a tarball).
  2. DevOps: Alloy OTLP receiver — the cluster's Alloy has no OTLP receiver yet; a short runbook to
    enable one in the Grafana k8s-monitoring Helm release (routing to the existing Grafana Cloud
    remote_write) is prepared and will be handed to devops.

🤖 Generated by an agent (Claude). The cross-model CLI review step was skipped due to local tooling auth — please ensure a review runs before merge.

Add an opt-in metrics path. When `metrics.enabled` and `metrics.otlpEndpoint`
are set, the worker folds each stats window — the same delta counts and raw
per-render timing samples `logStats` already computes — into OpenTelemetry
counters, histograms, and gauges and pushes them over OTLP/HTTP.

Design notes:
- Off by default. The OpenTelemetry SDK is dynamically imported only when
  enabled, so a disabled worker loads no OTel code and pays zero cost.
- Single integration point: `RenderWorker.logStats` calls `metrics.record()`
  once per window (reusing the existing aggregation — no new hot-path work).
- Histograms record the raw ms samples (not the pre-aggregated percentiles) so
  fleet-wide quantiles can be re-derived correctly across workers/pods.
- Export is periodic and bounded (fire-and-forget): a slow/unreachable
  collector never blocks, delays, throws into record(), or fails a render, and
  the shutdown flush is capped so it can't delay container exit.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces opt-in OpenTelemetry metrics export over OTLP/HTTP for the render worker. It adds OpenTelemetry dependencies, implements a lazy-loaded metrics recorder module to track various performance and resource utilization metrics, and integrates it into the worker lifecycle. The feedback highlights an improvement opportunity in Worker.ts to call this.logStats() during the destroy() sequence, ensuring that any metrics accumulated since the last interval are captured and exported before shutdown.

// throws). Cap the wait so a down collector's flush retries can't delay container exit; a
// dropped final window is an acceptable trade at shutdown. AbortController cancels the cap
// timer once the flush wins so it doesn't linger on the event loop (mirrors the drain above).
if (this.metrics) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

During graceful shutdown or uncaught exceptions, destroy() is called to clean up resources. However, any metrics accumulated since the last 60-second logStats() interval are currently lost because logStats() is not called before shutting down the metrics provider. Calling this.logStats() at the start of destroy() ensures that the final window's metrics are captured, logged, and successfully exported via OpenTelemetry. Wrapping it in a try-catch block ensures that any unexpected errors during stats collection (e.g., during an uncaught exception) do not block the critical browser cleanup process.

Suggested change
if (this.metrics) {
try {
this.logStats();
} catch (err) {
logger.warn({ err }, 'failed to log final stats during destroy');
}
if (this.metrics) {

… down metrics

destroy() shut down the metrics provider without folding in the stats accumulated
since the last 60s logStats() tick, so a graceful shutdown/deploy dropped up to a
full window of counts/timings from the export. Call logStats() once at the start of
destroy() (guarded, before this.metrics is cleared) so the final window is logged and
recorded into the OTel instruments ahead of the flush.

Addresses gemini-code-assist review on PR #30.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@harper-joseph

Copy link
Copy Markdown
Contributor Author

Addressed the review finding in 1ccc4e0.

destroy() now calls logStats() once (guarded, before this.metrics is cleared) so the up-to-one-window of counts/timings accumulated since the last 60s tick is logged and folded into the OTel instruments before the flush — no more losing the final window on graceful shutdown/deploy. Adjusted the flush comment so the two no longer read as contradictory. Build + tests + lint green.

(AI-generated: authored by an agent addressing the gemini-code-assist comment.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant