Add OpenTelemetry metrics exporter built on MetricBuffer - #1701
Conversation
Resolves temporalio#1049 as scoped by @Sushisource: drain a MetricBuffer into a real OTel MeterProvider (views, resources, exemplars all come for free) rather than wiring MetricMeter across a multiprocessing queue. MetricsExporter polls MetricBuffer.retrieve_updates() on a fixed interval and maps buffered updates onto the OTel metrics API: counters via Counter.add() (buffered counter values are deltas), histograms via Histogram.record(), and gauges via create_observable_gauge() backed by a lock-protected last-value cache (OTel invokes gauge callbacks from its own export thread, concurrently with the drain loop). Instrument and attribute objects are cached keyed by id(), exploiting the identity-stability guarantee BufferedMetric/attributes already document. Lifecycle (run/shutdown/async context manager) mirrors Worker's existing asyncio.Event-based shutdown pattern rather than using threads, since that's the only such pattern already in this codebase. Bumps the opentelemetry extra's floor from >=1.11.1 to >=1.12.0 for both api and sdk -- the public opentelemetry.metrics module simply doesn't exist at 1.11.1, confirmed by direct import against the wheel. uv.lock is left untouched; CI resolves it fresh via `uv sync --all-extras` with no --frozen/--locked gate on PR checks. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b9b56ac441
ℹ️ 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".
| if self._started: | ||
| raise RuntimeError("MetricsExporter is already running") | ||
| self._started = True | ||
| self._shutdown_event.clear() |
There was a problem hiding this comment.
When an async with body exits without first suspending, __aenter__ has only scheduled run() and __aexit__ can call shutdown() before that task executes. shutdown() sets the event and waits, but run() then clears the pending signal here and polls forever, so context exit hangs indefinitely. Start-up needs a readiness handshake or must not erase a shutdown request that can already have been issued.
Useful? React with 👍 / 👎.
| [project.optional-dependencies] | ||
| grpc = ["grpcio>=1.48.2,<2"] | ||
| opentelemetry = ["opentelemetry-api>=1.11.1,<2", "opentelemetry-sdk>=1.11.1,<2"] | ||
| opentelemetry = ["opentelemetry-api>=1.12.0,<2", "opentelemetry-sdk>=1.12.0,<2"] |
There was a problem hiding this comment.
When an environment installs only temporalio[lambda-worker-otel] under constraints selecting its permitted OpenTelemetry 1.11.1 floor, temporalio.contrib.aws.lambda_worker.otel imports temporalio.contrib.opentelemetry, whose new unconditional MetricsExporter import requires the public opentelemetry.metrics module introduced in 1.12. The regular extra is raised here, but lambda-worker-otel still declares API/SDK >=1.11.1, so a supported installation now fails during import; raise those floors as well or avoid importing the metrics module for tracing-only consumers.
Useful? React with 👍 / 👎.
| "TracingWorkflowInboundInterceptor", | ||
| "OpenTelemetryInterceptor", | ||
| "OpenTelemetryPlugin", | ||
| "MetricsExporter", |
There was a problem hiding this comment.
This exports a new user-facing MetricsExporter API but the commit leaves the Unreleased changelog unchanged. Add a high-level entry under the existing CHANGELOG.md convention as required by the repository contributor guidance.
AGENTS.md reference: AGENTS.md:L72-L78
Useful? React with 👍 / 👎.
1 participant
What was changed
Adds
MetricsExportertotemporalio.contrib.opentelemetry: drains atemporalio.runtime.MetricBufferon a fixed interval and exports through a real OpenTelemetryMeterProvider.Counter.add()(buffered counter values are deltas).Histogram.record().create_observable_gauge()backed by a lock-protected last-value cache, since OTel invokes gauge callbacks from its own export thread concurrently with the drain loop.id(), exploiting the identity-stability guaranteeBufferedMetric/BufferedMetricUpdate.attributesalready document.run()/shutdown()/async with) mirrorsWorker's existingasyncio.Event-based shutdown pattern, since that's the only background-polling precedent already in this codebase (no threading precedent exists in the Python layer).retrieve_updates()raisingRuntimeError(buffer never attached to a constructedRuntime) propagates out ofrun()rather than spinning silently. Any other per-update failure is isolated, logged, and optionally reported via anon_errorcallback, without blocking the rest of that drain batch.Also bumps the
opentelemetryextra's floor from>=1.11.1to>=1.12.0for bothopentelemetry-apiandopentelemetry-sdk— this is required, not a preference: the publicopentelemetry.metricsmodule does not exist at 1.11.1 (confirmed by direct import against the wheel), so the feature can't be built without it. Kept the sdk floor in lockstep with the api floor, matching how they're already pinned together.uv.lockis left untouched. Regenerating it locally pulled in ~1600 unrelated lines from pre-existing lockfile drift unrelated to this change; CI resolves fresh viauv sync --all-extraswith no--frozen/--lockedgate on PR checks, so this shouldn't block anything.Why?
Resolves the scope @Sushisource set in #1049: rather than wiring
MetricMeteracross a multiprocessing queue (the issue's original ask), drainMetricBufferinto a real OTelMeterProviderdirectly, so users get full access to standard OTel features (views, resource, exemplars/tracing-integration) instead of a second, narrower metrics abstraction.Validation
pytest tests/contrib/opentelemetry/test_metrics_exporter.py— 7/7 passing, against a real locally-built native bridge extension (not skipped/mocked): counter delta accumulation across multiple drains, gauge latest-value-wins semantics, histogram count/sum, attribute passthrough, description/unit passthrough, the buffer-not-attachedRuntimeError, fullasync withstart/shutdown lifecycle (background task actually completes, doesn't leak), and error-in-one-update-doesn't-block-the-rest isolation.ruff check/ruff format --check— clean on all changed/new files.mypy/pyright— 0 errors on all changed/new files.AI assistance disclosure
Claude Code assisted with implementation: designed the class after exploring
MetricBuffer's API and this repo's existingcontrib/opentelemetryandWorkerlifecycle conventions, verified the 1.11.1 version-floor issue by directly inspecting the wheel rather than assuming, and installed Rust/protobuf to build the native bridge extension locally so the test suite could actually run rather than being left unverified. I reviewed the design and the tradeoffs above (asyncio-task lifecycle over threading, observable-gauge-plus-lock over a synchronous gauge API, fail-fast vs. log-and-continue error handling) and can defend them in review.Fixes #1049.