HYBIM-898 Update migration tool README(New PR, DO NOT REVIEW) - #229
HYBIM-898 Update migration tool README(New PR, DO NOT REVIEW)#229ridhima-splunk wants to merge 1 commit into
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 — The new enable_metrics() → set_metrics() row (and its checklist mirror) is factually wrong against the actual galileo/splunk_ao APIs and will break migrations that follow it; keyword-argument renames that accompany the log stream → agent stream rename are also entirely undocumented.
General Comments
- 🟠 major (documentation): I verified every rename row in this PR against the real APIs — current
src/splunk_ao/and the pre-rename galileo sources preserved in git history (f129877:src/galileo/...for the original galileo SDK,28f0746^for pre-entity-rename). Most rows check out (module paths,Metric/Llm/Local/Code→*Evaluator,BuiltInEvaluators+Evaluator.metrics,MetricSpec/LocalMetricConfig/schema.metrics.Metricunchanged,Projectmethod renames, all module-level helper renames).
Two classes of problem remain:
- The
enable_metrics()→set_metrics()row is wrong (see line 238 / line 526).galileo.log_stream.LogStreamnever hadenable_metrics()— it hadget_metrics()/set_metrics(), and both names are unchanged insplunk_ao.agent_stream.AgentStream.enable_metrics()only ever existed ongalileo.log_streams.LogStream(theLogStreamResponsesubclass), theLogStreamsservice, and the module-level function — all three becameenable_evaluators(). - The guide documents class/function/module renames but omits the keyword-argument renames that came with the same entity rename (see line 268). Those produce
TypeErrorat runtime, so they belong in a guide that claims to document "every breaking change a customer must make".
Worth noting the internal docs/domain-entity-rename.md:190-191 contains the same incorrect log_stream.enable_metrics(…) → agent_stream.set_metrics(…) mapping, which is presumably where this row came from — that file should be corrected too so the error doesn't get copied again.
Follow-ups
Suggested follow-up work that could be tracked as Jira tickets:
docs/domain-entity-rename.md:190-191: The internal rename doc carries the same incorrect mapping this PR copied into the customer guide:log_stream.enable_metrics(…) → agent_stream.set_metrics(…)and "enable_metrics(…) → enable_evaluators(…)(AgentStreams service / module-level only)". In realityenable_metrics()only existed on thelog_streams.LogStreamresponse class /LogStreamsservice / module-level helper (all nowenable_evaluators(), including on the returnedAgentStreamobjects), whileget_metrics()/set_metrics()on the object-API class were never renamed. Correct this file so the error is not propagated into future docs.splunk-ao-migration-tool/README.md:140-154: The §2.2 sub-module table has no rows for the__future__surface. galileo exposedgalileo.__future__.log_streamandgalileo.__future__.metricsubmodules plusfrom galileo.__future__ import LogStream, Metric, LlmMetric, …; in splunk_ao those submodules were deleted outright (sibling shims like__future__/project.pysurvived), so the only equivalents arefrom splunk_ao.__future__ import AgentStream, Evaluator, …or the top-level modules. Worth a row or a note for anyone who adopted the__future__API.splunk-ao-migration-tool/README.md:273-288: Pre-existing gap unrelated to the entity rename: §3.4 lists OTel/handler class renames but omits the module-level function renameadd_galileo_span_processor()→add_splunk_ao_span_processor()(src/splunk_ao/otel.py:292, formerlyf129877:src/galileo/otel.py:315). Consider adding it alongside the class rows.
|
|
||
| | Class | Old method / property | New method / property | | ||
| |-------|-----------------------|-----------------------| | ||
| | `AgentStream` (was `LogStream`) | `enable_metrics()` | `set_metrics()` | |
There was a problem hiding this comment.
🟠 major (bug): This row is incorrect in both columns, and following it will break migrating code.
There are two distinct classes named AgentStream (as there were two named LogStream in galileo), and they have disjoint metric APIs:
splunk_ao.agent_stream.AgentStream(the object API, exported asfrom splunk_ao import AgentStream) hasget_metrics()andset_metrics()(src/splunk_ao/agent_stream.py:409,443). In galileo,galileo.log_stream.LogStreamhad exactly the same two methods (f129877:src/galileo/log_stream.py:407,441) —set_metrics()is not a rename of anything; it is unchanged, andenable_metrics()never existed on this class.splunk_ao.agent_streams.AgentStream(theLogStreamResponsesubclass returned byAgentStreams().get()/list()/create()and byget_agent_stream()/list_agent_streams()/create_agent_stream()) has onlyenable_evaluators()(src/splunk_ao/agent_streams.py:125) — noset_metrics().
So the customers who actually wrote log_stream.enable_metrics([...]) are holding the second kind of object, and this row tells them to call .set_metrics(...) on it → AttributeError.
Suggested replacement rows below (the get_metrics() note at line 245 then becomes redundant and can be dropped).
| | `AgentStream` (was `LogStream`) | `enable_metrics()` | `set_metrics()` | | |
| | `AgentStream` (object API, `splunk_ao.agent_stream`) | `get_metrics()` / `set_metrics()` | **unchanged** — same names, no migration needed | | |
| | `AgentStream` (service result, `splunk_ao.agent_streams` — returned by `AgentStreams()` and the module-level helpers) | `enable_metrics()` | `enable_evaluators()` | |
🤖 Generated by the Astra agent
| - [ ] Update evaluator module imports: `splunk_ao.metric` → `splunk_ao.evaluator` | ||
| - [ ] Rename `LogStream` → `AgentStream`, `LogStreams` → `AgentStreams` | ||
| - [ ] Update `Project` method calls: `create_log_stream()` → `create_agent_stream()`, `list_log_streams()` → `list_agent_streams()`, `.logstreams` → `.agent_streams` | ||
| - [ ] Replace `log_stream.enable_metrics()` with `agent_stream.set_metrics()` (use `enable_evaluators()` only on the `AgentStreams` service / module-level helper); `AgentStream.get_metrics()` is unchanged |
There was a problem hiding this comment.
🟠 major (bug): Same defect as the table row at line 238, and the parenthetical makes it worse: enable_evaluators() is not restricted to the service class and module-level helper — it is also the method on the AgentStream instances those helpers return (src/splunk_ao/agent_streams.py:125). Conversely set_metrics() exists only on the object-API splunk_ao.agent_stream.AgentStream, where it was already called set_metrics() in galileo, so it is not a migration step at all.
| - [ ] Replace `log_stream.enable_metrics()` with `agent_stream.set_metrics()` (use `enable_evaluators()` only on the `AgentStreams` service / module-level helper); `AgentStream.get_metrics()` is unchanged | |
| - [ ] Replace `enable_metrics()` with `enable_evaluators()` on the `AgentStreams` service, on `AgentStream` objects returned by `AgentStreams()` / `get_agent_stream()` / `list_agent_streams()` / `create_agent_stream()`, and on the module-level helper — and update the keyword `log_stream_name=` → `agent_stream_name=` | |
| - [ ] Note: `AgentStream.get_metrics()` and `AgentStream.set_metrics()` on the object API (`splunk_ao.agent_stream`) keep their existing names |
🤖 Generated by the Astra agent
| **Module-level convenience function renames:** | ||
|
|
||
| | Old (`galileo`) | New (`splunk_ao`) | | ||
| |-----------------|-------------------| | ||
| | `get_log_stream()` | `get_agent_stream()` (`splunk_ao.agent_streams`) | | ||
| | `list_log_streams()` | `list_agent_streams()` (`splunk_ao.agent_streams`) | | ||
| | `create_log_stream()` | `create_agent_stream()` (`splunk_ao.agent_streams`) | | ||
| | `enable_metrics()` | `enable_evaluators()` (`splunk_ao.agent_streams`) | | ||
| | `get_metrics()` | `get_evaluators()` (`splunk_ao.evaluators`) | | ||
| | `delete_metric()` | `delete_evaluator()` (`splunk_ao.evaluators`) | | ||
| | `create_custom_llm_metric()` | `create_custom_llm_evaluator()` (`splunk_ao.evaluators`) | |
There was a problem hiding this comment.
🟠 major (documentation): The rename also changed public keyword-argument names, which this guide never mentions. Since these are keyword-only or commonly passed by keyword, migrated code that only applies the documented class/function renames still fails with TypeError at runtime. Verified renames (galileo → splunk-ao):
| Callable | Old keyword | New keyword |
|---|---|---|
enable_evaluators() (module-level and AgentStreams method) |
log_stream_name= |
agent_stream_name= (src/splunk_ao/agent_streams.py:690,483) |
get_evaluators() |
log_stream_id= |
agent_stream_id= (src/splunk_ao/evaluators.py:207) |
export_records() / Export.records() |
log_stream_id= |
agent_stream_id= (src/splunk_ao/export.py:39) |
SplunkAOLogger(...) |
log_stream_id= |
agent_stream_id= (src/splunk_ao/logger/logger.py:239) |
SplunkAOOTLPExporter(...) / SplunkAOSpanProcessor(...) |
logstream=, log_stream_id= |
agentstream=, agent_stream_id= |
agent_streams.AgentStream(...) ctor |
log_stream= |
agent_stream= (src/splunk_ao/agent_streams.py:103) |
The OTel pair is worth calling out explicitly: src/splunk_ao/otel.py:58-64 rejects logstream/log_stream_id with an explicit TypeError, so it is a hard failure rather than a silently-ignored kwarg. Suggest adding a short "keyword argument renames" subsection here (plus a checklist item in §9) with the rule any log_stream/log_stream_id/log_stream_name keyword becomes agent_stream* and the OTel logstream= → agentstream= exception to the underscore convention.
🤖 Generated by the Astra agent
|
|
||
| ### 3.3a Log Stream / Agent Stream Classes | ||
|
|
||
| > **Domain rename:** "Log Streams" → "Agent Streams". All classes and methods containing "LogStream" or "log_stream" are renamed to use "AgentStream" / "agent_stream". |
There was a problem hiding this comment.
🟡 minor (documentation): "All classes and methods containing LogStream or log_stream are renamed" is too strong and will send readers looking for renames that don't exist. The generated transport layer deliberately keeps the old naming: splunk_ao.resources.api.log_stream, LogStreamResponse, LogStreamCreateRequest (imported at src/splunk_ao/agent_streams.py:6-13), and the wire format is unchanged too — the OTLP header is still logstream and the resource attribute still splunk_ao.logstream.name (src/splunk_ao/exporter/config.py:136,154). Scoping the claim to the public object/service APIs avoids the confusion.
| > **Domain rename:** "Log Streams" → "Agent Streams". All classes and methods containing "LogStream" or "log_stream" are renamed to use "AgentStream" / "agent_stream". | |
| > **Domain rename:** "Log Streams" → "Agent Streams". Public classes, methods, and module paths containing `LogStream` / `log_stream` are renamed to `AgentStream` / `agent_stream`. The generated transport layer (`splunk_ao.resources.*`, e.g. `LogStreamResponse`) and the on-the-wire OTLP header / resource attribute names are intentionally **unchanged** — server-side renaming is tracked separately. |
🤖 Generated by the Astra agent
See old PR for description and comments history.