feat: add OpenTelemetry metrics - #3163
anish-sahoo wants to merge 13 commits into
Conversation
There was a problem hiding this comment.
I'm Bonk, and I've done a quick review of your PR.
This PR adds opt-in OpenTelemetry metrics (runtime + model), a metrics signal to cog.yaml, custom MeterProvider support via telemetry.py, and the parent/worker plumbing to export bounded prediction, rejection, setup, active, duration, and slot-state metrics.
Overall the design is coherent and well-tested (Go tests pass; Rust unit tests cover the registry, slot-state accounting, and protocol compat). I found two issues in the new runtime-metrics accounting worth addressing.
|
Review posted. Both inline findings were submitted successfully on PR #3163. Summary of review: I reviewed the full 5,300-line diff of this PR (Go config/dockerfile/image plumbing, Rust coglet runtime-metrics, Python SDK telemetry module, docs, and integration tests). Go tests for Two actionable findings were posted as inline comments (both with suggestions):
Everything else — the protocol |
|
I've completed a thorough review of the current state of PR #3163 (head I verified:
LGTM |
|
Verified: the only change since the previous LGTM review at LGTM |
|
LGTM |
|
I've reviewed the two commits pushed after the last LGTM review (
Docs ( No actionable issues found. LGTM |
Fix a select! busy-spin when the orchestrator handle drops, order slot state transitions before publishing permits, take a read lock on the rejection hot path, and record terminal counts for predictions that end before processing. Add value assertions for runtime metrics, fix doc inaccuracies, and dedupe the tracing extra behind observability.
|
LGTM |
bghira
left a comment
There was a problem hiding this comment.
One concern: both providers export their final cumulative snapshot twice during shutdown. This reproduced with the default export interval, so it’s caused by calling force_flush() immediately before shutdown():
.../python/cog/_telemetry.py:137
.../crates/coglet/src/runtime_metrics.rs:182
|
LGTM |
|
LGTM |
|
I've reviewed the current head ( Verification performed:
The coglet-python test failures in this environment are pre-existing and unrelated to the PR (SDK wheel not built; No actionable issues found. LGTM |
|
LGTM |
There was a problem hiding this comment.
I'm Bonk, and I've done a quick review of your PR.
This commit (d6e1344) hardens the metrics lifecycle and config compatibility: shutdown now finalizes in-flight predictions and poisons slots, dispatch-time request building avoids registering on failure, worker setup failures reap the child process, and the observability build plumbing disables inherited telemetry.
I found one issue worth addressing.
Summary of changes: opt-in OpenTelemetry metrics for Cog; this commit adds metrics lifecycle hardening plus config-compatibility fixes for telemetry env vars and the observability capability check.
| if provider is None: | ||
| return | ||
| try: | ||
| provider.force_flush() |
There was a problem hiding this comment.
This force_flush() re-adds what commit 3691e52 removed and re-opens the double-export that bghira reported. With the default PeriodicExportingMetricReader (60s interval) used by _create_default_meter_provider() and the documented custom provider, the final cumulative snapshot is exported twice on shutdown:
provider.force_flush()→ readercollect()→exporter.export()(export cuda selection logic #1).provider.shutdown()→ sets the shutdown event, then the ticker thread performs its "one last collection below before shutting down completely" →collect()→exporter.export()(export local refactor #2) before closing.
I verified this against opentelemetry-sdk 1.44.0 (the version pinned in uv.lock): PeriodicExportingMetricReader.force_flush() calls MetricReader.force_flush() → collect(), and shutdown() always performs a final collect() in the ticker before exporter.shutdown(). For cumulative instruments (the default temporality) the second export is a duplicate of the same snapshot.
test_shutdown_exports_metrics_without_a_periodic_thread does not catch this because it constructs the reader with export_interval_millis=math.inf, which disables the periodic thread entirely (no daemon thread is created), so the shutdown-path collect() never runs. The default configuration is exactly the case that double-exports.
shutdown() alone already exports the final snapshot for a periodic reader; the explicit force_flush() only duplicates it. Consider reverting to shutdown-only and updating the test to assert a single export with a default-interval reader:
| provider.force_flush() | |
| def _shutdown_provider( | |
| provider: TracerProvider | MeterProvider | None, signal: str | |
| ) -> None: | |
| if provider is None: | |
| return | |
| try: | |
| provider.shutdown() | |
| except Exception: | |
| _logger.exception("Failed to shut down Python %s provider", signal) |
|
Review: 1 finding. I'm Bonk, and I've done a quick review of your PR. The commit
|
This adds opt-in OpenTelemetry metrics to Cog. The Rust parent exports fixed runtime metrics, and the Python worker gets a standard
MeterProviderfor model metrics. Metrics are off by default. Existing response metrics do not change, and exporter failures do not affect predictions.User experience
Enable metrics in
cog.yaml:The boolean form
metrics: trueis accepted as shorthand.Collector settings stay at runtime:
No Python configuration is required for the defaults.
metrics.enabled: truecreates the runtime and Python providers when an OTLP endpoint is present.Runtime metrics
The parent exports six instruments:
cog.runtime.prediction.count{prediction}operation,statuscog.runtime.prediction.rejected{prediction}operation,reasoncog.runtime.prediction.active{prediction}operationcog.runtime.prediction.durationsoperation,statuscog.runtime.setup.durationsstatuscog.runtime.slot.count{slot}stateThe attribute values are closed sets:
operation:predictortrainstatus:succeeded,failed, orcanceledreason:invalid_input,not_ready, orat_capacitystate:available,busy, orpoisonedPrediction and setup histograms use fixed bucket boundaries. Request IDs, slot IDs, model inputs and outputs, and error text are not metric attributes.
Model metrics
Cog installs the Python provider before importing the model. Model code uses the normal OpenTelemetry API:
These instruments are separate from
record_metric(), which continues to populate the prediction response.The parent and worker providers can send to the same collector, but they remain independent. A custom Python provider does not replace the parent provider, and the worker does not duplicate the runtime metrics.
Custom Python telemetry
observability.configcan customize either Python signal:Factories are optional. If a factory is missing, Cog uses its default for that signal.
Cog loads this module before model import, validates returned providers, and owns flush and shutdown. Existing zero-argument tracer factories still work. Invalid custom factories or instrumentation fail setup instead of falling back silently.
Runtime metric selection
Models can disable Cog metrics without replacing the parent provider:
RuntimeMetricsConfig(enabled=False)disables all runtime metrics but leaves the Python provider available. Runtime instruments cannot be renamed or relabeled fromtelemetry.py.The worker sends the selection to the parent in its optional
Readyfield. Older workers that omit the field use defaults. If telemetry configuration fails before producing a valid selection, the failed setup metric also uses defaults.Accounting and lifecycle
Prediction totals, durations, and active decrements run from the guarded terminal setters. This prevents racing success, failure, and cancellation paths from recording twice.
Slot counts come from an explicit parent-owned state table. Available, busy, and poisoned counts remain disjoint and sum to configured concurrency.
The prediction dispatch path now unregisters stale routing state on every early return. An unstarted permit goes back to the pool only after unregister completes. Once a socket send begins, cancellation poisons the slot instead of making it available for another request.
Providers are flushed during graceful shutdown. Missing endpoints and invalid built-in exporter settings log a warning and leave serving unaffected. Generic HTTP endpoints receive
/v1/metrics; signal-specific endpoints are used as provided.Documentation and examples
The observability guide now covers tracing and metrics together. The YAML, environment, Python, build, and runtime references have also been updated.
examples/hello-concurrencyshows custom trace and meter providers, a model counter, and runtime metric selection.OpenTelemetry log export is not part of this PR.
Dependency bumps
Updates rustls to 0.23.45 to address RUSTSEC-2026-0285.