Skip to content

fix(config): HYBIM-960 derive standalone API URLs from custom console hosts - #212

Open
pradystar wants to merge 2 commits into
mainfrom
fix/HYBIM-960-standalone-api-url
Open

fix(config): HYBIM-960 derive standalone API URLs from custom console hosts#212
pradystar wants to merge 2 commits into
mainfrom
fix/HYBIM-960-standalone-api-url

Conversation

@pradystar

@pradystar pradystar commented Aug 6, 2026

Copy link
Copy Markdown
Collaborator

Summary

Correct standalone API URL derivation so CRUD operations and OTLP trace export use the same API host for custom console domains.

What changed

  • Added a shared standalone API URL resolver.
  • Preserved explicit SPLUNK_AO_API_URL overrides.
  • Continued mapping console. and app. hosts to api. and preserving existing api. hosts.
  • Added an api. prefix for custom console hosts without a recognized prefix.
  • Kept localhost development behavior in galileo-core which always resolves to http://localhost:8088 and the /otel/v1/traces endpoint unchanged, .
  • Updated standalone configuration documentation and the changelog.
  • Left O11y endpoint derivation, authentication, and routing unchanged.

Testing

  • Focused configuration/deployment/exporter tests: 135 passed.
  • Full SDK suite: 2,143 passed, 4 skipped.
  • Ruff, formatting, mypy over 116 source files, poetry check --lock, and git diff --check passed.

Compatibility / risk

This changes fallback behavior for standalone custom domains: a console host such as customer.example.com now derives api.customer.example.com. Deployments whose API does not follow this convention must set SPLUNK_AO_API_URL explicitly. O11y behavior is unchanged.

@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-5). It may contain mistakes.

Verdict: request_changes — The new resolver drops the ticket's explicit "no scheme" branch and now raises an unpacking ValueError for scheme-less console URLs (reachable via the unvalidated StandaloneConfig.console_url), and info.data["console_url"] replaces a graceful lookup with a KeyError when console URL validation fails.

General Comments

  • 🟠 major (bug): The Jira ticket's proposed logic has four branches, one of which is explicitly for console URLs with no scheme:
elif "://" in console_url:
    api_url = console_url.replace("://", "://api.", 1)
else:
    api_url = f"api.{console_url}"   # <-- this branch

The implementation dropped that last branch and instead does an unconditional base_url.split("://", 1) unpack into two names. That is not a no-op omission — it turns a previously-degraded-but-working input into a hard ValueError. See the line comment on deployment.py:59 for the concrete failure path.

This matters because the two callers of resolve_standalone_api_url give very different guarantees about the input:

  • SplunkAOConfig.set_api_url passes str(info.data["console_url"]), where console_url is a pydantic Url and galileo-core's ensure_https_console_url has already prepended https://. A scheme is guaranteed.
  • StandaloneConfig.console_url is a plain str dataclass field, and StandaloneConfig.from_env() assigns os.environ["SPLUNK_AO_CONSOLE_URL"] verbatim with no normalization at all.

So the shared helper is only safe on one of its two call sites. Since the stated goal of the PR is to make CRUD and OTLP agree, the helper should be the place that normalizes, not assume a caller already did.

Follow-ups

