feat(splunk-ao-adk): rename LogStream → AgentStream env var (HYBIM-914) - #141
Conversation
fercor-cisco
left a comment
There was a problem hiding this comment.
🤖 This review was generated by the Astra agent (claude-opus-4-8). It may contain mistakes.
Verdict: needs_discussion — Rename is complete and correct, but the ADK package hard-cuts the old env var while the core SDK it depends on still honors it as a deprecated alias — an inconsistency the ticket explicitly flags as an open decision.
General Comments
- 🟠 major (design): This PR hard-cuts
SPLUNK_AO_LOG_STREAMin the ADK package (onlySPLUNK_AO_AGENT_STREAMis read/validated), but the coresplunk-aoSDK this package depends on keepsSPLUNK_AO_LOG_STREAMas an explicit deprecated alias — seesrc/splunk_ao/config.py:58(("SPLUNK_AO_LOG_STREAM", "GALILEO_LOG_STREAM") # deprecated alias) andsrc/splunk_ao/utils/env_helpers.py:74/112/125(getenv("SPLUNK_AO_AGENT_STREAM") or getenv("SPLUNK_AO_LOG_STREAM")). The result is inconsistent behavior for a user who has only the old var set: the ADK__init__validation raisesValueError, even though the underlying SDK resolution path would have accepted it. HYBIM-914 explicitly leaves this as a decision ("whether to hard-cut ... or provide a deprecation warning" / "a backward compatibility alias should be considered"). Please confirm the intent — either match the SDK's backward-compat alias here, or document/accept that the ADK integration is a hard cut that diverges from the SDK. This should be resolved before merge to avoid confusing upgrade behavior.
| "coverage>=7.9.2", | ||
| "splunk-ao>=0.1.0,<1.0.0", | ||
| "galileo-core[testing]>=3.82.0", | ||
| "opentelemetry-resourcedetector-gcp==1.12.0a0", |
There was a problem hiding this comment.
🟡 minor (other): Pinning a pre-release (==1.12.0a0) as a workaround for the yanked 1.13.0 is reasonable to unblock uv sync, but an exact alpha pin will silently rot: once a non-yanked stable release satisfying google-adk lands, this pin will hold the resolver back and no one will notice. Consider adding a # TODO/tracking-issue reference next to it so it gets revisited, and confirm 1.12.0a0 actually satisfies google-adk's constraint on this dependency (the PR body says 1.13.0 was the stable version that satisfied it).
🤖 Generated by the Astra agent
There was a problem hiding this comment.
Dug into this and tested resolution directly (uv 0.11.17, dry-run uv lock against PyPI). Findings, with a correction to my own earlier reasoning:
The exact pin is no longer necessary. With the line removed entirely, resolution succeeds and uv auto-selects 1.12.0a0:
| Scenario | Result |
|---|---|
| pin removed | ✅ Resolved 147 packages — uv picks 1.12.0a0 on its own |
==1.12.0a0 |
✅ resolves |
Why: uv treats yanked 1.13.0 as not available, so for opentelemetry-resourcedetector-gcp every available version is a pre-release → uv's default if-necessary prerelease strategy kicks in and allows the alpha. So the yank actually enables the fallback. The pin was almost certainly a real fix when it was added (before 1.13.0 was yanked, a stable release existed, if-necessary didn't trigger, and uv sync failed) — but that condition is now gone.
Suggested change — replace the exact pin with an explicit-marker range:
# google-adk needs this GCP detector, whose only stable release (1.13.0) is
# yanked; the a0 marker opts into the pre-release line. Drop when a non-yanked
# stable satisfies google-adk.
"opentelemetry-resourcedetector-gcp>=1.12.0a0",The a0 marker opts just this package into pre-releases regardless of yank state (so it survives 1.13.0 being un-yanked), and the range lets uv move up to any future non-yanked stable automatically — no rot, no TODO needed. Simply deleting the line also works today but silently depends on the yank staying in place.
Second point confirmed: 1.12.0a0 satisfies the constraint — uv tree --invert shows google-adk v1.36.2 pulling it in transitively (directly and via google-cloud-spanner, the GCP OTel exporters, etc.).
Multi-Model Review: PR #141 — feat(splunk-ao-adk): rename LogStream → AgentStream env var
Overall Verdict: ✅ Approved with suggestionsThe rename is mechanically complete (all 18 occurrences covered). The Two minor items remain: 🟡 MEDIUM — Add a test locking in the intentional hard-cutover behaviorFiles: Since the hard cutover is a deliberate design choice (confirmed by @fercor-cisco), it should be locked in with an explicit test. Without it, a future contributor could accidentally add backward compat and there would be no regression guard — and no documentation of the intent. Suggested test for both def test_old_env_var_not_recognized(self, monkeypatch):
"""SPLUNK_AO_LOG_STREAM (old name) is intentionally not supported; SPLUNK_AO_AGENT_STREAM required."""
monkeypatch.setenv("SPLUNK_AO_LOG_STREAM", "some-stream")
monkeypatch.delenv("SPLUNK_AO_AGENT_STREAM", raising=False)
with pytest.raises(ValueError, match="SPLUNK_AO_AGENT_STREAM"):
SplunkAOADKCallback(project="my-project")⚪ NIT — Test isolation: clean up
|
| Finding | Verdict |
|---|---|
Hard cutover breaks old SPLUNK_AO_LOG_STREAM users |
Resolved — fercor-cisco confirmed hard cutover is intentional; no backward compat needed |
opentelemetry-resourcedetector-gcp==1.12.0a0 alpha pin |
Not an issue — 1.13.0 yanked ("breaks imports"); alpha is the only viable version |
| Core SDK still reads old name (Codex) | False positive — origin/main has full backward compat; Codex browsed local branch |
| "Split-read bug" (Cursor) | False positive — SplunkAOObserver re-resolves via env_helpers which reads new name correctly |
| Stale docstrings (Cursor) | False positive — diff covers all 6 occurrences across both files |
Alpha pin malformed in pyproject.toml (Cursor) |
False positive — correctly placed inside [dependency-groups] dev |
|
We do not need to support fallback or backward compatibility with |
Summary
pre-releases and require explicit opt-in
Test plan