Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update telemetry to support command-remapping #848

Merged

Conversation

marckhouzam
Copy link
Contributor

@marckhouzam marckhouzam commented Jan 28, 2025

What this PR does / why we need it

This PR adapts the telemetry logic to respect command-remapping of plugin commands.

The important changes are as follows.

1- The telemetry plugin command-tree (~/.cache/tanzu/plugins_command_tree/command_tree.yaml) is no longer built when a plugin is installed, but when the plugin is first invoked. This is because the new logic needs to find the cobra command that has been created for a remapped command, and this cobra command does not exist yet when the plugin is being installed.

2- The telemetry plugin command-tree was previously only showing the sub-commands of a plugin. This is no longer sufficient because a plugin can remap a command to be outside its own command-tree path. For example the resource plugin provides a tanzu deploy command which is clearly not under the tanzu resource command-path. To fix this, this PR build the telemetry command-tree in such a way that it includes the entire, remapped, hierarchy of the commands provided by a plugin. For example, for the resource plugin, we would see both the deploy and the resource commands at the first level of that plugin's command-tree.

3- The telemetry plugin command-tree did not previously include targets (e.g., operations). This was fine because the command-tree represented sub-commands of the plugin, and when telemetry was gathered when a command was typed, it could ignore the target. This is no longer working because when collecting telemetry, we need to start from the exact command-path that the user typed because we need to accept remapped commands; and this user input sometimes has the target present.

4- When determining aliases for a command, we now need to account for remapped command and for such cases, map the user input to the how the command can be invoked directly in the plugin. For example, when looking for aliases for tanzu deploy, we need to call <resource_plugin_binary> deploy and this mapping is not always this straight forward. This PR therefore looks for the cobra command that is invoked by what the user typed, and uses a new annotation to know how to map it back to a plugin invocation. Note that it would have been possible to invoke the tanzu CLI directly to print the help of a plugin command and let it do the remapping automatically; the problem however, is that the CLI is much slower than calling a plugin directly (due to its overhead), so building the command-tree becomes very slow. This PR therefore continues to call the plugin binary directly to invoke the help of the plugin's different command to obtain any aliases.

5- Backwards-compatibility: because the telemetry plugin command-tree is different than the one built by older CLIs, it would affect telemetry of those older CLIs, when running them after having run the new CLI with the new command-tree format. To avoid this, this PR changes the location of the command-tree from command_tree.yaml to command_tree_v2.yaml. With this change, both the new CLI and the older ones can be used alternatively and they will each have their own command-tree with its own format.

Which issue(s) this PR fixes

Fixes # N/A

Describe testing done for PR

I tested the following cases:

  1. doing a tanzu plugin install --group vmware-tanzu/plugin-engineer with the Asteroid versions and seeing that all plugins get installed, but that no plugin tree gets created yet
  2. doing a tanzu plugin clean and seeing the new cache and old cache are cleared (the directory is removed), then rebuilding the command-tree one plugin at a time
  3. doing a tanzu plugin uninstall acceleratorv2 and seeing that the command-tree for that plugin only was removed
  4. Running each plugin and made sure there was no error generating the command-tree in command_tree_v2.yaml for each one:
tz accelerator -h
tz ops apply -h
tz apps -h       
tz build -h      
tz ops cluster -h     
tz cluster-group -h
tz ops clustergroup -h
tz domain -h
tz domain-binding -h
tz egress -h
tz package -h
tz project -h
tz role -h
tz resource -h
tz route -h
tz secret -h
tz services -h
tz space -h
tz workflow -h

# Using aliases
tz project ls
tz apps config nse delete -h
  1. Looked at the content of the telemetry database while running different commands:
# Core command
tz context list
# global plugin
tz space list
# k8s plugin
tz secret -h
tz k8s secret -h
tz kubernetes secret -h
# ops plugin
tz ops clustergroup
tz operations clustergroup
# Remapped commands
tz cluster -h
tz cluster uncordon -h
tz deploy -h
tz role binding list -h
tz role bind Administrator
  1. Looked at the content of the telemetry database while running different commands with tanzu CLI version 1.5.1:
# Core command
tanzu context list
# global plugin
tanzu space list
# k8s plugin
tanzu secret -h
tanzu k8s secret -h
tanzu kubernetes secret -h
# ops plugin
tanzu ops clustergroup
tanzu operations clustergroup
# Remapped commands
tanzu cluster -h
tanzu cluster uncordon -h # this one didn't catch the uncordon because remapping isn't handled in old CLIs
tanzu deploy -h
tanzu role binding list -h # this one didn't catch anything after "role" because remapping isn't handled in old CLIs
tanzu role bind Administrator # this one didn't catch anything after "role" because remapping isn't handled in old CLIs

Release note

Update the telemetry logic to support command-remapping

Additional information

Special notes for your reviewer

