Skip to content

feat(sdk): 59790 configurable event sinks#160

Open
namrataghadi-galileo wants to merge 25 commits intomainfrom
feature/59790-configurable-event-sinks
Open

feat(sdk): 59790 configurable event sinks#160
namrataghadi-galileo wants to merge 25 commits intomainfrom
feature/59790-configurable-event-sinks

Conversation

@namrataghadi-galileo
Copy link
Copy Markdown
Contributor

Summary

  • add a configurable event ingestion abstraction so already-created ControlExecutionEvents can be delivered either through the existing built-in SDK observability path or through a registered custom ControlEventSink
  • keep event creation mode and event delivery sink as separate concerns:
  • merge_events controls where events are created
  • the sink controls where created events are delivered
  • preserve the existing OSS/default behavior while enabling merged event creation to use either the default sink or a custom sink
  • keep observability as the top-level gate for both event creation and event emission

Behavior

  • observability disabled: no control events are created or emitted, regardless of merge_events or sink registration
  • default creation mode (merge_events=False):
  • local events are created in the SDK and delivered through the existing built-in enqueue/Postgres path
  • server-side events are still created and ingested by the server
  • custom sinks are intentionally ignored in this mode
  • merged creation mode (merge_events=True) + default sink:
  • the SDK reconstructs local and server events after evaluation
  • the server skips final server-side ingestion
  • the merged batch is delivered through the existing built-in enqueue/Postgres path
  • merged creation mode (merge_events=True) + custom sink:
  • the SDK reconstructs local and server events after evaluation
  • the server skips final server-side ingestion
  • the merged batch is delivered through the registered custom sink
  • custom sink failures are treated as observability failures only and do not change evaluation outcomes

What changed

  • added configurable sink helpers in sdks/python/src/agent_control/telemetry/event_sink.py:
  • ControlEventSink
  • set_control_event_sink(...)
  • has_control_event_sink()
  • emit_control_events(...)
  • clear_control_event_sink()
  • re-exported the sink API from:
  • sdks/python/src/agent_control/telemetry/init.py
  • sdks/python/src/agent_control/init.py
  • updated sdks/python/src/agent_control/evaluation.py so:
  • merged mode still controls event creation
  • merged-mode delivery now goes through emit_control_events(...)
  • default creation mode continues to use the built-in enqueue path directly
  • merged mode remains gated on initialized session state and observability being enabled
  • updated SDK lifecycle cleanup in sdks/python/src/agent_control/init.py so:
  • shutdown() / reset clears the registered sink
  • init() also clears any previously registered sink at the start of a new SDK session to avoid stale sink leakage across re-init
  • added and updated tests:
  • sdks/python/tests/test_event_sink.py
  • sdks/python/tests/test_observability_updates.py
  • sdks/python/tests/test_shutdown.py

Testing

  • SDK tests were updated to cover:
  • default sink fallback behavior
  • merged creation with default sink
  • merged creation with custom sink
  • custom sink error handling
  • sink cleanup on shutdown and re-init
  • observability-disabled no-op behavior for event creation/emission

@namrataghadi-galileo namrataghadi-galileo changed the base branch from main to feature/59787-merge-events April 1, 2026 22:52
@codecov
Copy link
Copy Markdown

codecov bot commented Apr 1, 2026

@namrataghadi-galileo namrataghadi-galileo changed the base branch from feature/59787-merge-events to main April 1, 2026 22:55
@namrataghadi-galileo namrataghadi-galileo changed the base branch from main to feature/59787-merge-events April 1, 2026 22:55
@namrataghadi-galileo namrataghadi-galileo changed the base branch from feature/59787-merge-events to main April 1, 2026 22:56
Copy link
Copy Markdown
Contributor

@lan17 lan17 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking issues:

  • server/src/agent_control_server/endpoints/evaluation.py:242-263 now trusts X-Agent-Control-Merge-Events from 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-485 clears any previously registered control-event sink during init(). That breaks the natural setup order set_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.

Comment on lines -41 to -42
if not isinstance(trace_id, str) or not isinstance(span_id, str):
return None
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is taken care of in previous PR.

@namrataghadi-galileo
Copy link
Copy Markdown
Contributor Author

@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
init(..., merge_events=True); set_control_event_sink(...)
both preserve the custom sink. Sink cleanup still happens on shutdown/reset, not silently during initialization.

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().

Copy link
Copy Markdown
Contributor

@lan17 lan17 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-Events suppression 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 order set_control_event_sink(...); init(..., merge_events=True).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants