Skip to content

feat(env-vars): rename SPLUNK_AO_LOG_STREAM → SPLUNK_AO_AGENT_STREAM (HYBIM-914)#120

Merged
shuningc merged 9 commits into
mainfrom
HYBIM-914-logstream-rename
Jul 24, 2026
Merged

feat(env-vars): rename SPLUNK_AO_LOG_STREAM → SPLUNK_AO_AGENT_STREAM (HYBIM-914)#120
shuningc merged 9 commits into
mainfrom
HYBIM-914-logstream-rename

Conversation

@shuningc

@shuningc shuningc commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Renames LogStreamAgentStream and LogStreamsAgentStreams throughout
    the splunk-ao core package to align with the new naming convention
  • Updates all references in src/splunk_ao/ including agent_streams.py,
    agent_control.py, config.py, configuration.py, decorator.py,
    logger/logger.py, middleware/tracing.py, utils/env_helpers.py,
    and utils/singleton.py
  • Fixes failing tests that still patched splunk_ao.logger.logger.LogStreams
    — updated to AgentStreams in test_logger_batch.py and test_openai_agents.py

Test plan

  • All existing tests pass (1995 previously passing, 2 fixed)
  • splunk-ao-adk and splunk-ao-a2a rename will be handled in a separate PR

@fercor-cisco fercor-cisco left a comment

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.

🤖 This review was generated by the Astra agent (claude-opus-4-8). It may contain mistakes.

Verdict: request_changes — Backward-compat for the old env vars is applied inconsistently across read paths, so old vars work on some code paths and silently fail on others.

General Comments

  • 🟠 major (design): This PR makes an inconsistent decision about backward compatibility for the old env vars. env_helpers.py was changed to fall back to the deprecated SPLUNK_AO_LOG_STREAM / SPLUNK_AO_LOG_STREAM_ID (silent alias), but three other read paths do a hard cut and no longer honor the old names at all:

  • config.py _BRIDGE (lines 57-58) maps only SPLUNK_AO_AGENT_STREAM*GALILEO_LOG_STREAM*, so galileo-core-backed paths won't see the old var.

  • configuration.py _CONFIGURATION_KEYS (lines 94-95): Configuration.default_logstream_name/id now read only the new var.

  • agent_control.py (line 97): get_agent_control_target() reads only SPLUNK_AO_AGENT_STREAM_ID.

Net effect for a user who still has only SPLUNK_AO_LOG_STREAM_ID set: logging via the singleton/otel/logger path keeps working (env_helpers falls back), but Agent Control resolution silently falls through to AgentControlTargetUnresolvedError, and Configuration/the galileo bridge ignore it. Partial backward-compat that varies by code path is harder to reason about than either a clean hard cut or a uniform alias. The ticket (HYBIM-914) explicitly left this as a 'decision needed' (hard-cut vs. deprecation alias) — please pick one and apply it consistently across all read paths. If aliasing, also emit a one-time DeprecationWarning so users know to migrate; right now the fallback is completely silent.

Follow-ups

Suggested follow-up work that could be tracked as Shortcut stories:

  • README.md:82-129: The ticket's acceptance criteria require updating all .env.example, README, and tutorial files to the new variable names. The root README.md still documents SPLUNK_AO_LOG_STREAM / SPLUNK_AO_LOG_STREAM_ID (lines 82, 98, 129, 232), and ~80 files under examples/ still reference the old names. These still work via the env_helpers alias but now advertise a deprecated var as canonical. The PR body scopes docs/adk/a2a to a separate PR, so tracking as a follow-up rather than a blocker.

Comment thread src/splunk_ao/agent_control.py Outdated
)

env_log_stream_id = _strip_optional_string(os.getenv("SPLUNK_AO_LOG_STREAM_ID"))
env_log_stream_id = _strip_optional_string(os.getenv("SPLUNK_AO_AGENT_STREAM_ID"))

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.

🟠 major (bug): Inconsistent with the backward-compat aliases added in env_helpers.py. A user who upgraded but still has only SPLUNK_AO_LOG_STREAM_ID set will have Agent Control resolution silently fail here (falling through to AgentControlTargetUnresolvedError at line 112), even though their logging keeps working via the aliased _get_log_stream_or_default. Either add the same fallback here or drop the aliases in env_helpers.py for a clean hard cut.

Suggested change
env_log_stream_id = _strip_optional_string(os.getenv("SPLUNK_AO_AGENT_STREAM_ID"))
env_log_stream_id = _strip_optional_string(
os.getenv("SPLUNK_AO_AGENT_STREAM_ID") or os.getenv("SPLUNK_AO_LOG_STREAM_ID")
)

🤖 Generated by the Astra agent

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.

Adding old env vars

