-
Notifications
You must be signed in to change notification settings - Fork 51
feat(observability): RPC + workflow + DB-pool metrics for the agent JSON-RPC endpoint #379
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 7 commits
a8817b5
2582bf2
0b9d30e
6e1f69c
9d0ae7d
1b757c1
5e94141
15ba465
0e754eb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,5 @@ | ||
| import secrets | ||
| import time | ||
| from collections.abc import AsyncIterator | ||
|
|
||
| from fastapi import APIRouter, HTTPException, Query, Request | ||
|
|
@@ -43,6 +44,9 @@ | |
| DAuthorizedResourceIds, | ||
| ) | ||
| from src.utils.logging import make_logger | ||
| from src.utils.rpc_metrics import ( | ||
| record_rpc_request, | ||
| ) | ||
| from src.utils.task_authorization import check_task_or_collapse_to_404 | ||
|
|
||
| logger = make_logger(__name__) | ||
|
|
@@ -549,6 +553,7 @@ async def _handle_sync_rpc( | |
| request_headers: dict[str, str] | None = None, | ||
| ) -> AgentRPCResponse: | ||
| """Handle synchronous JSON-RPC requests.""" | ||
| start = time.perf_counter() | ||
| try: | ||
| result_entity = await agents_acp_use_case.handle_rpc_request( | ||
| agent_id=agent_id, | ||
|
|
@@ -580,6 +585,11 @@ async def _handle_sync_rpc( | |
| # else: | ||
| # raise ValueError(f"Unsupported method: {request.method}") | ||
| # logger.info(f"AgentRPCResponse Result: {result}") | ||
| record_rpc_request( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. record_rpc_request fires before AgentRPCResponse.model_validate on the success path. If that validation raises, the except ValidationError branch records the same request a second time as a -32602 error, so agentex.rpc.requests and the duration histogram count it twice. Suggest validating first, then recording: response = AgentRPCResponse.model_validate(
{"id": request.id, "result": serialized_result, "error": None}
)
record_rpc_request(
method=request.method.value,
streaming=False,
duration_s=time.perf_counter() - start,
)
return response
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. good point, will record it after the validation to avoid double metricing
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 15ba465. Recording now goes through a |
||
| method=request.method.value, | ||
| streaming=False, | ||
| duration_s=time.perf_counter() - start, | ||
| ) | ||
| return AgentRPCResponse.model_validate( | ||
| { | ||
| "id": request.id, | ||
|
|
@@ -591,10 +601,24 @@ async def _handle_sync_rpc( | |
| except ValidationError as e: | ||
| logger.error(f"Validation error in RPC request: {e}", exc_info=True) | ||
| error = JSONRPCError(code=-32602, message=f"Invalid parameters: {e}") | ||
| record_rpc_request( | ||
| method=request.method.value, | ||
| streaming=False, | ||
| duration_s=time.perf_counter() - start, | ||
| error_code=error.code, | ||
| error_type=type(e).__name__, | ||
| ) | ||
| return AgentRPCResponse(id=request.id, error=error.model_dump(), result=None) | ||
| except Exception as e: | ||
| logger.error(f"Error handling JSON-RPC request: {e}", exc_info=True) | ||
| error = JSONRPCError(code=-32603, message=str(e)) | ||
| record_rpc_request( | ||
| method=request.method.value, | ||
| streaming=False, | ||
| duration_s=time.perf_counter() - start, | ||
| error_code=error.code, | ||
| error_type=type(e).__name__, | ||
| ) | ||
| return AgentRPCResponse(id=request.id, error=error.model_dump(), result=None) | ||
|
|
||
|
|
||
|
|
@@ -608,6 +632,8 @@ async def _handle_streaming_rpc( | |
| """Handle streaming JSON-RPC requests.""" | ||
|
|
||
| async def rpc_response_generator(): | ||
| start = time.perf_counter() | ||
| error_type: str | None = None | ||
| result_entity_async_iterator = None | ||
| try: | ||
| result_entity_async_iterator = await agents_acp_use_case.handle_rpc_request( | ||
|
|
@@ -640,6 +666,7 @@ async def rpc_response_generator(): | |
|
|
||
| except Exception as e: | ||
| logger.error(f"Error in streaming RPC response: {e}", exc_info=True) | ||
| error_type = type(e).__name__ | ||
| # Yield error response | ||
| error_response = AgentRPCResponse( | ||
| id=request.id, | ||
|
|
@@ -648,6 +675,13 @@ async def rpc_response_generator(): | |
| ) | ||
| yield error_response.model_dump_json().encode() + b"\n" | ||
| finally: | ||
| record_rpc_request( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: a client disconnect mid-stream raises GeneratorExit (or CancelledError) at the yield, which skips the except Exception branch, so this finally records the request as a success with a truncated duration. That deflates streaming duration percentiles and inflates the success rate. Suggest tracking error_code and error_type separately so disconnects carry error.type without counting as JSON-RPC errors (record_rpc_request already supports that combination, since the error counter is driven only by error_code): error_type: str | None = None
error_code: int | None = None
...
except (GeneratorExit, asyncio.CancelledError) as e:
# Client went away mid-stream: tag it, but not a JSON-RPC error.
error_type = type(e).__name__
raise
except Exception as e:
error_type = type(e).__name__
error_code = -32603
...
finally:
record_rpc_request(
...,
error_code=error_code,
error_type=error_type,
)That also lets dashboards exclude disconnects from percentiles via error.type, and drops the None if error_type is None else -32603 derivation. |
||
| method=request.method.value, | ||
| streaming=True, | ||
| duration_s=time.perf_counter() - start, | ||
| error_code=None if error_type is None else -32603, | ||
| error_type=error_type, | ||
| ) | ||
| # CRITICAL: Ensure the async iterator is properly closed | ||
| # This ensures HTTP connections are released back to the pool | ||
| if result_entity_async_iterator is not None and hasattr( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,6 +19,8 @@ | |
| from datadog import statsd | ||
| from sqlalchemy import event | ||
| from sqlalchemy.engine import ExecutionContext | ||
| from sqlalchemy.exc import TimeoutError as SQLAlchemyTimeoutError | ||
| from sqlalchemy.pool import AsyncAdaptedQueuePool | ||
|
|
||
| from src.utils.logging import make_logger | ||
| from src.utils.otel_metrics import get_meter | ||
|
|
@@ -39,6 +41,28 @@ | |
| # StatsD is only enabled if DD_AGENT_HOST is configured | ||
| _STATSD_ENABLED = bool(os.environ.get("DD_AGENT_HOST")) | ||
|
|
||
| # Bucket boundaries (seconds) for the connection-acquisition wait histogram. | ||
| # The SDK's default boundaries are millisecond-magnitude integers [0, 5, 10, ...] | ||
| # that assume a millisecond unit; against a seconds-unit metric almost every | ||
| # pool wait (µs–ms normally, up to the ~30s pool_timeout under saturation) falls | ||
| # in the first bucket and quantiles are meaningless. These span instant-to- | ||
| # timeout so p95/p99 stay usable. | ||
| _POOL_WAIT_BUCKET_BOUNDARIES_S = ( | ||
| 0.001, | ||
| 0.005, | ||
| 0.01, | ||
| 0.025, | ||
| 0.05, | ||
| 0.1, | ||
| 0.25, | ||
| 0.5, | ||
| 1.0, | ||
| 2.5, | ||
| 5.0, | ||
| 10.0, | ||
| 30.0, | ||
| ) | ||
|
|
||
|
|
||
| def _format_statsd_tags(attributes: dict) -> list[str]: | ||
| """Convert OTel attributes dict to Datadog StatsD tags list.""" | ||
|
|
@@ -70,6 +94,84 @@ def _parse_db_url(url: str) -> tuple[str, int, str]: | |
| return host, port, db_name | ||
|
|
||
|
|
||
| class _PoolWaitInstruments: | ||
| """OTel instruments + attributes for connection-acquisition metrics. | ||
|
|
||
| Held on the pool instance so ``connect()`` can emit without looking back | ||
| through the collector. A ``None`` slot on the pool means OTel is not | ||
| configured and the override degrades to a straight passthrough. | ||
| """ | ||
|
|
||
| __slots__ = ("wait_time", "pending_requests", "timeouts", "attributes") | ||
|
|
||
| def __init__(self, wait_time, pending_requests, timeouts, attributes): | ||
| self.wait_time = wait_time | ||
| self.pending_requests = pending_requests | ||
| self.timeouts = timeouts | ||
| self.attributes = attributes | ||
|
|
||
|
|
||
| class InstrumentedAsyncAdaptedQueuePool(AsyncAdaptedQueuePool): | ||
| """Async connection pool that measures connection acquisition. | ||
|
|
||
| Sources the three OTel DB connection-pool metrics that the checkout/checkin | ||
| events in ``PostgresPoolMetrics`` cannot: by the time ``checkout`` fires the | ||
| wait is already over, SQLAlchemy exposes no "started waiting" event, and a | ||
| pool timeout raises before any connection is handed out. | ||
|
|
||
| ``Pool.connect()`` is the single, non-recursive entry point for obtaining a | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice! |
||
| connection (``connect() -> _ConnectionFairy._checkout() -> _do_get()``) and | ||
| runs inside the acquiring greenlet, so wall-clock timing around it captures | ||
| the real wait — including time blocked on a saturated pool. Overriding | ||
| ``_do_get`` directly would double-count, because ``QueuePool._do_get`` | ||
| recurses on itself on the overflow-retry path. | ||
|
|
||
| Emits (OTel-only; no StatsD dual-emit, these are new series with no Datadog | ||
| consumer yet): | ||
| * ``db.client.connection.wait_time`` — time to obtain a connection | ||
| * ``db.client.connection.pending_requests`` — requests waiting right now | ||
| * ``db.client.connection.timeouts`` — pool acquisition timeouts | ||
|
|
||
| The pool is constructed inside ``create_async_engine`` before the collector | ||
| has meters, so instruments are attached post-construction; until then, and | ||
| whenever OTel is disabled, ``connect()`` is a plain passthrough. | ||
| """ | ||
|
|
||
| # None => passthrough (OTel disabled, or instruments not yet attached). | ||
| _wait_instruments: _PoolWaitInstruments | None = None | ||
|
|
||
| def attach_wait_instruments(self, instruments: _PoolWaitInstruments) -> None: | ||
| self._wait_instruments = instruments | ||
|
|
||
| def recreate(self) -> InstrumentedAsyncAdaptedQueuePool: | ||
| # dispose()/reset recreates the pool object; carry instrumentation | ||
| # forward so metrics don't silently go dark after a pool recreate. | ||
| new_pool = super().recreate() | ||
| new_pool._wait_instruments = self._wait_instruments | ||
| return new_pool | ||
|
|
||
| def connect(self): | ||
| instruments = self._wait_instruments | ||
| if instruments is None: | ||
| return super().connect() | ||
|
|
||
| attributes = instruments.attributes | ||
| instruments.pending_requests.add(1, attributes) | ||
| start = time.monotonic() | ||
| try: | ||
| connection = super().connect() | ||
| except SQLAlchemyTimeoutError: | ||
| instruments.timeouts.add(1, attributes) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So we're only capturing timeouts? Timeout is good but connect can fail for numerous reasons right?. Say, what if Postgres is rejecting a connection due to max connections already reached on the DB? Or there's a network drop? It's neither a timeout nor a successful wait, so it just goes dark.. worth adding a catch-all failure path for that.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point, i was adding for metrics in semconv and |
||
| raise | ||
| finally: | ||
| instruments.pending_requests.add(-1, attributes) | ||
|
|
||
| # Only reached on success: a timed-out (or otherwise failed) acquisition | ||
| # obtained no connection, so it must not land in the wait_time histogram. | ||
| instruments.wait_time.record(time.monotonic() - start, attributes) | ||
| return connection | ||
|
|
||
|
|
||
| class PostgresPoolMetrics: | ||
| """ | ||
| Collects and emits PostgreSQL connection pool metrics. | ||
|
|
@@ -151,6 +253,52 @@ def __init__( | |
| unit="s", | ||
| ) | ||
|
|
||
| # Connection-acquisition metrics (OTel DB semconv). Sourced from the | ||
| # InstrumentedAsyncAdaptedQueuePool.connect() seam rather than pool | ||
| # events, which can't see the wait, the current waiters, or a timeout. | ||
| self._connection_wait_time: Histogram = meter.create_histogram( | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess we should measure how long a connection is held.. So, here we are measuring the wait to acquire a connection but not how long it's been held. Pool exhaustion is a function of both i.e how long requests queue and how long connections stay checked out once acquired
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, hold time is covered above in |
||
| name="db.client.connection.wait_time", | ||
| description="The time it took to obtain an open connection from the pool", | ||
| unit="s", | ||
| explicit_bucket_boundaries_advisory=_POOL_WAIT_BUCKET_BOUNDARIES_S, | ||
| ) | ||
|
|
||
| self._connection_pending_requests: UpDownCounter = meter.create_up_down_counter( | ||
| name="db.client.connection.pending_requests", | ||
| description="The number of current pending requests for an open connection", | ||
| unit="{request}", | ||
| ) | ||
|
|
||
| self._connection_timeouts: Counter = meter.create_counter( | ||
| name="db.client.connection.timeouts", | ||
| description=( | ||
| "The number of connection timeouts that have occurred trying to " | ||
| "obtain a connection from the pool" | ||
| ), | ||
| unit="{timeout}", | ||
| ) | ||
|
|
||
| # Hand the instruments to the pool so its connect() override can emit. | ||
| # Only InstrumentedAsyncAdaptedQueuePool carries the hook; a differently | ||
| # configured engine (e.g. NullPool in tests) is left as a passthrough. | ||
| pool = self.engine.sync_engine.pool | ||
| if isinstance(pool, InstrumentedAsyncAdaptedQueuePool): | ||
| pool.attach_wait_instruments( | ||
| _PoolWaitInstruments( | ||
| wait_time=self._connection_wait_time, | ||
| pending_requests=self._connection_pending_requests, | ||
| timeouts=self._connection_timeouts, | ||
| attributes=self.base_attributes, | ||
| ) | ||
| ) | ||
| else: | ||
| logger.warning( | ||
| "Pool for %s is %s, not InstrumentedAsyncAdaptedQueuePool; " | ||
| "wait_time/pending_requests/timeouts will not be emitted", | ||
| pool_name, | ||
| type(pool).__name__, | ||
| ) | ||
|
|
||
| # Track last reported values for delta calculation | ||
| self._last_idle = 0 | ||
| self._last_used = 0 | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we pull this into a @contextmanager that handles start/end timing and records the metric (including error classification) in one place, rather than each handler managing start = time.perf_counter() and calling record_rpc_request at every exit point? That would let us reuse it across the library instead of repeating the same start/duration/record pattern in every function that needs rpc timing
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. Done in 15ba465 to use a context manager