Suggested follow-up work that could be tracked as Jira tickets:

  • examples/agent/langgraph-otel/main.py:67-69: This example hand-rolls the old derivation (console_url.replace("://console.", "://api.").replace("://app.", "://api.")) and now diverges from the SDK: it will not add the new api. prefix for a custom console host, so an example user with SPLUNK_AO_CONSOLE_URL=https://customer.example.com posts spans to https://customer.example.com/otel/v1/traces while the SDK itself targets https://api.customer.example.com/.... Consider importing resolve_standalone_api_url (or StandaloneConfig.otlp_endpoint) here so the example cannot drift from the resolver again.
  • src/splunk_ao/deployment.py:144-145: StandaloneConfig.console_url is an unvalidated str while SplunkAOConfig.console_url is a pydantic Url that gets a scheme prepended by ensure_https_console_url. That asymmetry is the root cause of the scheme-less crash flagged in this review, and it will keep producing this class of bug for any future consumer of the raw string. Consider normalizing in __post_init__ (prepend https:// when no scheme is present, strip the trailing slash) so both deployment configs offer the same guarantees to shared helpers.
  • src/splunk_ao/deployment.py:57-63: The resolver does string surgery on the whole post-scheme remainder, so a console URL carrying a path is handled oddly: https://example.com/console yields host+path example.com/console, the startswith("api.") guard tests against that combined string, and the result is https://api.example.com/console. Rewriting this on top of urllib.parse.urlsplit/urlunsplit would confine the prefix logic to netloc, preserve port and path correctly, and remove the need for manual :// handling entirely.

Comment on lines +57 to +63
base_url = console_url.rstrip("/")
base_url = base_url.replace("://console.", "://api.", 1).replace("://app.", "://api.", 1)
scheme, host = base_url.split("://", 1)
if not host.startswith("api."):
host = f"api.{host}"

return f"{scheme}://{host}"

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): base_url.split("://", 1) unpacks into exactly two names, so any console_url without a scheme raises ValueError: not enough values to unpack (expected 2, got 1).

This is reachable, and it is a regression rather than a pre-existing wart:

  • StandaloneConfig.console_url is an unvalidated str, and from_env() copies SPLUNK_AO_CONSOLE_URL straight through (deployment.py:165). Nothing prepends a scheme on this path.
  • With SPLUNK_AO_CONSOLE_URL=customer.example.com, StandaloneConfig.from_env().otlp_endpoint now raises. That property is called during SplunkAOLogger.__init__ (logger/logger.py:394) and SplunkAOOTLPExporter.__init__ (otel.py:139), so logger construction dies with an opaque unpacking error instead of exporting anything.
  • The previous implementation was self.console_url.replace("://console.", "://api.", 1).replace("://app.", "://api.", 1), which left a scheme-less host untouched and produced customer.example.com/otel/v1/traces — wrong, but not an exception.

A scheme-less console URL is a realistic user input: several example .env files in this repo prompt for SPLUNK_AO_CONSOLE_URL=your-splunk-ao-console-url with no scheme, and the ticket's own pseudocode has a dedicated branch for it.

str.partition keeps this a one-line fix and defaults to https, matching what galileo-core's ensure_https_console_url does on the config path:

Suggested change
base_url = console_url.rstrip("/")
base_url = base_url.replace("://console.", "://api.", 1).replace("://app.", "://api.", 1)
scheme, host = base_url.split("://", 1)
if not host.startswith("api."):
host = f"api.{host}"
return f"{scheme}://{host}"
base_url = console_url.rstrip("/")
base_url = base_url.replace("://console.", "://api.", 1).replace("://app.", "://api.", 1)
scheme, separator, host = base_url.partition("://")
if not separator:
scheme, host = "https", base_url
if not host.startswith("api."):
host = f"api.{host}"
return f"{scheme}://{host}"

🤖 Generated by the Astra agent

Comment thread src/splunk_ao/config.py Outdated
Comment on lines +109 to +112
console_url_value = str(info.data["console_url"])
api_url_value = str(api_url) if api_url is not None else None
resolved_api_url = resolve_standalone_api_url(console_url_value, api_url_value)
return super().set_api_url(resolved_api_url, info)

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): info.data["console_url"] raises KeyError when console_url fails its own validation. In pydantic v2, info.data contains only the fields validated successfully so far, and per-field validators for later fields still run — so a bad console URL means this subscript throws. A KeyError is not a ValueError/AssertionError, so pydantic does not collect it as a validation error; it propagates out of model construction and replaces the real diagnostic. The user typo'd their console URL and gets KeyError: 'console_url' with no mention of the actual problem.

The base implementation this replaced deliberately avoided that (base_config.py:185-190):

console_url = info.data.get("console_url")
if console_url is None:
    raise ValueError(
        "Console URL is required. Please set the environment variable "
        "`GALILEO_CONSOLE_URL` to your Galileo console URL."
    )

Reachable via SplunkAOConfig.get(console_url="not a url") or SPLUNK_AO_CONSOLE_URL="not a url"ensure_https_console_url prepends https://, the embedded space still fails Url validation, and this validator then runs without console_url in info.data.

