Skip to content

feat(cli): cli-telemetry status command #697

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

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

kaizencc
Copy link
Contributor

@kaizencc kaizencc commented Jul 8, 2025


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@aws-cdk-automation aws-cdk-automation requested a review from a team July 8, 2025 20:08
@github-actions github-actions bot added the p2 label Jul 8, 2025
@kaizencc kaizencc temporarily deployed to integ-approval July 8, 2025 20:25 — with GitHub Actions Inactive
/**
* Whether or not we collect telemetry
*/
export function canCollectTelemetry(context: Context): boolean {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@iliapolo I pulled out this function from #631 and removed our own feature flag CLI_TELEMETRY=true because it misrepresents whether our users have enabled/disabled telemetry.

Signed-off-by: github-actions <[email protected]>
@codecov-commenter
Copy link

codecov-commenter commented Jul 8, 2025

Codecov Report

Attention: Patch coverage is 80.00000% with 6 lines in your changes missing coverage. Please review.

Project coverage is 80.06%. Comparing base (9edad83) to head (750b548).

Files with missing lines Patch % Lines
packages/aws-cdk/lib/cli/cli.ts 25.00% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #697      +/-   ##
==========================================
- Coverage   80.13%   80.06%   -0.07%     
==========================================
  Files          49       50       +1     
  Lines        7358     7383      +25     
  Branches      827      828       +1     
==========================================
+ Hits         5896     5911      +15     
- Misses       1439     1453      +14     
+ Partials       23       19       -4     
Flag Coverage Δ
suite.unit 80.06% <80.00%> (-0.07%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

integTest(
'CLI Telemetry reports status',
withDefaultFixture(async (fixture) => {
const contextFile = path.join(fixture.integTestDir, 'cdk.context.json');
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the impact of this file on the test?

### `cdk cli-telemetry`

Enables or disables cli telemetry collection for your local CDK App. Records your
choice in `cdk.context.json`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can I also set it manually in the context property of cdk.json on the project level?


You can also check the current status on whether your CDK App is opted in or out of
cli telemetry collection. Note that this takes into account all methods of disabling
cli telemetry, including environment variables and preferences set globally in `~/.cdk.json`.
Copy link
Contributor

Choose a reason for hiding this comment

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

And context in <app-root>/cdk.json?

@@ -206,6 +207,15 @@ export class CdkToolkit {
await this.props.configuration.saveContext();
}

public async cliTelemetryStatus() {
const currentStatus = canCollectTelemetry(this.props.configuration.context);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const currentStatus = canCollectTelemetry(this.props.configuration.context);
const canCollect = canCollectTelemetry(this.props.configuration.context);

Makes the next if easier to read.

public async cliTelemetryStatus() {
const currentStatus = canCollectTelemetry(this.props.configuration.context);
if (currentStatus) {
await this.ioHost.asIoHelper().defaults.info('CLI Telemetry is enabled. Run \'cdk cli-telemetry --disable\' to disable for this CDK App.');
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe link to the README so that the user is also exposed to the other mechanisms?

});

test('returns false if env variable is set', async () => {
process.env.DISABLE_CLI_TELEMETRY = 'true';
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be in a finally block that reverts back to the original value (see if we already have a withEnv or something).


// THEN
expect(info).toHaveBeenCalledWith('CLI Telemetry is disabled. Run \'cdk cli-telemetry --enable\' to enable for this CDK App.');
process.env.CDK_CLI_DISABLE_TELEMETRY = CDK_CLI_DISABLE_TELEMETRY;
Copy link
Contributor

Choose a reason for hiding this comment

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

Put in finally

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants