Repository navigation
SAO-17698: Address review comments - #271
adityamehra wants to merge 2 commits into
Conversation
fercor-cisco
left a comment
fercor-cisco
left a comment
There was a problem hiding this comment.
Moving SPLUNK_AO_SIMULATION_RUN_ID into simulation_run_context.py makes sense. One blocking issue: the leftover re-export in attribute_mapping.py fails Ruff F401, and the pre-commit hook deletes it automatically, which breaks five test modules. There is also a small test gap on the converter path. Details are inline.
| from galileo_core.schemas.logging.span import AgentSpan, LlmSpan, RetrieverSpan, ToolSpan, WorkflowSpan | ||
| from galileo_core.schemas.logging.step import BaseStep, StepType | ||
| from splunk_ao.logger.control import ControlSpan | ||
| from splunk_ao.simulation_run_context import SPLUNK_AO_SIMULATION_RUN_ID |
There was a problem hiding this comment.
Nothing in this module uses this import any more. It only keeps the old splunk_ao.converter.attribute_mapping import path working. Ruff 0.15.22, the version pinned in pre-commit, flags it:
$ ruff check --no-fix src/splunk_ao/converter/attribute_mapping.py
F401 [*] `splunk_ao.simulation_run_context.SPLUNK_AO_SIMULATION_RUN_ID` imported but unused
--> src/splunk_ao/converter/attribute_mapping.py:16:46
Ruff marks it auto-fixable, and the ruff hook in .pre-commit-config.yaml runs with --fix. The next commit that touches this file deletes the line silently. Five test modules still import the constant from here and would then fail at collection with ImportError:
tests/test_tracing.py:9tests/test_span_converter.py:16tests/test_otel_native_paths.py:17tests/test_logger_otel_egress.py:10tests/test_http_instrumentation.py:35
Suggested fix: point those five imports at splunk_ao.simulation_run_context and delete this line. The old path only exists on agent-simulation-poc and was never released, so there are no outside callers to keep working. If you would rather keep the alias, make the re-export explicit so Ruff accepts it:
| from splunk_ao.simulation_run_context import SPLUNK_AO_SIMULATION_RUN_ID | |
| from splunk_ao.simulation_run_context import SPLUNK_AO_SIMULATION_RUN_ID as SPLUNK_AO_SIMULATION_RUN_ID |
| def test_simulation_run_id_is_normalized_at_public_context_boundaries(monkeypatch: pytest.MonkeyPatch) -> None: | ||
| monkeypatch.setattr(splunk_ao_context, "get_logger_instance", Mock()) | ||
|
|
||
| with splunk_ao_context.simulation_run(" run-1\n"): | ||
| assert _simulation_run_id_context.get() == "run-1" | ||
|
|
||
| with splunk_ao_context(simulation_run_id="\trun-2 "): | ||
| assert _simulation_run_id_context.get() == "run-2" |
There was a problem hiding this comment.
nit: This covers trimming at the public context entry points, but only checks the context variable. SpanConverter.convert_span trims separately with validate_simulation_run_id (span_converter.py:96), and tests/test_span_converter.py only tests IDs that get rejected ("", " ", 123) and an already-clean "run-1". Could you add one converter case showing that simulation_run_id=" run-1 " is exported as splunk_ao.simulation.run_id == "run-1"? Without it, nothing tests the claim that trimming keeps span grouping consistent downstream.
This branch has not been deployed
2 participants
Jira: SAO-18670
SPLUNK_AO_SIMULATION_RUN_IDinto the dependency-light simulation_run_context.py module and updated the source imports to remove the converter import cycle.