Falling back to super() when console_url is absent keeps the base class's clear message and its Url-vs-str handling, and costs nothing on the happy path:

Suggested change
console_url_value = str(info.data["console_url"])
api_url_value = str(api_url) if api_url is not None else None
resolved_api_url = resolve_standalone_api_url(console_url_value, api_url_value)
return super().set_api_url(resolved_api_url, info)
console_url = info.data.get("console_url")
if console_url is None:
return super().set_api_url(api_url, info)
api_url_value = str(api_url) if api_url is not None else None
resolved_api_url = resolve_standalone_api_url(str(console_url), api_url_value)
return super().set_api_url(resolved_api_url, info)

🤖 Generated by the Astra agent

Comment on lines +54 to +55
if "localhost" in console_url or "127.0.0.1" in console_url:
return "http://localhost:8088"

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 (bug): The PR description says "Kept localhost development behavior and the /otel/v1/traces endpoint unchanged," but the OTLP endpoint for local dev did change. otlp_endpoint previously had no localhost branch at all — it only did the console./app. replacements — so the console port was carried through:

console_url otlp_endpoint before after
http://localhost:8088 http://localhost:8088/otel/v1/traces unchanged
http://localhost:3000 http://localhost:3000/otel/v1/traces http://localhost:8088/otel/v1/traces

Routing to the API port rather than the console port is very likely the intended outcome, so I'm not asking you to revert it — but it should be stated in the description and the CHANGELOG rather than described as unchanged, and it deserves a test (see the tests/test_deployment.py comment). The hardcoded 8088 also silently discards a non-default local API port; anyone running the API elsewhere locally must now set SPLUNK_AO_API_URL, which is worth the same README note the custom-domain fallback got.

Separately, "localhost" in console_url is a substring test, so https://localhost.customer.example.com also collapses to http://localhost:8088. That over-broad match is inherited verbatim from galileo-core's set_api_url, so it is pre-existing on the CRUD path — but this change newly extends it to OTLP export.

🤖 Generated by the Astra agent

Comment thread tests/test_deployment.py

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 (testing): The new test_resolve_standalone_api_url parametrization covers six happy-path shapes but none of the inputs that changed behavior in a risky way:

  1. No scheme ("customer.example.com") — currently raises ValueError. The ticket called this case out explicitly; a parametrized case here would have caught it.
  2. Explicit api_url argument — the if api_url: return api_url short-circuit is only exercised indirectly through SplunkAOConfig in test_config.py and through StandaloneConfig.otlp_endpoint; the resolver's own override behavior is untested.
  3. otlp_endpoint for a localhost console on a non-8088 port — this is the one case whose output actually changed (see the deployment.py:54 comment), and nothing asserts the new value.

Cases 1 and 3 are both inside the problem this PR set out to solve, so they belong in this PR rather than a follow-up.

🤖 Generated by the Astra agent

@fercor-cisco

Copy link
Copy Markdown
Collaborator

Regressions in this PR

The core fix is right: customer.example.com previously derived its own host as the API host on the CRUD path (galileo-core's replace("console", "api") is a no-op when there's no console substring), and both paths now agree on api.customer.example.com. One regression alongside it, plus two things that should be documented or reworded.

1. src/splunk_ao/config.py:109KeyError replaces the real validation error (major)

console_url_value = str(info.data["console_url"])

In pydantic v2 info.data holds only fields that validated successfully, and later per-field validators still run. When console_url fails its own validation the key is absent, and KeyError is not a ValueError/AssertionError, so pydantic doesn't collect it — it propagates out of model construction and replaces the real diagnostic.

SplunkAOConfig(api_key="k", console_url="http://[bad"):

  • before: ValidationError with 2 errors, including console_url: Input should be a valid URL, invalid IPv6 address
  • after: KeyError: 'console_url'

The user typo'd their console URL and gets no mention of it. The base implementation avoids this deliberately (base_config.py:185-190) by using .get() and raising a ValueError naming the missing variable — worth mirroring, since a ValueError gets collected alongside the console_url error instead of masking it.

2. src/splunk_ao/deployment.py:54-55 — localhost OTLP port change is undocumented (minor)

otlp_endpoint had no localhost branch before; it only did the prefix replacements, so the console port carried through. Sharing the resolver introduces one:

SPLUNK_AO_CONSOLE_URL otlp_endpoint before after
http://localhost:8088 http://localhost:8088/otel/v1/traces unchanged
http://localhost:3000 http://localhost:3000/otel/v1/traces http://localhost:8088/otel/v1/traces

Routing to the API port rather than the console port is very likely intended, so I'm not asking for a revert — but the description states localhost behavior is "unchanged," and nothing tests the one output that actually moved.

3. Schemeless URLs — not a regression, but the error should be explicit

Schemeless SPLUNK_AO_CONSOLE_URL never worked. StandaloneConfig.console_url is a raw str copied straight from the env (deployment.py:144,165) with nothing normalizing it — unlike the CRUD path, where galileo-core's ensure_https_console_url prepends https:// before the resolver runs. With SPLUNK_AO_CONSOLE_URL=customer.example.com:

  • before: constructed fine, then requests.MissingSchema: Invalid URL 'customer.example.com/otel/v1/traces': No scheme supplied at the first export
  • after: ValueError: not enough values to unpack (expected 2, got 1) from split("://", 1) at line 59, during SplunkAOLogger.__init__ (logger/logger.py:394) / SplunkAOOTLPExporter.__init__ (otel.py:139)

No working behavior is lost, so this isn't a regression — but the new failure names neither the variable nor the problem. Since the ticket called this case out, reject it explicitly in StandaloneConfig.__post_init__ with a message naming SPLUNK_AO_CONSOLE_URL and showing the expected form. SPLUNK_AO_API_URL deserves the same check: an explicit value short-circuits the resolver at line 51 and reaches the exporter unvalidated.

Test gaps

test_resolve_standalone_api_url covers six happy-path shapes but none of the inputs whose behavior changed: schemeless console_url (3), the explicit-api_url short-circuit at lines 51-52, and otlp_endpoint for a localhost console on a non-8088 port (2).

@pradystar

Copy link
Copy Markdown
Collaborator Author

1. src/splunk_ao/config.py:109KeyError replaces the real validation error (major)

console_url_value = str(info.data["console_url"])

In pydantic v2 info.data holds only fields that validated successfully, and later per-field validators still run. When console_url fails its own validation the key is absent, and KeyError is not a ValueError/AssertionError, so pydantic doesn't collect it — it propagates out of model construction and replaces the real diagnostic.

SplunkAOConfig(api_key="k", console_url="http://[bad"):

  • before: ValidationError with 2 errors, including console_url: Input should be a valid URL, invalid IPv6 address
  • after: KeyError: 'console_url'
  1. Fixed. The validator now reads console_url with info.data.get("console_url"). If the field is absent because its validation failed, it delegates to super().set_api_url(api_url, info), preserving the base validator’s ValueError behavior instead of leaking a KeyError. I also added a regression test confirming that an invalid console URL such as http://[bad produces a Pydantic ValidationError identifying console_url.
  2. Right that localhost OTLP behavior changes; the PR description’s “unchanged” statement was inaccurate. This is an intentional bug fix. The OTLP endpoint should always be based on the standalone API URL: {api_url}/otel/v1/traces. Previously, the OTLP path independently re-read the raw console URL, so http://localhost:3000 incorrectly produced http://localhost:3000/otel/v1/traces, even though the standalone API URL resolves to http://localhost:8088`. The shared resolver corrects that inconsistency, so CRUD and OTLP now both use the proper API base. I corrected the PR description accordingly.
  3. Agreed that the opaque unpacking ValueError should not be introduced. I addressed this by normalizing scheme-less console URLs instead of rejecting them. The shared resolver now follows the existing CRUD behavior and prepends https:// before deriving the API hostname:
    customer.example.com
    → https://api.customer.example.com
    → https://api.customer.example.com/otel/v1/traces
    
    This avoids both the previous delayed MissingSchema failure and the new construction-time unpacking error without making standalone configuration stricter than the CRUD path. The scheme-less case is covered in the resolver’s parameterized test. Explicit SPLUNK_AO_API_URL remains an unchanged passthrough, as it was before this PR; broader validation of explicit overrides is outside the custom-console-host fix.

@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-5). It may contain mistakes.

Verdict: request_changes — The new unconditional api. prefix breaks previously-working on-prem console URLs that are IP literals or single-label hosts (e.g. http://10.0.0.5:8088, http://ao-prod:8088), and the behavior change is filed as a bug fix with no migration note.

General Comments

  • 🟡 minor (design): The resolver silently rewrites the API host, and when it guesses wrong the user's first symptom is an opaque failure far from the cause: for CRUD it's a healthcheck connection error inside GalileoConfig.set_api_url surfaced through Configuration.connect()'s generic "Connection failed: ..." wrapper; for OTLP it's a silent export failure in a background span sink.

Since the derived host is now a guess about the deployment's DNS layout rather than a mechanical console.api. substitution, consider emitting a debug/info log from resolve_standalone_api_url when the derived host differs from the console host, naming SPLUNK_AO_API_URL as the override. One line makes an otherwise invisible inference diagnosable:

if derived != console_host:
    logger.debug(
        f"Derived standalone API URL {derived} from console URL {console_url}. "
        "Set SPLUNK_AO_API_URL if your deployment does not follow this convention."
    )

Follow-ups

Suggested follow-up work that could be tracked as Jira tickets:

  • examples/agent/langgraph-otel/main.py:62-69: This example hand-rolls the old derivation (console_url.replace("://console.", "://api.").replace("://app.", "://api.")) and now disagrees with the SDK for custom console hosts: for SPLUNK_AO_CONSOLE_URL=https://customer.example.com the example posts to https://customer.example.com/otel/v1/traces while the SDK uses https://api.customer.example.com/otel/v1/traces. Not blocking (the example is standalone and doesn't import the SDK resolver), but it now teaches the wrong convention. Consider importing resolve_standalone_api_url, or deriving the endpoint from StandaloneConfig.from_env().otlp_endpoint, so the example cannot drift from the SDK again.
  • src/splunk_ao/deployment.py:49-65: resolve_standalone_api_url passes an explicit api_url through completely unvalidated (line 51-52), on both the OTLP path (StandaloneConfig.from_env() copies SPLUNK_AO_API_URL straight from the env, line 167) and the CRUD path. A schemeless SPLUNK_AO_API_URL=backend.example.com therefore reaches the exporter and fails at first export with requests.MissingSchema rather than at construction. This is unchanged by this PR and correctly out of scope for the custom-console-host fix, but now that the resolver is the single funnel for both paths it is the natural place to normalize/reject explicit overrides the same way console URLs are handled.

Comment on lines +57 to +65
base_url = console_url.rstrip("/")
if "://" not in base_url:
base_url = f"https://{base_url}"
base_url = base_url.replace("://console.", "://api.", 1).replace("://app.", "://api.", 1)
scheme, host = base_url.split("://", 1)
if not host.startswith("api."):
host = f"api.{host}"

return f"{scheme}://{host}"

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 api. prefix is applied to any host that isn't already api.-prefixed, including hosts where an api. subdomain cannot exist. This breaks deployments that worked before this PR on both paths.

Concrete failures:

SPLUNK_AO_CONSOLE_URL before (CRUD + OTLP) after
http://10.0.0.5:8088 http://10.0.0.5:8088 http://api.10.0.0.5:8088
http://ao-prod:8088 http://ao-prod:8088 http://api.ao-prod:8088

IP-literal and single-label (k8s service / hostname) console URLs are the normal shape for on-prem single-ingress installs — the repo's own fixtures and docs use exactly this layout (http://localhost:8088 in splunk-ao-a2a/README.md:76, http://fake.test:8088 in tests/conftest.py:26). Pre-PR, galileo-core's console_url.replace("console", "api") was a no-op on these, so api_url fell back to the console host and worked when console and API share a host+port; otlp_endpoint did the same. After this change both resolve to a hostname that does not resolve in DNS, and the only recovery is setting SPLUNK_AO_API_URL.

127.0.0.1 is already special-cased above, which shows IP-shaped input was contemplated — but 10.0.0.5, 192.168.x.x, and ::1 are not, and neither is any dotless host. Prefixing only makes sense for a dotted DNS name, so guard on that:

Note this also fixes the sibling case where the host part carries a port or path into the startswith("api.") check.

Suggested change
base_url = console_url.rstrip("/")
if "://" not in base_url:
base_url = f"https://{base_url}"
base_url = base_url.replace("://console.", "://api.", 1).replace("://app.", "://api.", 1)
scheme, host = base_url.split("://", 1)
if not host.startswith("api."):
host = f"api.{host}"
return f"{scheme}://{host}"
base_url = console_url.rstrip("/")
if "://" not in base_url:
base_url = f"https://{base_url}"
base_url = base_url.replace("://console.", "://api.", 1).replace("://app.", "://api.", 1)
scheme, host = base_url.split("://", 1)
hostname = host.partition("/")[0].partition(":")[0]
# Only a dotted DNS name can gain an `api.` label. IP literals and
# single-label hosts (k8s services, bare hostnames) must be left alone —
# `api.10.0.0.5` and `api.ao-prod` do not resolve.
is_ip_literal = ":" in hostname or (bool(hostname) and all(part.isdigit() for part in hostname.split(".")))
is_dotted_domain = "." in hostname and not is_ip_literal
if is_dotted_domain and not hostname.startswith("api."):
host = f"api.{host}"
return f"{scheme}://{host}"

🤖 Generated by the Astra agent

Comment thread CHANGELOG.md
Comment on lines +12 to +14
- Standalone custom console domains now derive a consistent `api.` hostname
for both CRUD operations and OTLP trace export unless `SPLUNK_AO_API_URL` is
set explicitly.

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 (documentation): This is filed under ### Fixed, but for a class of existing users it is a breaking change, not a fix. Any standalone deployment whose console host lacks a console./app. prefix previously had api_url fall back to the console host (galileo-core's replace("console", "api") was a no-op) and worked if console and API shared a host. Those deployments now point at api.<console-host> and stop working until SPLUNK_AO_API_URL is set.

The PR description does call this out under "Compatibility / risk", but the CHANGELOG is what users actually read on upgrade, and it currently reads as a pure improvement. Two other behavior changes are also unrecorded: otlp_endpoint for a localhost console on a non-8088 port now retargets to http://localhost:8088, and hosts containing console as a non-leading substring (customer-console.example.com) now derive api.customer-console.example.com instead of customer-api.example.com.

Please add a ### Changed entry with the migration step:

Suggested change
- Standalone custom console domains now derive a consistent `api.` hostname
for both CRUD operations and OTLP trace export unless `SPLUNK_AO_API_URL` is
set explicitly.
### Changed
- **Standalone API URL derivation.** Console hosts without a leading `console.`
or `app.` label now derive an `api.`-prefixed API hostname instead of reusing
the console host. Deployments that serve the console and API from the same
host, or whose API hostname does not follow this convention, must now set
`SPLUNK_AO_API_URL` explicitly. Two related changes: `otlp_endpoint` for a
localhost console now targets the API port (`http://localhost:8088`) rather
than the console port, and `console` is now matched only as a leading label,
so hosts such as `customer-console.example.com` derive
`api.customer-console.example.com`.
### Fixed
- Standalone custom console domains now derive a consistent `api.` hostname
for both CRUD operations and OTLP trace export unless `SPLUNK_AO_API_URL` is
set explicitly.

🤖 Generated by the Astra agent

Comment on lines +61 to +63
scheme, host = base_url.split("://", 1)
if not host.startswith("api."):
host = f"api.{host}"

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 (bug): host here is everything after ://, so any path on the console URL is carried into the derived API base. https://customer.example.com/ui yields https://api.customer.example.com/ui, and otlp_endpoint then produces https://api.customer.example.com/ui/otel/v1/traces.

The function's contract is to return an API base URL, so the console's path component should not survive. Pre-PR this was harmless (the host was unchanged, so the path was at least still valid for that origin); now both the host and the path are wrong. Deriving from the parsed hostname — as suggested in the comment on lines 57-65 — drops the path as a side effect. If you'd rather keep the string manipulation, strip the path explicitly:

host = host.partition("/")[0]

A parametrized case such as ("https://customer.example.com/ui", None, "https://api.customer.example.com") would pin this.

🤖 Generated by the Astra agent

Comment on lines +54 to +55
if "localhost" in console_url or "127.0.0.1" in console_url:
return "http://localhost:8088"

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 (bug): "localhost" in console_url is an unanchored substring test, so a legitimate public host containing that label collapses to http://localhost:8088 — e.g. https://localhost.customer.example.com sends both CRUD traffic and OTLP spans to the developer's own machine.

This over-broad match is inherited verbatim from galileo-core's set_api_url, so on the CRUD path it is pre-existing — but this PR newly extends it to OTLP export, which previously had no localhost branch at all. Since you're already centralizing the logic here, matching the host rather than the whole string closes it:

hostname = urlsplit(base_url if "://" in console_url else f"https://{console_url}").hostname or ""
if hostname in ("localhost", "127.0.0.1", "::1") or hostname.endswith(".localhost"):
    return "http://localhost:8088"

At minimum this deserves a parametrized negative case (https://localhost.customer.example.comhttps://api.localhost.customer.example.com) so the intent is recorded either way.

🤖 Generated by the Astra agent

Comment thread README.md
Comment on lines +63 to +66
For standalone custom domains, the SDK derives the API hostname by replacing a
leading `console.` or `app.` label with `api.`, or by adding an `api.` prefix
when neither label is present. Set `SPLUNK_AO_API_URL` explicitly when your API
does not follow this convention.

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 (documentation): This paragraph documents the console./app.api. mapping and the api. fallback, but omits the localhost branch directly above it in the resolver, which is the case on-prem developers hit first: any console URL containing localhost or 127.0.0.1 resolves to http://localhost:8088 regardless of the console port. Anyone running the API on a different local port must set SPLUNK_AO_API_URL, and nothing tells them so.

Worth one more sentence here, especially since splunk-ao-a2a/README.md:76 advertises http://localhost:8088 as the example console URL:

Suggested change
For standalone custom domains, the SDK derives the API hostname by replacing a
leading `console.` or `app.` label with `api.`, or by adding an `api.` prefix
when neither label is present. Set `SPLUNK_AO_API_URL` explicitly when your API
does not follow this convention.
For standalone custom domains, the SDK derives the API hostname by replacing a
leading `console.` or `app.` label with `api.`, or by adding an `api.` prefix
when neither label is present. Local development console URLs (`localhost` or
`127.0.0.1`) always resolve to `http://localhost:8088`. Set `SPLUNK_AO_API_URL`
explicitly when your API does not follow these conventions — for example when
the console and API share a hostname, or when a local API listens on a port
other than 8088.

🤖 Generated by the Astra agent

@fercor-cisco

Copy link
Copy Markdown
Collaborator

Triaged the review — responses below.

api. prefix on IP-literal / single-label hosts — accepted as non-blocking for this PR. Filed as HYBIM-963, which also covers the path-leak and the startswith("api.") port/path issue, since parsing the hostname fixes all three.

CHANGELOG ### Changed entry — disagree, keeping this under ### Fixed. There are no deployments serving the console and API from the same host except dev-mode localhost, which the resolver already special-cases. The api. prefix is an addition: it creates cases where SPLUNK_AO_API_URL no longer needs to be set, and removes none that worked before. No migration step applies.

"localhost" in console_url substring matchhttps://localhost.customer.example.com is a spurious case, not handling it.

README localhost branch — not documenting; the intended audience is not running the API server locally.

Two low-priority follow-ups filed:

  • HYBIM-964 — debug log when the derived API host differs from the console host, naming SPLUNK_AO_API_URL as the override.
  • HYBIM-965examples/agent/langgraph-otel/main.py hand-rolls the old derivation and now disagrees with the SDK for custom console hosts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants