Enable Fuchsia safe command line tool analytics #82

BobEvans · 2021-03-24T12:11:53Z

This PR allows Fuchsia DX tools to collect analytics without collecting any user passed data.

Any tool that wants to collect analytics can re-parse the args with an extra flag, "--dump_args_passed" to retrieve a string of the command options suitable for posting to analytics.

…es that is safe for analytics tracking

erickt

Could you also make sure that this flag shows up in --help?

erickt · 2021-03-25T03:33:42Z

README.md

@@ -174,4 +174,9 @@ struct SubCommandTwo {
 }
 ```

+It is also possible to retrieve just the names of the arguments and switches 
+passed at runtime, by including the switch `--dump_args_passed` in the args, similarly


Rather than --dump_args_passed, I think it'd be more aligned with the style guide as --dump-args-passed.

name updated to match style in latest commit.

Could you also make sure that this flag shows up in --help?

Should this be included in --help output or would it confuse an end user of a tool built using argh since it mirrors back to them the options and subcommands they entered?

erickt · 2021-03-25T03:37:37Z

argh_derive/src/lib.rs

@@ -291,7 +291,18 @@ fn impl_from_args_struct(
                    } else {
                        __remaining_args
                    };
-                    #name = Some(<#ty as argh::FromArgs>::from_args(&__command, __remaining_args)?);
+                    if __dump_args {


Can we make this a feature we have to opt into, rather than it being on by default? I figure this would only be used in certain situations. Something like this (or some better name):

#[derive(FromArgs)] #[argh(with_dump_args_passed)] struct Cmd { ... }

This seems reasonable to reduce code generated by the macro expansion.

One question, in the ffx case, e.g. open-ended set of plugins in separate files defining their own argh subcommand, is there any ordering guarantee in compilation such that the developer would only have to annotate the TopLevel struct with opt-in, or, would each plugin developer need to add the annotation to opt-in as well? I am thinking the latter.

FFX may be a non-standard use case. Do most users of argh define all args in one file?

That's a good question. According to the ffx docs, all the plugins are declared as subcommands, like:

#[ffx_command()] #[derive(FromArgs, Debug, PartialEq)] #[argh(subcommand, name = "example", description = "an example")] pub struct ExampleCommand { #[argh(positional)] /// example optional positional string parameter pub example: Option<String>, }

I would imagine we'd want something like #[argh(with_dump_args_passed)] would be specified on the main entry point. Do you know how this feature interacts with subcommands?

argh_derive/src/lib.rs

README.md

erickt · 2021-03-25T04:03:01Z

argh_derive/src/lib.rs

@@ -291,7 +291,18 @@ fn impl_from_args_struct(
                    } else {
                        __remaining_args
                    };
-                    #name = Some(<#ty as argh::FromArgs>::from_args(&__command, __remaining_args)?);
+                    if __dump_args {


This command seems pretty specialized. Could we make this an opt-in feature, with something like:

#[derive(FromArgs)] #[argh(description = "", dump_passed_args)] struct Cmd { ... }

Happy for suggestions on a better attribute name. We could even make it such that this flag was customizable, with something like:

#[derive(FromArgs)] #[argh(description = "", dump_passed_args="--dump-passed-args")] struct Cmd { ... }

To make it easier for people to customize.

See above question about a TopLevel opt-in versus per subcommand.

Is the benefit that the macro expansion will not generate the bits of logic to accumulate the passed_args if the user has opted out?

BobEvans · 2021-03-25T23:11:22Z

Thanks for the quick turnaround. I updated all the names to comply with the style guide and had a question or two about opt-in and about including a description in the help output.

BobEvans · 2021-04-09T22:22:07Z

Ping

Please enter the commit message for your changes. Lines starting

erickt · 2021-04-20T16:39:53Z

src/lib.rs

+    /// The first argument `command_name` is the identifier for the current
+    /// command, treating each segment as space-separated. The second argument,
+    /// args, is the rest of the command line arguments.
+    fn to_args(command_name: &[&str], args: &[&str]) -> String;


Why did you end up using a static method here? I imagined this more to be fn to_args(&self) -> Vec<String>;. This has two advantages:

The arguments need to be fully parsed first.

This would allow the caller to escape the arguments as necessary.

Err, I mean:

fn to_args(&self) -> Vec<String> { vec![] }

We need to have this be a default method for two reasons. First, adding a non-default method to a trait is not backwards compatible. Second, this defaults custom implementations to not return anything, which is more private.

erickt · 2021-04-20T16:42:26Z

README.md

+It is also possible to retrieve just the names of the arguments and switches 
+passed at runtime, by including the switch `--dump-args-passed` in the args, similarly
+to how `--help` can be passed. This elides any user values passed and creates an 
+output for safely collecting data for analytics.


I don't think we need --dump-args-passed now that we have FromArgs::to_args(). Then it's up to the caller whether or not to call this function to trigger this functionality, with something like:

#[derive(FromArgs)] struct Cmd { #[argh(rename = "dumped-args-passed")] dump_args: bool, ... } fn main() { let cmd = argh::from_env(); if cmd.dump_args { let dumped_args = cmd.to_args(); println!("{}", cmd.to_args().map(escape_arg).join(" ")); std::process::exit(0); } } fn escpe_arg(&str) -> String { ... }

erickt · 2021-04-20T16:43:28Z

argh_derive/src/lib.rs

@@ -291,7 +291,18 @@ fn impl_from_args_struct(
                    } else {
                        __remaining_args
                    };
-                    #name = Some(<#ty as argh::FromArgs>::from_args(&__command, __remaining_args)?);
+                    if __dump_args {
+                        __args_dump.push(__subcommand.name.to_string().replace(" ", "\\ "));


As mentioned below, I think we should keep the arguments as a vector to allow callers to escape arguments if necessary.

erickt · 2021-04-20T16:51:03Z

argh_derive/src/lib.rs

                    }
                )*
                unreachable!("no subcommand matched")
            }
+
+            // Note: This can never get called because enums are only SubCommand containers
+            fn to_args(command_name: &[&str], args: &[&str]) -> String {


Don't we need to recurse into the enum variants?

erickt · 2021-04-20T17:00:16Z

src/lib.rs

+    /// The first argument `command_name` is the identifier for the current
+    /// command, treating each segment as space-separated. The second argument,
+    /// args, is the rest of the command line arguments.
+    fn to_args(command_name: &[&str], args: &[&str]) -> String;


Err, I mean:

fn to_args(&self) -> Vec<String> { vec![] }

We need to have this be a default method for two reasons. First, adding a non-default method to a trait is not backwards compatible. Second, this defaults custom implementations to not return anything, which is more private.

erickt · 2021-07-08T00:07:22Z

Closing in favor of #91.

Bob Evans added 2 commits March 24, 2021 04:58

Add an option to retrieve a string containing args passed but no valu…

82ff4f4

…es that is safe for analytics tracking

removed obsolete method

af5b7e8

erickt self-requested a review March 25, 2021 03:28

erickt requested changes Mar 25, 2021

View reviewed changes

rename dump_args_passed

c46f014

Bob Evans added 2 commits April 12, 2021 14:56

Escaped spaces in subcommand names

93b10d3

Please enter the commit message for your changes. Lines starting

Made a top level Trait fn to product args list

6fd7c9f

erickt reviewed Apr 20, 2021

View reviewed changes

erickt requested changes Apr 20, 2021

View reviewed changes

erickt mentioned this pull request May 5, 2021

Add support for redacting commandline arguments #91

Merged

erickt closed this Jul 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable Fuchsia safe command line tool analytics #82

Enable Fuchsia safe command line tool analytics #82

BobEvans commented Mar 24, 2021

erickt left a comment

erickt Mar 25, 2021

BobEvans Mar 25, 2021

erickt Mar 25, 2021

BobEvans Mar 25, 2021

erickt Apr 13, 2021

erickt Mar 25, 2021

BobEvans Mar 25, 2021

BobEvans commented Mar 25, 2021

BobEvans commented Apr 9, 2021

erickt Apr 20, 2021

erickt Apr 20, 2021

erickt Apr 20, 2021

erickt Apr 20, 2021

erickt Apr 20, 2021

erickt Apr 20, 2021

erickt commented Jul 8, 2021

Enable Fuchsia safe command line tool analytics #82

Enable Fuchsia safe command line tool analytics #82

Conversation

BobEvans commented Mar 24, 2021

erickt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BobEvans commented Mar 25, 2021

BobEvans commented Apr 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erickt commented Jul 8, 2021