Add stability metrics and API request tracking to telemetry - #1326
Open
r33drichards wants to merge 2 commits into
Open
Add stability metrics and API request tracking to telemetry#1326r33drichards wants to merge 2 commits into
r33drichards wants to merge 2 commits into
Conversation
Move OpenTelemetry from optional to required dependency in cua-core so that operational telemetry (latency, errors, throughput) is collected out of the box. Add stability-specific metrics: API request tracking (success/error/latency), a configurable latency target threshold for SLO monitoring, and a churn rate metric inspired by customer-happiness models that flags requests as "unhappy" when they fail or exceed the latency target. Key changes: - cua-core: OTel packages are now required dependencies - New OTel instruments: api_requests_total, api_request_duration, api_errors_total, api_requests_exceeding_latency_target - StabilityTracker class for in-process success/error/churn computation - CloudAPIClient._request instrumented with request-level telemetry - Removed try/except ImportError guards in agent/computer OTel code - 16 new tests covering tracker math, metric recording, and config https://claude.ai/code/session_013SPeuLkNwmAWgRvcT1pzPB
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughThe PR consolidates OpenTelemetry telemetry imports by removing conditional Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libs/python/core/cua_core/telemetry/otel.py`:
- Around line 697-715: The churn_rate calculation double-counts requests that
are both failed and slow; add an explicit counter self._unhappy_requests
initialized in __init__ and increment it in record when a request is failed OR
exceeds the latency target (i.e., if not success or duration_seconds >
self._latency_target). Then update the churn_rate computation to use
self._unhappy_requests / self._total_requests (instead of
min(self._failed_requests + self._slow_requests, self._total_requests)). Ensure
you update both the initializer (add self._unhappy_requests = 0), the record
method (increment whenever not success or slow), and the churn_rate logic (use
the explicit counter).
- Around line 770-782: get_stability_tracker currently returns a single
process-wide StabilityTracker that only accumulates monotonically; to make
stability_score reflect recent behavior, modify StabilityTracker (and its
constructor usage in get_stability_tracker) to implement a rolling/windowed
policy (e.g., time-windowed buckets, timestamped events with pruning, or a deque
of recent samples) or start an internal scheduler that periodically calls its
reset() or expires old entries; update get_stability_tracker to instantiate the
tracker with a time_window parameter and, if using scheduled resets, have the
tracker start a background timer/task on creation and expose a stop/cleanup
method to avoid leaks; ensure references to _stability_tracker, _tracker_lock,
StabilityTracker, get_stability_tracker, reset(), and stability_score are
updated accordingly so stability_score represents a recent sliding window rather
than a lifetime average.
In `@libs/python/core/tests/test_stability_metrics.py`:
- Around line 15-20: The temporary "core" module stub (_core_stub) is being
inserted into sys.modules at import time and persists for the entire test
session; instead, change the test to install the stub only for the duration of
each test by moving the sys.modules modification into a fixture or using
patch.dict/sys.modules or pytest's monkeypatch.setitem inside a setup fixture.
Specifically, replace the top-level sys.modules.setdefault("core", _core_stub)
with code that yields the stub from a fixture (or uses patch.dict/sys.modules or
monkeypatch.setitem) and ensures removal/restoration after each test so
_core_stub and sys.modules are scoped per-test.
In `@libs/python/cua-cli/cua_cli/api/client.py`:
- Around line 73-78: The call to record_api_request(...) is passing the raw path
variable which contains high-cardinality resource identifiers; update the call
site (where record_api_request is invoked) to pass a normalized low-cardinality
endpoint label instead of path — e.g., compute a route template or fixed
operation name (using a helper like normalize_path_to_template or a manual
mapping for operations such as "uploads:complete", "objects:get", "tags:list")
and pass that normalized string as the endpoint argument; keep method,
status_code, and duration as-is and ensure normalize logic is used wherever
record_api_request(...) is called so metrics use stable low-cardinality endpoint
labels.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f42fc9b6-aae1-43a5-911c-118fc18c0f70
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
libs/python/agent/cua_agent/callbacks/otel.pylibs/python/computer/computer/computer.pylibs/python/computer/computer/otel_wrapper.pylibs/python/core/cua_core/telemetry/__init__.pylibs/python/core/cua_core/telemetry/otel.pylibs/python/core/pyproject.tomllibs/python/core/tests/test_stability_metrics.pylibs/python/cua-cli/cua_cli/api/client.py
Comment on lines
+697
to
+715
| def __init__(self, latency_target: Optional[float] = None): | ||
| self._lock = Lock() | ||
| self._total_requests = 0 | ||
| self._failed_requests = 0 | ||
| self._slow_requests = 0 # exceeded latency target | ||
| self._latency_target = latency_target or _get_latency_target() | ||
|
|
||
| def record( | ||
| self, | ||
| success: bool, | ||
| duration_seconds: float, | ||
| ) -> None: | ||
| """Record an API request outcome.""" | ||
| with self._lock: | ||
| self._total_requests += 1 | ||
| if not success: | ||
| self._failed_requests += 1 | ||
| if duration_seconds > self._latency_target: | ||
| self._slow_requests += 1 |
Contributor
There was a problem hiding this comment.
churn_rate double-counts failed slow requests.
The current math only caps failed + slow at total, which is still wrong whenever the overlap is partial. Example: 10 requests where 2 are both failed and slow should yield churn 0.2, but this code reports 0.4. Track an explicit "unhappy requests" counter at write time instead of trying to reconstruct the union later.
Suggested fix
class StabilityTracker:
def __init__(self, latency_target: Optional[float] = None):
self._lock = Lock()
self._total_requests = 0
self._failed_requests = 0
self._slow_requests = 0 # exceeded latency target
+ self._unhappy_requests = 0 # failed or slow, counted once
self._latency_target = latency_target or _get_latency_target()
def record(
self,
success: bool,
duration_seconds: float,
) -> None:
"""Record an API request outcome."""
with self._lock:
self._total_requests += 1
+ is_slow = duration_seconds > self._latency_target
if not success:
self._failed_requests += 1
- if duration_seconds > self._latency_target:
+ if is_slow:
self._slow_requests += 1
+ if (not success) or is_slow:
+ self._unhappy_requests += 1
`@property`
def churn_rate(self) -> float:
with self._lock:
if self._total_requests == 0:
return 0.0
- unhappy = self._failed_requests + self._slow_requests
- return min(unhappy, self._total_requests) / self._total_requests
+ return self._unhappy_requests / self._total_requests
def reset(self) -> None:
with self._lock:
self._total_requests = 0
self._failed_requests = 0
self._slow_requests = 0
+ self._unhappy_requests = 0Also applies to: 741-751
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libs/python/core/cua_core/telemetry/otel.py` around lines 697 - 715, The
churn_rate calculation double-counts requests that are both failed and slow; add
an explicit counter self._unhappy_requests initialized in __init__ and increment
it in record when a request is failed OR exceeds the latency target (i.e., if
not success or duration_seconds > self._latency_target). Then update the
churn_rate computation to use self._unhappy_requests / self._total_requests
(instead of min(self._failed_requests + self._slow_requests,
self._total_requests)). Ensure you update both the initializer (add
self._unhappy_requests = 0), the record method (increment whenever not success
or slow), and the churn_rate logic (use the explicit counter).
Comment on lines
+770
to
+782
| # Global singleton tracker | ||
| _stability_tracker: Optional[StabilityTracker] = None | ||
| _tracker_lock = Lock() | ||
|
|
||
|
|
||
| def get_stability_tracker() -> StabilityTracker: | ||
| """Return the global :class:`StabilityTracker` instance, creating it if needed.""" | ||
| global _stability_tracker | ||
| if _stability_tracker is None: | ||
| with _tracker_lock: | ||
| if _stability_tracker is None: | ||
| _stability_tracker = StabilityTracker() | ||
| return _stability_tracker |
Contributor
There was a problem hiding this comment.
The global tracker never becomes a rolling/windowed signal.
get_stability_tracker() returns one process-wide accumulator that only changes monotonically unless some external caller remembers to invoke reset(). That means stability_score trends toward a lifetime average, so recent outages get diluted away instead of reflecting current stability. If this metric is meant for health checks or adaptive retry logic, it needs an actual time window or scheduled reset policy.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libs/python/core/cua_core/telemetry/otel.py` around lines 770 - 782,
get_stability_tracker currently returns a single process-wide StabilityTracker
that only accumulates monotonically; to make stability_score reflect recent
behavior, modify StabilityTracker (and its constructor usage in
get_stability_tracker) to implement a rolling/windowed policy (e.g.,
time-windowed buckets, timestamped events with pruning, or a deque of recent
samples) or start an internal scheduler that periodically calls its reset() or
expires old entries; update get_stability_tracker to instantiate the tracker
with a time_window parameter and, if using scheduled resets, have the tracker
start a background timer/task on creation and expose a stop/cleanup method to
avoid leaks; ensure references to _stability_tracker, _tracker_lock,
StabilityTracker, get_stability_tracker, reset(), and stability_score are
updated accordingly so stability_score represents a recent sliding window rather
than a lifetime average.
Comment on lines
+15
to
+20
| # The posthog module imports ``from core import __version__`` which relies on | ||
| # a namespace alias that may not be present in every environment. Provide a | ||
| # stub so that importing cua_core.telemetry doesn't blow up during tests. | ||
| _core_stub = types.ModuleType("core") | ||
| _core_stub.__version__ = "0.0.0-test" | ||
| sys.modules.setdefault("core", _core_stub) |
Contributor
There was a problem hiding this comment.
Scope the core module stub to each test instead of the whole session.
This mutates sys.modules at import time and leaves the stub in place for any later test that imports core, which can cause unrelated failures or mask the real package. Patch sys.modules inside a fixture or patch.dict(...)/monkeypatch.setitem(...) so it gets cleaned up automatically.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libs/python/core/tests/test_stability_metrics.py` around lines 15 - 20, The
temporary "core" module stub (_core_stub) is being inserted into sys.modules at
import time and persists for the entire test session; instead, change the test
to install the stub only for the duration of each test by moving the sys.modules
modification into a fixture or using patch.dict/sys.modules or pytest's
monkeypatch.setitem inside a setup fixture. Specifically, replace the top-level
sys.modules.setdefault("core", _core_stub) with code that yields the stub from a
fixture (or uses patch.dict/sys.modules or monkeypatch.setitem) and ensures
removal/restoration after each test so _core_stub and sys.modules are scoped
per-test.
Comment on lines
+73
to
+78
| record_api_request( | ||
| endpoint=path, | ||
| method=method, | ||
| status_code=status_code, | ||
| duration_seconds=duration, | ||
| ) |
Contributor
There was a problem hiding this comment.
Use a low-cardinality endpoint label here.
path is often resource-specific in this client (name, upload_id, part_number, tag), so recording it verbatim as endpoint will create a new metric series per object/upload. That can blow up OTEL cardinality and make these counters expensive or unusable in production. Please normalize this to a route template or a small fixed operation name before calling record_api_request().
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libs/python/cua-cli/cua_cli/api/client.py` around lines 73 - 78, The call to
record_api_request(...) is passing the raw path variable which contains
high-cardinality resource identifiers; update the call site (where
record_api_request is invoked) to pass a normalized low-cardinality endpoint
label instead of path — e.g., compute a route template or fixed operation name
(using a helper like normalize_path_to_template or a manual mapping for
operations such as "uploads:complete", "objects:get", "tags:list") and pass that
normalized string as the endpoint argument; keep method, status_code, and
duration as-is and ensure normalize logic is used wherever
record_api_request(...) is called so metrics use stable low-cardinality endpoint
labels.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR adds comprehensive stability metrics and API request tracking to the OpenTelemetry telemetry module. It introduces a
StabilityTrackerclass for in-process stability scoring, new OTel metrics for API request monitoring, and integrates telemetry recording into the CLI API client.Key Changes
Core Telemetry Enhancements (
cua_core/telemetry/otel.py)New OTel Metrics: Added four new metric instruments for API request tracking:
cua_sdk_api_requests_total: Counter for total API requests by endpoint and statuscua_sdk_api_request_duration_seconds: Histogram for request latencycua_sdk_api_errors_total: Counter for API errors by type and endpointcua_sdk_api_requests_exceeding_latency_target: Counter for SLO breachesLatency Target Configuration: Added
_get_latency_target()function to readCUA_LATENCY_TARGET_SECONDSenvironment variable (defaults to 30 seconds)API Recording Functions:
record_api_request(): Records successful and failed HTTP requests with status codesrecord_api_error(): Records connection-level failures (timeouts, DNS errors, etc.) with status code 0StabilityTracker Class: Thread-safe in-process tracker that computes:
success_rate: Fraction of successful requestserror_rate: Fraction of failed requestschurn_rate: Fraction of "unhappy" requests (failed OR exceeded latency target)stability_score: Overall stability (1.0 - churn_rate)reset()methodGlobal Singleton:
get_stability_tracker()function provides access to a globalStabilityTrackerinstanceCLI API Client Integration (
cua_cli/api/client.py)_request()method:time.perf_counter()record_api_request()record_api_error()StabilityTrackerfor both success and failure casesComputer Module Updates
OTEL_AVAILABLEchecks in:computer/otel_wrapper.pycomputer/computer.pyagent/callbacks/otel.pyDependency Changes (
pyproject.toml)[otel]extras to core dependencies[otel]and[telemetry]extras for backwards compatibility with existingpip install cua-core[otel]usagePublic API Exports (
cua_core/telemetry/__init__.py)StabilityTrackerget_stability_trackerrecord_api_requestrecord_api_errorNotable Implementation Details
StabilityTrackeruses a lock to protect concurrent access to countershttps://claude.ai/code/session_013SPeuLkNwmAWgRvcT1pzPB
Summary by CodeRabbit
New Features
Chores
Tests