Skip to content

SAO-17698: Address review comments #271

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.

Sign up for GitHub

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

Open
adityamehra wants to merge 2 commits into
base: agent-simulation-poc
Choose a base branch
from
Open

SAO-17698: Address review comments #271

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2525211
Address review comments
adityamehra Oct 12, 2026
ee20cc3
test(telemetry): mock context logger lookup
adityamehra Oct 12, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/splunk_ao/converter/attribute_mapping.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,10 @@
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

@fercor-cisco fercor-cisco Oct 12, 2026

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.

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:9
  • tests/test_span_converter.py:16
  • tests/test_otel_native_paths.py:17
  • tests/test_logger_otel_egress.py:10
  • tests/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:

Suggested change
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


SPLUNK_AO_SYSTEM = "splunk_ao.system"
SPLUNK_AO_SYSTEM_VALUE = "splunk_ao_python"
SPLUNK_AO_SIMULATION_RUN_ID = "splunk_ao.simulation.run_id"

CONTENT_ALIAS_BY_GEN_AI: Mapping[str, str] = {
"gen_ai.input.messages": "splunk_ao.input.messages",
Expand Down
4 changes: 2 additions & 2 deletions src/splunk_ao/converter/span_converter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@
from opentelemetry.trace import SpanContext, SpanKind, Status, StatusCode

from galileo_core.schemas.logging.step import BaseStep, StepType
from splunk_ao.converter.attribute_mapping import SPLUNK_AO_SIMULATION_RUN_ID, build_span_attributes
from splunk_ao.simulation_run_context import validate_simulation_run_id
from splunk_ao.converter.attribute_mapping import build_span_attributes
from splunk_ao.simulation_run_context import SPLUNK_AO_SIMULATION_RUN_ID, validate_simulation_run_id
from splunk_ao.utils.headers_data import get_package_version

_EPOCH = datetime(1970, 1, 1, tzinfo=UTC)
Expand Down
3 changes: 1 addition & 2 deletions src/splunk_ao/otel.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
from galileo_core.schemas.logging.span import Span as SplunkAOSpan
from splunk_ao.config import SplunkAOConfig
from splunk_ao.converter import build_span_attributes
from splunk_ao.converter.attribute_mapping import SPLUNK_AO_SIMULATION_RUN_ID
from splunk_ao.decorator import (
_agent_stream_context,
_dataset_input_context,
Expand All @@ -36,7 +35,7 @@
)
from splunk_ao.exporter.diagnostics import get_export_health
from splunk_ao.session_context import get_effective_session_id
from splunk_ao.simulation_run_context import get_simulation_run_id
from splunk_ao.simulation_run_context import SPLUNK_AO_SIMULATION_RUN_ID, get_simulation_run_id

logger = logging.getLogger(__name__)

Expand Down
7 changes: 5 additions & 2 deletions src/splunk_ao/session_context.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,11 @@
from opentelemetry.context import Context
from opentelemetry.propagators import textmap

from splunk_ao.converter.attribute_mapping import SPLUNK_AO_SIMULATION_RUN_ID
from splunk_ao.simulation_run_context import get_baggage_simulation_run_id, get_simulation_run_id
from splunk_ao.simulation_run_context import (
SPLUNK_AO_SIMULATION_RUN_ID,
get_baggage_simulation_run_id,
get_simulation_run_id,
)

GEN_AI_CONVERSATION_ID = "gen_ai.conversation.id"

Expand Down
6 changes: 3 additions & 3 deletions src/splunk_ao/simulation_run_context.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,16 +5,16 @@
from opentelemetry import baggage
from opentelemetry.context import Context

from splunk_ao.converter.attribute_mapping import SPLUNK_AO_SIMULATION_RUN_ID
SPLUNK_AO_SIMULATION_RUN_ID = "splunk_ao.simulation.run_id"

_simulation_run_id_context: ContextVar[str | None] = ContextVar("simulation_run_id_context", default=None)


def validate_simulation_run_id(simulation_run_id: object) -> str:
"""Validate a public simulation run ID and return it unchanged."""
"""Validate a public simulation run ID and return its normalized value."""
if not isinstance(simulation_run_id, str) or not simulation_run_id.strip():
raise ValueError("simulation_run_id must be a non-empty string")
return simulation_run_id
return simulation_run_id.strip()


def get_baggage_simulation_run_id(context: Context | None = None) -> str | None:
Expand Down
10 changes: 10 additions & 0 deletions tests/test_splunk_ao_context.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,16 @@ def test_simulation_run_scope_restores_the_previous_value() -> None:
_simulation_run_id_context.reset(outer_token)


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"
Comment on lines +87 to +94

@fercor-cisco fercor-cisco Oct 12, 2026

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.

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.



def test_init_clears_active_simulation_run(monkeypatch: pytest.MonkeyPatch) -> None:
logger = Mock()
singleton = Mock()
Expand Down
Loading