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

aws-sam-cli: disable telemetry #79478

Merged
merged 2 commits into from Feb 12, 2020
Merged

aws-sam-cli: disable telemetry #79478

merged 2 commits into from Feb 12, 2020

Conversation

@stefano-m
Copy link
Contributor

@stefano-m stefano-m commented Feb 7, 2020

Motivation for this change

Since v0.19.0 aws-sam-cli sends telemetry data to AWS[1]. To protect the users'
privacy, we opt-out by default.

[1] aws/aws-sam-cli#1272

Cc maintainers @andreabedini @dhl

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Since v0.19.0 aws-sam-cli sends telemetry data to AWS[1]. To protect the users'
privacy, we opt-out by default.

[1] aws/aws-sam-cli#1272
@stefano-m
Copy link
Contributor Author

@stefano-m stefano-m commented Feb 7, 2020

Example runs:

Telemetry enabled

Output edited to improve readability.

Note that they assign unique values to:

  • requestId
  • installationId
  • sessionId
 ./result-with-telemetry/bin/sam init --debug
Telemetry endpoint configured to be https://aws-serverless-tools-telemetry.us-west-2.amazonaws.com/metrics
Which template source would you like to use?
	1 - AWS Quick Start Templates
	2 - Custom Template Location
Choice: ^C
Sending Telemetry: {'metrics': 
 [
     {'commandRun': 
      {'awsProfileProvided': False, 
       'debugFlagProvided': True, 
       'region': '',
       'commandName': 'sam init', 
       'duration': 6709, 
       'exitReason': 'Abort', 
       'exitCode': 255, 
       'requestId': '<REDACTED>',
       'installationId': '<REDACTED>',
       'sessionId': '<REDACTED>',
       'executionEnvironment': 'CLI', 
       'pyversion': '3.7.6',
       'samcliVersion': '0.40.0'}
     }
 ]
}
HTTPSConnectionPool(host='aws-serverless-tools-telemetry.us-west-2.amazonaws.com', port=443): Read timed out. (read timeout=0.1)
Aborted!

Telemetry disabled

 ./result-no-telemetry/bin/sam init --debug
Which template source would you like to use?
	1 - AWS Quick Start Templates
	2 - Custom Template Location
Choice: ^C
Aborted!
@stefano-m
Copy link
Contributor Author

@stefano-m stefano-m commented Feb 7, 2020

Also note that the installation ID and telemetry options are saved in a configuration file (although the env var takes precedence):

cat ~/.aws-sam/metadata.json

{
    "telemetryEnabled": true,
    "installationId": "<REDACTED>"
}
@andreabedini
Copy link
Contributor

@andreabedini andreabedini commented Feb 7, 2020

LGTM

@dhl
Copy link
Contributor

@dhl dhl commented Feb 8, 2020

This is awesome. Thanks for this PR!

@dhl
dhl approved these changes Feb 8, 2020
@stefano-m
Copy link
Contributor Author

@stefano-m stefano-m commented Feb 10, 2020

Anything else you need from me to get this merged, please do let me know.

Thanks for the awesome work you are doing on NixOS!

@dhl
Copy link
Contributor

@dhl dhl commented Feb 11, 2020

Hey @stefano-m, sorry for the delay in getting back to you.

Personally, I am okay with disabling telemetry by default (and thanks for being privacy-conscious!). I gave this a bit more thought and felt that by generating a wrapper around sam, it actually removes user's ability to opt into using telemetry, if they need that for whatever reason.

Can we generate the wrapper conditionally, so user's can actually override the default and opt in?

@stefano-m
Copy link
Contributor Author

@stefano-m stefano-m commented Feb 11, 2020

@dhl I can see your point, although I fail to understand why someone wanted to use telemetry (aws-sam-cli even disable it in their tests).

Note that for example powershell disables telemetry in the wrapper

--set TERM xterm --set POWERSHELL_TELEMETRY_OPTOUT 1 --set DOTNET_CLI_TELEMETRY_OPTOUT 1

(Disclosure: I somewhat contributed to those setting with #74516)

If you still prefer to have the option to enable telemetry, I would propose instead that we generate the wrapper with telemetry disabled by default and add a conditional parameter to the derivation to explicitly enable telemetry.

Would you be OK with that?

Thanks

@stefano-m
Copy link
Contributor Author

@stefano-m stefano-m commented Feb 11, 2020

I have just re-read your comment and actually you say

Can we generate the wrapper conditionally, so user's can actually override the default and opt in?

🤦‍♂️ ...

So, yes... I will do that. 🙇‍♂️

If someone really wants to opt into telemetry, they can do so by setting
`enableTelemetry` to `true` (the default is `false`), in which case the wrapper
that sets `SAM_CLI_TELEMETRY` to `0` will not be created.

Note that this actually allows a user to optionally disable telemetry from the
command line or the (poorly documented) configuration in
`~/.aws-sam/metadata.json`. The downside is telemetry will be enabled at least
on the first run, causing a unique installation ID to be saved in the
configuration file.
@ofborg ofborg bot requested a review from dhl Feb 11, 2020
@dhl
Copy link
Contributor

@dhl dhl commented Feb 12, 2020

@GrahamcOfBorg build aws-sam-cli

@dhl
dhl approved these changes Feb 12, 2020
pkgs/development/tools/aws-sam-cli/default.nix Outdated Show resolved Hide resolved
@rnhmjoj rnhmjoj merged commit c6ec7b6 into NixOS:master Feb 12, 2020
15 checks passed
15 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
aws-sam-cli on aarch64-linux Success
Details
aws-sam-cli on x86_64-linux Success
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.