Skip to content

docs(HYBIM-856): align evaluator terminology after HYBIM-949 rename - #202

Open
shuningc wants to merge 5 commits into
mainfrom
HYBIM-856-evaluators-docstring
Open

docs(HYBIM-856): align evaluator terminology after HYBIM-949 rename#202
shuningc wants to merge 5 commits into
mainfrom
HYBIM-856-evaluators-docstring

Conversation

@shuningc

@shuningc shuningc commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

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.

Update SplunkAOEvaluators docstrings and rename stale test_galileo_metrics_* tests to test_evaluators_*.

Co-authored-by: Cursor <cursoragent@cursor.com>
@shuningc
shuningc marked this pull request as ready for review August 3, 2026 22:26
@shuningc

shuningc commented Aug 4, 2026

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. 🚀

Reviewed commit: a6c3935df1

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

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 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 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_metrics test 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 ... change Because we imported from galileo to Because 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: BuiltInEvaluators still uses pre-rename terminology internally and in user-visible output: the __getattr__ docstring says "built-in metrics", the loop variable is scorer, and the raised AttributeError message reads "Built-in metric '{name}' not found". Since that error string surfaces directly to SDK users doing Evaluator.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: The enable_evaluators docstrings and examples throughout agent_streams.py still 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. The metrics= 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 touches src/splunk_ao/types.py:11-16 and src/splunk_ao/utils/metrics.py:101-133).

Comment thread src/splunk_ao/schema/metrics.py Outdated
"""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.

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 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.

Suggested change
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` pathssee
docs/domain-entity-rename.md).

🤖 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.

Updated

shuningc and others added 3 commits August 4, 2026 17:07
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>
@pradystar

Copy link
Copy Markdown
Collaborator

Can you please add to CHANGELOG?

Co-authored-by: Cursor <cursoragent@cursor.com>
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.

3 participants