@marckhouzam marckhouzam force-pushed the marck/fixTelemetryRemapping branch from 86b4a22 to f878029 Compare January 28, 2025 20:53
@marckhouzam marckhouzam marked this pull request as ready for review January 28, 2025 21:37
@marckhouzam marckhouzam requested a review from a team as a code owner January 28, 2025 21:37
@marckhouzam marckhouzam force-pushed the marck/fixTelemetryRemapping branch 2 times, most recently from 8fa6ea9 to 6b69507 Compare January 28, 2025 22:59
Copy link
Contributor

@vuil vuil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a really nice set of changes, thanks!
Minor comment nit.
Also a few typos in the PR description

near "how the command can be invoked"
because very slow -> becomes very slow

finally some questions about behavior of an old CLI using the new command tree format.

pkg/cli/plugin_cmd.go Outdated Show resolved Hide resolved
pkg/plugincmdtree/plugin_cache_reset.go Outdated Show resolved Hide resolved
- When building the command-tree of each plugin, we include all root
commands since the plugin can provide many such commands; before a
plugin only provided its own name as a root command.

- When building the command-tree of each plugin, we also include any
applicable target. This is important so telemetry can map the command
provided by the user with the command tree.

- When saving a command invocation to telemetry, we need to pass the
entire command path, since the plugin name is no longer necessarily a
prefix in the invocation.

- Fix finding aliases by determinining the source command-path within
the plugin command-tree and invoking it with "-h".

- Clear the previous telemety command-tree if the last CLI that was run
was < 1.5.3. This is because the command-tree generated by 1.5.3 is
different than for previous versions.

Signed-off-by: Marc Khouzam <[email protected]>
@marckhouzam marckhouzam force-pushed the marck/fixTelemetryRemapping branch from 6b69507 to 800e9ef Compare January 28, 2025 23:48
Copy link
Contributor

@prkalle prkalle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for changes. Nicely done!
Looks good at the first pass. I will do some testing and update if I find any issues/comments

pkg/plugincmdtree/cache.go Show resolved Hide resolved
Signed-off-by: Marc Khouzam <[email protected]>
To allow older CLIs (< 1.5.3) to continue using the old command-tree
format for telemetry, this commit uses a new command_tree_v2.yaml file
for the new format. Switching back and forth from new to old CLIs will
no longer mess with the same plugin tree cache and will keep telemetry
in a better shape for older CLIs.

This change also means we do not need to reset the command_tree.yaml
file using a global initializer, so this commit remove all chaneges
related to that.

Signed-off-by: Marc Khouzam <[email protected]>
Copy link
Contributor

@vuil vuil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the prompt updates, lgtm!

@marckhouzam marckhouzam merged commit 63a1eb4 into vmware-tanzu:main Jan 29, 2025
7 checks passed
@marckhouzam marckhouzam deleted the marck/fixTelemetryRemapping branch January 29, 2025 01:31
@marckhouzam marckhouzam added this to the v1.5.3 milestone Jan 29, 2025
marckhouzam added a commit to marckhouzam/tanzu-cli that referenced this pull request Jan 29, 2025
- When building the command-tree of each plugin, we include all root
commands since the plugin can provide many such commands; before a
plugin only provided its own name as a root command.

- When building the command-tree of each plugin, we also include any
applicable target. This is important so telemetry can map the command
provided by the user with the command tree.

- When saving a command invocation to telemetry, we need to pass the
entire command path, since the plugin name is no longer necessarily a
prefix in the invocation.

- Fix finding aliases by determining the source command-path within
the plugin command-tree and invoking it with "-h".

- To allow older CLIs (< 1.5.3) to continue using the old command-tree
format for telemetry, this commit uses a new command_tree_v2.yaml file
for the new format. Switching back and forth from new to old CLIs will
therefore not mess with the same plugin command-tree cache and will
keep telemetry in a better shape for older CLIs.

Signed-off-by: Marc Khouzam <[email protected]>
anujc25 pushed a commit that referenced this pull request Jan 29, 2025
- When building the command-tree of each plugin, we include all root
commands since the plugin can provide many such commands; before a
plugin only provided its own name as a root command.

- When building the command-tree of each plugin, we also include any
applicable target. This is important so telemetry can map the command
provided by the user with the command tree.

- When saving a command invocation to telemetry, we need to pass the
entire command path, since the plugin name is no longer necessarily a
prefix in the invocation.

- Fix finding aliases by determining the source command-path within
the plugin command-tree and invoking it with "-h".

- To allow older CLIs (< 1.5.3) to continue using the old command-tree
format for telemetry, this commit uses a new command_tree_v2.yaml file
for the new format. Switching back and forth from new to old CLIs will
therefore not mess with the same plugin command-tree cache and will
keep telemetry in a better shape for older CLIs.

Signed-off-by: Marc Khouzam <[email protected]>
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