Repository navigation
feat(telemetry): add simulation run context - #266
adityamehra wants to merge 1 commit into
Conversation
fercor-cisco
left a comment
fercor-cisco
left a comment
There was a problem hiding this comment.
A run ID that groups spans across conversations is useful, and you've wired it into both telemetry paths: spans created through the logger and native OpenTelemetry spans. Before merge, two things need fixing: a nested splunk_ao_context(...) drops the outer simulation_run() scope, and one test edit removed existing coverage. I'd also like to rename the attribute while it's still unreleased.
Please add a PR description that covers:
- the problem this solves
- why the run ID is a span attribute and not baggage or a Resource attribute
- how it's meant to interact with
splunk_ao_context(...),init()and session scopes
Rename the attribute. Please use splunk_ao.simulation.run_id instead of splunk_ao.simulation_run.id. A splunk_ao.simulation.* namespace leaves room for more simulation attributes later, such as scenario or persona IDs, without each one getting its own top-level prefix. Since this hasn't shipped, renaming now costs nothing. Later it would break consumers. Please update the README, ARCHITECTURE.md, CHANGELOG.md, AGENTS.md and the tests to match.
Cross-process propagation. The run ID lives only in a local context variable. It isn't in baggage and isn't carried by the session propagator wrapper. So when a simulation client calls an instrumented agent service over HTTP, the server-side spans won't have the run ID. If grouping across services is in scope, add the run ID to the propagator. If not, please state the limit in the README and ARCHITECTURE.md.
Docs. The new simulation_run_id= parameter on splunk_ao_context(...) is public, but the README and CHANGELOG.md only mention simulation_run(). Please document it, or drop it if simulation_run() is meant to be the only entry point.
| _experiment_id_context.set(None) | ||
| _mode_context.set(_get_mode_or_default(None)) | ||
| set_session_context(None) | ||
| _simulation_run_id_context.set(None) |
There was a problem hiding this comment.
Major. __call__ resets _simulation_run_id_context to None on entry, which breaks the README example. If a conversation wrapped in simulation_run("r") opens splunk_ao_context(session_id=...) or splunk_ao_context(project=...), no span inside that block gets the run ID.
A run ID groups spans; it isn't routing state, so it shouldn't be reset like project, Agent Stream or experiment. Please keep pushing the current value onto _simulation_run_id_stack so __exit__ restores it, but drop this reset and only override the value when simulation_run_id is passed:
| _simulation_run_id_context.set(None) | |
| # Simulation run scope is orthogonal to routing; inherit it unless explicitly overridden. |
Please add a regression test: with simulation_run("r"): with splunk_ao_context(project="p"): ... should export a span that carries "r".
| _span_stack_context.set([]) | ||
| _trace_context.set(None) | ||
| set_session_context(None) | ||
| _simulation_run_id_context.set(None) |
There was a problem hiding this comment.
Major (same root cause). init() also clears an active simulation_run() scope. If someone calls init() inside a run block, the rest of that block loses the run ID. I'd remove this line. reset() (line 1369) is an explicit full reset, so clearing there seems fine.
| assert _simulation_run_id_context.get() is None | ||
|
|
||
|
|
||
| def test_simulation_run_scope_restores_the_previous_value(monkeypatch: pytest.MonkeyPatch) -> None: |
There was a problem hiding this comment.
Major. The new test was inserted partway through test_nested_context_restoration. Everything from # First level context (line 51) onward now runs under test_simulation_run_scope_restores_the_previous_value. That has two effects:
- The original test now only checks that values start as
None, so its nesting coverage is gone. - The nesting block runs without the
AgentStreams,ProjectsandTracespatches or the reset fixture. It only passes becauseget_logger_instanceis monkeypatched.
Please move the new test after the original one and put the nesting block back. With the inheritance fix in decorator.py, the second-level assertion (_simulation_run_id_context.get() is None) should become == "run-1".
|
|
||
| if session_id: | ||
| span.set_attribute("gen_ai.conversation.id", session_id) | ||
| if simulation_run_id: |
There was a problem hiding this comment.
Minor. The two paths treat an empty run ID differently. This check is truthy, so simulation_run("") adds no attribute. SpanConverter.convert_span uses is not None, so the logger path exports "". Since simulation_run() and splunk_ao_context(simulation_run_id=...) are public inputs, I'd validate there and reject empty or non-string IDs with a ValueError. Then both paths can safely use the same check.
| start_time_ns = _to_unix_ns(span.created_at) | ||
| attributes = build_span_attributes(span, session_id) | ||
| if simulation_run_id is not None: | ||
| attributes["splunk_ao.simulation_run.id"] = simulation_run_id |
There was a problem hiding this comment.
Nit. The attribute name is a raw string literal in the converter, otel.py and the tests. Please define one constant next to SPLUNK_AO_SYSTEM in converter/attribute_mapping.py, using the renamed key, so the two paths can't drift:
SPLUNK_AO_SIMULATION_RUN_ID = "splunk_ao.simulation.run_id"
2 participants
No description provided.