HYBIM-898 Update migration tool README - #224
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
|
recheck |
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 — Four of the newly added rename rows do not match the shipped SDK (MetricSpec/LocalMetricConfig were never renamed; AgentStream.get_metrics()/enable_metrics() are misattributed), so users following the guide will hit ImportError/AttributeError.
General Comments
- 🟠 major (documentation): The rename tables added here were not cross-checked against the shipped API, and four rows are factually wrong (
MetricSpec → EvaluatorSpec,LocalMetricConfig → LocalEvaluatorConfig,AgentStream.get_metrics() → get_evaluators(),AgentStream.enable_metrics() → enable_evaluators()).EvaluatorSpecandLocalEvaluatorConfigdo not exist anywhere in the repo, andsplunk_ao.AgentStream(fromsrc/splunk_ao/agent_stream.py, the class exported in__init__.py) still hasget_metrics()/set_metrics()and has neitherget_evaluators()norenable_evaluators(). A migration guide is executable-by-hand instructions; a wrong row costs a user a debugging session.
This repo already carries the authoritative record of the HYBIM-730 rename in docs/domain-entity-rename.md, including a "Quick find-and-replace reference" that states exactly:
Metric → Evaluator (OO class, not schema model)
log_stream.enable_metrics(…) → agent_stream.set_metrics(…)
enable_metrics(…) → enable_evaluators(…) (AgentStreams service / module-level only)
get_metrics(…) → get_evaluators(…)
and explicitly notes splunk_ao.schema.metrics.Metric is not renamed. Please rebuild §3.3/§3.3a and the checklist from that document plus src/splunk_ao/__init__.py, and verify each "New" symbol is actually importable before merge.
Follow-ups
Suggested follow-up work that could be tracked as Jira tickets:
splunk-ao-migration-tool/README.md:1-1: Nothing verifies this guide against the SDK, which is how the incorrect rename rows got in. Consider a small test (e.g.tests/test_migration_guide.py) that parses the "New" column of the rename tables insplunk-ao-migration-tool/README.mdand asserts each symbol is importable fromsplunk_aoor the stated submodule viaimportlib/getattr. That turns future rename drift into a CI failure instead of a user-facing bug report.src/splunk_ao/agent_stream.py:409-496: Terminology inconsistency in the SDK itself (pre-existing, not introduced by this PR): after the Metrics → Evaluators rename, the publicAgentStreamstill exposesget_metrics()/set_metrics(), while the siblingAgentStreamclass inagent_streams.py:23exposesenable_evaluators(). Two same-named classes with different evaluator APIs is what made this guide easy to get wrong. Consider addingget_evaluators()/set_evaluators()as the canonical names on the public class (keeping the old ones as aliases for compatibility), and/or renaming the internalagent_streams.AgentStreamresponse wrapper to something distinct.src/splunk_ao/agent_stream.py:484-490: Theset_metrics()docstring example usesEvaluator.metrics.correctness. The HYBIM-730 commit message claimedEvaluator.evaluatorsas the canonical accessor, butevaluator.py:145only definesmetrics = BuiltInEvaluators(). Worth confirming which accessor is intended as public and aligning the docstrings (anddocs/domain-entity-rename.md, which documentsEvaluator.metrics) so the published API reference is unambiguous.
|
@fercor-cisco jira for follow-ups: https://splunk.atlassian.net/browse/HYBIM-994 |
Updated migration guide with domain entity renames (
Metrics → Evaluators, Log Streams → Agent Streams)GalileoMetric → SplunkAOEvaluator, GalileoMetrics → SplunkAOEvaluators, and lower-level renameswith import diff examples
LogStream → AgentStream, LogStreams → AgentStreams, method renames onAgentStream (get_metrics(), enable_metrics())andProject (create_log_stream(), list_log_streams(), .logstreams), with import and usage diff examplesgalileo.metric → splunk_ao.evaluatorNote: