fix: stop recording self completions in telemetry - #8
Merged
Conversation
`eval "$(arkouda self completions zsh)"` in a shell startup file fires one telemetry event per shell opened rather than per deliberate use. On a sample local log this accounted for 765 of 1056 events (72%), burying the real invocations the log exists to surface. The telemetry ADR already documented `command` as one of `list`, `decision`, `check`, or `new`, so `self` was never part of the recorded set; this brings the implementation back in line with it. Match on `SelfCommand::Completions` rather than all of `SelfCmd` so a future `self` subcommand is not silently dropped from the log.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds an ChangesTelemetry recording exclusion
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Main
participant Cli
participant Command
participant Telemetry
Main->>Cli: parse args
Main->>Command: is_recorded()
Command-->>Main: true/false
alt is_recorded == true
Main->>Telemetry: record event
else is_recorded == false
Main->>Main: skip recording
end
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
arkouda self completions zshis meant to be wired into a shell startup file:which means it fires once per shell opened, not once per deliberate use of the tool. It therefore floods the telemetry log with events that carry no information about how arkouda is actually used.
On a local log covering 2026-05-20 → 2026-07-09, this was 765 of 1056 events (72%). Filtering it out leaves 291 real invocations — and only then does the log answer the questions the telemetry ADR was written to answer. For instance,
decision --sectionturns out to be used in 1 of 30decisioncalls, despite the skill prompt recommending it.Change
Skip telemetry for
self completions.The ADR already documented
commandas one oflist,decision,check, ornew—selfwas never part of the recorded set, so this is implementation drift being corrected rather than a decision being reversed. The ADR's "What is not recorded" section is updated to state the exclusion explicitly.Command::is_recorded()matches on the innerSelfCommand::Completionsrather than on all ofSelfCmd, so a futureselfsubcommand (e.g.self update) is not silently dropped from the log.Verification
make cigreen: fmt, clippy, 30 tests,arkouda checkclean on all 4 ADRs.ARKOUDA_STATE_DIR: threeself completions zshruns create no log file at all, whilecheckandliststill record normally.Note for reviewers
Existing logs still contain the historical
selfevents. They can be dropped in place with:Downstream consumers should tolerate their presence in older files either way.
Summary by CodeRabbit
Bug Fixes
Documentation