search

acquia / cli

Command-line interface for Acquia Cloud Platform products
https://docs.acquia.com/acquia-cli/
GNU General Public License v2.0
42 stars 45 forks source link

Track environment provider in telemetry. #1741

Closed grasmash closed 1 month ago

grasmash commented 1 month ago

Motivation

We often don't know if ACLI is running in Code Studio, on local, or elsewhere.

Proposed changes

Add an "env_provider" data point to telemetry, based on env variables, to determine env provider.

codecov[bot] commented 1 month ago

Codecov Report

Attention: Patch coverage is 96.87500% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 91.73%. Comparing base (50fdfff) to head (33e55c8).

Files Patch % Lines
src/Helpers/TelemetryHelper.php 96.87% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1741 +/- ## ============================================ + Coverage 91.70% 91.73% +0.02% - Complexity 1805 1810 +5 ============================================ Files 121 121 Lines 6474 6506 +32 ============================================ + Hits 5937 5968 +31 - Misses 537 538 +1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

github-actions[bot] commented 1 month ago

Try the dev build for this PR: https://acquia-cli.s3.amazonaws.com/build/pr/1741/acli.phar

curl -OL https://acquia-cli.s3.amazonaws.com/build/pr/1741/acli.phar
chmod +x acli.phar
grasmash commented 1 month ago

Looks like there are no automated tests for the TelemetryHelper at present. Also, I don't see how Amplitude ever gets initialized in local environments, because it only gets initialized when AMPLITUDE_KEY is set via an environmental variable. How does this work for Lando and DDEV usage?

Other way to implement this would be to add it to event properties rather than user properties, like here: https://github.com/acquia/cli/blob/50fdfffb8b65d9d308fdd7a1d25ec009ff0bbc38/src/Command/CommandBase.php#L254

But, we're already tracking ah_env as a user property. Not sure why that's not part of the event properties instead. Probably not great to change now because it would mess up historical data trends.

grasmash commented 1 month ago

Ok, not sure why the mutation testing is failing. I tried to appease it, but to no avail.