Comment thread src/splunk_ao/config.py
Comment on lines +57 to +58
("SPLUNK_AO_AGENT_STREAM", "GALILEO_LOG_STREAM"),
("SPLUNK_AO_AGENT_STREAM_ID", "GALILEO_LOG_STREAM_ID"),

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.

🟠 major (bug): The bridge only propagates the new SPLUNK_AO_AGENT_STREAM* vars to GALILEO_*. Combined with the silent alias added in env_helpers.py, a user with only the old SPLUNK_AO_LOG_STREAM* set will have the value honored on the env_helpers path but NOT bridged to galileo-core here. If backward compat is intended, the old vars need to be bridged too (or map GALILEO_LOG_STREAM from whichever of the two is present).

🤖 Generated by the Astra agent

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.

Adding old env vars

Comment on lines +94 to +95
ConfigKey(name="default_logstream_name", env_var="SPLUNK_AO_AGENT_STREAM", description="Default agent stream name"),
ConfigKey(name="default_logstream_id", env_var="SPLUNK_AO_AGENT_STREAM_ID", description="Default agent stream ID"),

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.

🟡 minor (design): Configuration.default_logstream_name / default_logstream_id now resolve only from the new env vars. This diverges from the aliased fallback in env_helpers.py, so the old vars are honored on one path but ignored here. Align with whichever backward-compat approach is chosen for the PR.

🤖 Generated by the Astra agent

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.

Adding the old env vars back

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.

src/splunk_ao/agent_streams.py:571-571 (line not in diff)

🟡 minor (documentation): Stale user-facing message: still references the old SPLUNK_AO_LOG_STREAM env var. Since this is the canonical name being renamed, update it to SPLUNK_AO_AGENT_STREAM to match the rest of the rename.

Suggested change
raise ValueError("agent_stream_name must be provided (or set SPLUNK_AO_AGENT_STREAM env var)")

🤖 Generated by the Astra agent

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.

Modified

@fercor-cisco fercor-cisco left a comment

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.

🤖 This review was generated by the Astra agent (claude-opus-4-8). It may contain mistakes.

Verdict: approve — Clean mechanical rename; prior major backward-compat concerns addressed via aliases. Two minor gaps remain.

Follow-ups

Suggested follow-up work that could be tracked as Shortcut stories:

  • tests/test_config.py:49-53: The deprecated-alias bridging (SPLUNK_AO_LOG_STREAM / SPLUNK_AO_LOG_STREAM_ID → GALILEO_LOG_STREAM*) is excluded from both parametrized bridge tests via _CANONICAL_BRIDGE_PAIRS, so the backward-compat path added in this PR has zero test coverage. Since backward compat was the explicit reviewer concern, add a test that sets only the deprecated var and asserts it (a) bridges to the GALILEO_* target and (b) is honored by _get_log_stream_or_default / _get_log_stream_from_env / _get_log_stream_id_from_env and get_agent_control_target, including that the new var takes precedence when both are set.

Comment on lines +94 to +95
ConfigKey(name="default_logstream_name", env_var="SPLUNK_AO_AGENT_STREAM", description="Default agent stream name"),
ConfigKey(name="default_logstream_id", env_var="SPLUNK_AO_AGENT_STREAM_ID", description="Default agent stream ID"),

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.

🟡 minor (design): These two ConfigKeys now resolve only from the new SPLUNK_AO_AGENT_STREAM/SPLUNK_AO_AGENT_STREAM_ID vars, with no fallback to the deprecated SPLUNK_AO_LOG_STREAM* vars. This diverges from the backward-compat aliases you added in env_helpers.py, config.py, and agent_control.py: a user who upgraded but still has only SPLUNK_AO_LOG_STREAM set will have the value honored on the resolution paths but show up as "Not set" via Configuration.default_logstream_name / get_configuration().

Note you replied "Adding the old env vars back" to the earlier comment on these lines, but the diff shows no fallback was added here. Impact is low because default_logstream_name/default_logstream_id aren't consumed by the actual log-stream resolution logic (they're only defined in this file), but for consistency and to match your stated intent, consider honoring the deprecated var here too. The ConfigKey/metaclass currently supports a single env_var per key, so this needs either a small metaclass change (e.g. an optional fallback_env_var) or dropping the aliases entirely for a clean hard cut.

🤖 Generated by the Astra agent

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.

There's no need for back-compatibility with SPLUNK_AO_LOG_STREAM, that was never released. It was just a temporary state in the process of renaming.

@shuningc
shuningc merged commit 2deac83 into main Jul 24, 2026
17 checks passed
@shuningc
shuningc deleted the HYBIM-914-logstream-rename branch July 24, 2026 00:18
@github-actions github-actions Bot locked and limited conversation to collaborators Jul 24, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants