docs(HYBIM-856): align evaluator terminology after HYBIM-949 rename - #202
docs(HYBIM-856): align evaluator terminology after HYBIM-949 rename#202shuningc wants to merge 5 commits into
Conversation
Update SplunkAOEvaluators docstrings and rename stale test_galileo_metrics_* tests to test_evaluators_*. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
fercor-cisco
left a comment
There was a problem hiding this comment.
🤖 This review was generated by the Astra agent (claude-opus-5). It may contain mistakes.
Verdict: request_changes — The rename is correct as far as it goes, but the PR's own stated scope (removing stale galileo_metrics test naming) is left incomplete, and the PR is filed under HYBIM-856 — an unrelated, already-resolved ticket — so there is no valid tracking issue describing this work.
General Comments
- 🟠 major (testing): Stated scope is not fully covered — one stale
galileo_metricstest name remains.
The PR description says it will "Rename stale test_galileo_metrics_* functions ... to test_evaluators_*". A repo-wide grep for galileo_metrics shows exactly one occurrence left after this change:
tests/test_experiment.py:1502: def test_lookup_by_galileo_metrics_enum(
That test is specifically about looking up an aggregate by the SplunkAOEvaluators enum (see line 1516: # When: looking up by SplunkAOEvaluators enum), so it is exactly the same class of stale naming the three functions in tests/schemas/test_metrics.py had. Leaving it behind means the next person grepping galileo_metrics to confirm the HYBIM-949 rename is complete will still get a hit, which is the problem this PR exists to eliminate.
Suggested rename: test_lookup_by_evaluators_enum. No other code references the name (no CI -k selectors or test-name allowlists match it), so the rename is safe.
For reference, the only other remaining GalileoMetrics references are in splunk-ao-migration-tool/README.md (lines 199, 203, 444). Those are intentional — they document the old→new mapping for migrating users — and should be left alone.
- 🟠 major (question): The ticket reference in the title and branch name points at an unrelated, already-closed ticket.
The PR is titled docs(HYBIM-856): align evaluator terminology after HYBIM-949 rename and the branch is HYBIM-856-evaluators-docstring, but HYBIM-856 is:
"Fix comment in
examples/logging-samples/openai-responses/main.py" — Suggested fix: on line 15 ... changeBecause we imported from galileotoBecause we imported from splunk_ao
That ticket is Resolved / Fixed (2026-07-10), was delivered by PR #70, and the fix is already in the tree — examples/logging-samples/openai-responses/main.py:15 already reads Because we imported from `splunk_ao` . It has nothing to do with SplunkAOEvaluators docstrings or test naming.
HYBIM-949 (Rename SplunkAOMetrics to SplunkAOEvaluators) is also already Resolved (2026-07-31), so it can't be the tracking ticket for follow-up work either.
So this change has no open ticket describing the problem it solves or the intended end state. That matters beyond bookkeeping: without a specification of "which remaining references are in scope," there's no fixed target to check the diff against — which is precisely how the tests/test_experiment.py occurrence flagged above slipped through. Could you either (a) file a new HYBIM ticket for the terminology cleanup and retitle, or (b) point at the correct existing ticket if I've misread the mapping?
Follow-ups
Suggested follow-up work that could be tracked as Jira tickets:
src/splunk_ao/evaluator.py:71-81:BuiltInEvaluatorsstill uses pre-rename terminology internally and in user-visible output: the__getattr__docstring says "built-in metrics", the loop variable isscorer, and the raisedAttributeErrormessage reads "Built-in metric '{name}' not found". Since that error string surfaces directly to SDK users doingEvaluator.metrics.<typo>, it is the most visible remaining instance of the old vocabulary. Worth folding into a broader terminology sweep rather than this narrowly-scoped PR.src/splunk_ao/agent_streams.py:79-205: Theenable_evaluatorsdocstrings and examples throughoutagent_streams.pystill say "Enable metrics on a log stream", "just pass the metrics!", "List of metrics to enable on this log stream", etc. — both the metrics→evaluators and log stream→agent stream renames are unreflected in this user-facing documentation. Themetrics=parameter name itself is public API and should not change without a deprecation path, but the prose and example comments can be aligned. Good candidate for a single follow-up ticket covering all remaining docstring terminology (also touchessrc/splunk_ao/types.py:11-16andsrc/splunk_ao/utils/metrics.py:101-133).
| """Built-in Splunk AO evaluators. | ||
| Values are human-readable UI labels used for scorer lookup via the API. | ||
| Values are human-readable UI labels used for evaluator lookup via the API. |
There was a problem hiding this comment.
🟡 minor (documentation): This rewording drops a detail that was load-bearing, not just legacy terminology.
docs/domain-entity-rename.md:26-27 states explicitly:
Note: The underlying API endpoints continue to use the previous paths (
/log_streams,/scorers) — server-side renaming is tracked separately.
and docs/domain-entity-rename.md:201: "The rename is purely client-side. The server still uses /log_streams and /scorers paths."
The code bears this out — these enum values are resolved through the scorer API: src/splunk_ao/utils/metrics.py:142 pushes metric.value into label_searches, which is then passed to scorers_client.list_by_labels(...) at line 164, matched against ScorerResponse.label at line 173, and turned into ScorerConfig objects.
So "scorer lookup via the API" was literally accurate about the wire layer, while "evaluator lookup via the API" now implies an evaluator-named API surface that doesn't exist. Keeping the scorer detail while adopting evaluator terminology for the SDK-facing concept preserves both.
| Values are human-readable UI labels used for evaluator lookup via the API. | |
| Values are human-readable UI labels, matched against scorer labels by the | |
| API (endpoints still use the legacy `/scorers` paths — see | |
| docs/domain-entity-rename.md). |
🤖 Generated by the Astra agent
Complete the evaluator terminology cleanup by renaming the remaining galileo_metrics test identifier in test_experiment.py. Co-authored-by: Cursor <cursoragent@cursor.com>
Update user-facing docstrings and error messages to use evaluator/agent stream vocabulary after the SplunkAOEvaluators rename. Keep the public metrics= parameter name unchanged for API compatibility. Co-authored-by: Cursor <cursoragent@cursor.com>
Document that enum values are resolved via legacy /scorers endpoints while keeping evaluator terminology for the SDK-facing concept. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Can you please add to CHANGELOG? |
Co-authored-by: Cursor <cursoragent@cursor.com>
Summary
Follow-up to HYBIM-949 (SplunkAOMetrics → SplunkAOEvaluators) to align remaining docs and test naming with evaluator terminology.
Update SplunkAOEvaluators docstring in src/splunk_ao/schema/metrics.py: replace “metric scorers” / “scorer lookup” with “evaluators” / “evaluator lookup”
Rename stale test_galileo_metrics_* functions in tests/schemas/test_metrics.py to test_evaluators_*
No runtime or API behavior changes — documentation and test naming only.