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

feat(windows-installer): add client telemetry opt out option #210

Merged
merged 2 commits into from
Mar 13, 2024

Conversation

jericht
Copy link
Contributor

@jericht jericht commented Mar 13, 2024

What was the problem/requirement? (What/Why)

The Windows installer is missing the option to opt out of client telemetry. The Linux installer has this option, so we need to add it to the Windows one as well.

What was the solution? (How)

Add option to opt out of client telemetry --telemetry-opt-out. When this option is provided, the installer will use the Deadline Client's config API to update the telemetry.opt_out setting to true.

What is the impact of this change?

Users can opt-out of client telemetry through the Windows installer

How was this change tested?

  • Manually tested the installer and verified the Deadline Client config file was updated with the telemetry opt out option.
  • Ran the worker agent and ran a job to ensure the file being written by Administrator (a "not the worker" user) did not cause issues with the worker agent
  • Added and ran integration tests

Installer Output

Note: The icacls output between the opting out of client telemetry is output by deadline-cloud and will be removed in a future release of deadline-cloud.

> install-deadline-worker --farm-id $env:FARM_ID --fleet-id $env:FLEET_ID --region us-west-2 --telemetry-opt-out -y
===========================================================
|      Amazon Deadline Cloud Worker Agent Installer       |
===========================================================

Farm ID: farm-9476acb9a64443e580089ce53c8dd2a4
Fleet ID: fleet-6e184b9bea1647649246a76619951f45
Region: us-west-2
Worker agent user: deadline-worker
Worker job group: deadline-job-users
Worker agent program path: C:\Program Files\Python312\Scripts
Allow worker agent shutdown: False
Start service: False
Telemetry opt-out: True
INFO: Agent User deadline-worker already exists
INFO: Group deadline-job-users already exists
INFO: User deadline-worker is already a member of group deadline-job-users.
INFO: Provisioning root directory (C:\ProgramData\Amazon\Deadline)
INFO: Done provisioning root directory (C:\ProgramData\Amazon\Deadline)
INFO: Provisioning log directory (C:\ProgramData\Amazon\Deadline\Logs)
INFO: Done provisioning log directory (C:\ProgramData\Amazon\Deadline\Logs)
INFO: Provisioning persistence directory (C:\ProgramData\Amazon\Deadline\Cache)
INFO: Done provisioning persistence directory (C:\ProgramData\Amazon\Deadline\Cache)
INFO: Provisioning config directory (C:\ProgramData\Amazon\Deadline\Config)
INFO: Done provisioning config directory (C:\ProgramData\Amazon\Deadline\Config)
INFO: Updating configuration file
INFO: Done configuring ['farm_id', 'fleet_id', 'shutdown_on_stop'] in C:\ProgramData\Amazon\Deadline\Config\worker.toml
INFO: Opting out of client telemetry
processed file: C:\Users\deadline-worker\.deadline
Successfully processed 1 files; Failed processing 0 files
processed file: C:\Users\deadline-worker\.deadline
Successfully processed 1 files; Failed processing 0 files
processed file: C:\Users\deadline-worker\.deadline
Successfully processed 1 files; Failed processing 0 files
processed file: C:\Users\deadline-worker\.deadline
Successfully processed 1 files; Failed processing 0 files
INFO: Opted out of client telemetry

Deadline Client config file

> type C:\Users\deadline-worker\.deadline\config
[telemetry]
opt_out = true

Was this change documented?

No

Is this a breaking change?

No

@jericht jericht marked this pull request as ready for review March 13, 2024 18:28
@jericht jericht requested a review from a team as a code owner March 13, 2024 18:28
@jericht jericht force-pushed the jericht/winstaller_tele branch 3 times, most recently from 4ce26fc to b757b26 Compare March 13, 2024 19:01
Signed-off-by: Jericho Tolentino <68654047+jericht@users.noreply.github.com>
gmchale79
gmchale79 previously approved these changes Mar 13, 2024
deadline_client_config_path = deadline.client.config.config_file.CONFIG_FILE_PATH
if not deadline_client_config_path.startswith("~"):
raise InstallerFailedException(
f"Cannot opt out of telemtry: Expected Deadline client config file path to start with a tilde (~), but got: {deadline_client_config_path}\n"
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: telemtrytelemetry

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, fixed

Copy link
Contributor

@jusiskin jusiskin left a comment

Choose a reason for hiding this comment

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

Looks good! Just a spelling mistake and we need to add the else case.

Comment on lines +535 to +541
if telemetry_opt_out:
logging.info("Opting out of client telemetry")
update_deadline_client_config(
user=user_name,
settings={"telemetry.opt_out": "true"},
)
logging.info("Opted out of client telemetry")
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe we need an else which should set telemetry.opt_out to `false.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this makes sense, but this is not the same behavior as the Linux installer: https://github.com/casillas2/deadline-cloud-worker-agent/blob/58ce63d150d4df8229a7f418e8731a9f11411f23/src/deadline_worker_agent/installer/install.sh#L446-L450

Should I update the Linux installer to do the same? (If the user does not pass any --telemetry-opt-out, then we silently opt them in?)

Copy link
Contributor

@jusiskin jusiskin Mar 13, 2024

Choose a reason for hiding this comment

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

I suppose another alternative is that we:

  • add another argument --telemetry-opt-in which is mutually-exclusive with --telemetry-opt-out
  • when the installer detects a pre-existing config file, we'd not modify anything unless one of the arguments are passed

This makes re-installation behavior explicit and allows keeping the prior telemetry setting.

Maybe we should follow the Linux behavior for now and move this out-of-scope for the current PR.

Signed-off-by: Jericho Tolentino <68654047+jericht@users.noreply.github.com>
@jericht jericht merged commit 7551869 into mainline Mar 13, 2024
12 checks passed
@jericht jericht deleted the jericht/winstaller_tele branch March 13, 2024 22:22
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