Refactor command telemetry

[carole-lavillonniere]

Refactoring addressing suggested improvement #2 from @thrau in this comment #127 (review)

Changes

• Replaces the commandWithTelemetry decorator and per-command manual telemetry with a single instrumentCommands function in root.go that walks the Cobra command tree and wraps every RunE automatically
• Command names are derived from cmd.CommandPath() (stripping the lstk prefix) — no more hardcoded strings that can drift
• Removes *telemetry.Client from constructors that only needed it for the decorator (logout, status, logs, setup, config, volume, update, aws); kept where still needed for lifecycle events (start, stop, restart) or token updates (login)

Deviation from the suggestion

You can use Cobra's PersistentPreRun func to store data globally that you need (for example the startTime)

The PR instead captures startTime directly inside the RunE closure in instrumentCommands. This is simpler because local to the closure and there is no shared state needed.

Instead, we could centralize this into root.go and remove the tel dependency out of the constructors completely.

tel must still be passed to constructors for commands that do more things with it: emitting lifecycle events, updating auth token.

Change in telemetry data

Before this change lstk (without subcommand) and lstk start both emitted command="start" and could not be differentiated in telemetry. Now lstk emits "lstk" instead.

[anisaoshafi]

LGTM, much simpler 🚀

Question: I wasn't aware we want to track "lstk" differently from "lstk start". Why do we want to check them separately? I assume we need to adjust the dashboard queries so that start events count "lstk" in addition to "lstk start".

In AGENTS.md this part needs to be removed: "Wrap the command's RunE with commandWithTelemetry(name, tel, fn) — this handles timing, exit code, and error message automatically".

[carole-lavillonniere]

Question: I wasn't aware we want to track "lstk" differently from "lstk start". Why do we want to check them separately? I assume we need to adjust the dashboard queries so that start events count "lstk" in addition to "lstk start".

Good point. The different command name comes for free with the refactoring, and I assumed it would be better to have this added visibility over lstk versus lstk start. However if it's extra work with not much added value, I can bring back the previous command name. @thrau @mmaureenliu?

In AGENTS.md this part needs to be removed: "Wrap the command's RunE with commandWithTelemetry(name, tel, fn) — this handles timing, exit code, and error message automatically".

I cannot find this string in AGENTS.md, is it Claude or myself hallucinating 🙃?

@anisaoshafi

[anisaoshafi]

In AGENTS.md this part needs to be removed: "Wrap the command's RunE with commandWithTelemetry(name, tel, fn) — this handles timing, exit code, and error message automatically".

I cannot find this string in AGENTS.md, is it Claude or myself hallucinating 🙃?

@carole-lavillonniere sorry, my bad. I meant in add-commands/SKILL.md

[thrau]

Thank you for following up on this @carole-lavillonniere! To me this is much cleaner now and objectively better!

• Telemetry is now treated as a cross-cutting concern. By centralizing instrumentation in root.go, the individual commands (like aws, logs, or status) don't need to know that they are being timed or tracked.
• Adding a new command is now simpler. You don't have to worry about telemetry wiring, the framework handles it automatically.
• The use of recursive command wrapping ensures 100% coverage. Every command in the tree is now automatically instrumented with basic telemetry (execution time, exit code, flags), which makes analytics much more reliable.

I would consider adding a way to disable telemetry for individual commands that we may not want to track, once that need comes up.

[thrau]

praise: updating skills is sometimes forgotten! great that you keep it up to date with every change 🙌

[mmaureenliu]

Good point. The different command name comes for free with the refactoring, and I assumed it would be better to have this added visibility over lstk versus lstk start. However if it's extra work with not much added value, I can bring back the previous command name. @thrau @mmaureenliu?

There are two separate things we might want to understand from the lstk vs lstk start commands telemetry:

1. Intended operation, such as start, stop - here we don't care about the differentiation but need to make sure the logic (when counting start we also need to count lstk) is captured somewhere and applied correctly to stats without forcing the end data consumer to actively understand this nuance
2. User habits, i.e. do they prefer to use simply lstk or do they insist on lstk start. We may want to use this data to revise our assumption and to feed into future decision making on other tools. In this case we do need to differentiate between lstk and lstk start

In either case, we need to be careful with lstk -h, lstk -v, etc., which != lstk start (I guess you've taken care of this already?)

In terms of implementation, #1 is most important for me to track, so I don't mind whether differentiation is done but want to be sure counting is correct. #2 is nice to have.

[carole-lavillonniere]

Good point. The different command name comes for free with the refactoring, and I assumed it would be better to have this added visibility over lstk versus lstk start. However if it's extra work with not much added value, I can bring back the previous command name. @thrau @mmaureenliu?

There are two separate things we might want to understand from the lstk vs lstk start commands telemetry:

1. Intended operation, such as `start`, `stop` - here we don't care about the differentiation but need to make sure the logic (when counting `start` we also need to count `lstk`) is captured somewhere and applied correctly to stats without forcing the end data consumer to actively understand this nuance

2. User habits, i.e. do they prefer to use simply `lstk` or do they insist on `lstk start`. We may want to use this data to revise our assumption and to feed into future decision making on other tools. In this case we do need to differentiate between `lstk` and `lstk start`

In either case, we need to be careful with lstk -h, lstk -v, etc., which != lstk start (I guess you've taken care of this already?)

In terms of implementation, #1 is most important for me to track, so I don't mind whether differentiation is done but want to be sure counting is correct. #2 is nice to have.

Thanks @mmaureenliu, and @anisaoshafi for bringing attention to the matter.
I pushed this commit 34f0724 that will bring back the telemetry for lstk. It should now be unchanged and both lstk and lstk start emit command="start"

[anisaoshafi]