feat(sdk): 59790 configurable event sinks#160
feat(sdk): 59790 configurable event sinks#160namrataghadi-galileo wants to merge 25 commits intomainfrom
Conversation
…re/59787-merge-events
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
lan17
left a comment
There was a problem hiding this comment.
Blocking issues:
-
server/src/agent_control_server/endpoints/evaluation.py:242-263now trustsX-Agent-Control-Merge-Eventsfrom the caller and skips server-side observability ingestion solely on that header. A direct API caller can set the header and suppress control-execution events without reconstructing/re-emitting them anywhere. This got riskier in this PR because the generated TS SDK now exposes that header as a public request field. -
sdks/python/src/agent_control/__init__.py:483-485clears any previously registered control-event sink duringinit(). That breaks the natural setup orderset_control_event_sink(...); init(..., merge_events=True), because the custom sink is silently discarded and merged events fall back to the default queue. If that ordering is intentional, it needs to be made explicit in the public contract; otherwise the sink should survive initialization.
| if not isinstance(trace_id, str) or not isinstance(span_id, str): | ||
| return None |
There was a problem hiding this comment.
Regression: removed type validation on user-supplied trace context provider
The isinstance(trace_id, str) / isinstance(span_id, str) guards were removed. If a provider returns non-string truthy values (e.g., integer IDs from OpenTelemetry-style providers), they now pass the if not trace_id check but cause httpx to raise TypeError when placed into headers. This changes the behavior from fail-open (ignore bad provider, continue evaluation) to fail-hard (crash evaluation).
Suggest restoring the isinstance checks. This change also appears unrelated to the event sink feature.
There was a problem hiding this comment.
This is taken care of in previous PR.
|
@lan17 Merged-mode trust boundary is now init-scoped, not header-only. The server no longer suppresses observability ingestion based on X-Agent-Control-Merge-Events alone. Merged mode must first be enabled during init(..., merge_events=True) / initAgent, and the server records that for the initialized (authenticated client, agent) session. On /evaluation, the server only honors X-Agent-Control-Merge-Events: true if that same client previously initialized that same agent with merge-events enabled; otherwise it keeps the default ingestion path. So exposing the header in the TS SDK does not by itself let an arbitrary caller suppress ingestion. Custom sinks now survive init(). I removed the clear_control_event_sink() call from init(), so the natural setup order now works as expected: set_control_event_sink(...); init(..., merge_events=True), and Summary: both issues are addressed in the current branch: merged suppression is now gated by a trusted init-scoped session instead of a caller-controlled header, and sink registration is no longer discarded during init(). |
lan17
left a comment
There was a problem hiding this comment.
Re-checked the current head 6496502; it has not changed since my last review.
So the existing requested-changes review still stands:
- this branch still has the original caller-controlled
X-Agent-Control-Merge-Eventssuppression issue because it has not picked up the newer session-gating fix that was added on top of#155. - it still clears any previously registered control-event sink in
sdks/python/src/agent_control/__init__.py:483-485, which silently breaks the setup orderset_control_event_sink(...); init(..., merge_events=True).
Summary
Behavior
What changed
Testing