feat: CLI send in performance profile if emitted by app#1478
Conversation
This adds a new feature where the CDK app can emit a performance profile to a well-known file, and the CLI will ask the user whether to send it in as part of telemetry or not. The question will be posed as: ``` App synthesis produced a performance profile. Send in for analysis? (Y)es (N)o (A)lways N(E)ver ``` And the user can answer "Never" to permanently record that they never want to do this.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1478 +/- ##
==========================================
+ Coverage 88.10% 88.11% +0.01%
==========================================
Files 74 75 +1
Lines 10669 10721 +52
Branches 1457 1464 +7
==========================================
+ Hits 9400 9447 +47
- Misses 1241 1246 +5
Partials 28 28
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| // Ask the user | ||
| let decision: boolean | undefined = undefined; | ||
| while (decision === undefined) { | ||
| const answer: Choice = await promptly.prompt(`🔍 ${chalk.cyan('Your CDK app produced a performance profile to help AWS diagnose slow synthesis. Send in for analysis?')} (Y)es (N)o (A)lways Neve(R) (V)iew`, { |
There was a problem hiding this comment.
Why not use IoHelper here?
| /** | ||
| * If present, will be sent in every event | ||
| */ | ||
| permanentCounters?: Record<string, number>; |
There was a problem hiding this comment.
How is this supposed to be used?
| return await main(cmd, argv); | ||
| const ret = await main(cmd, argv); | ||
|
|
||
| await maybeCommitPerfCounters(ioHost); |
There was a problem hiding this comment.
Do we care about performance metrics only in case of success?
mrgrain
left a comment
mrgrain
left a comment
There was a problem hiding this comment.
Main blocker for me is that when asking for "always" approve, adding to the data might be an issue. We either need to make this abundantly clear on the opt-in, or version the data schema and store the opt-in only per schema version.
Similarly, if we phrase this exclusively as performance data, we cannot reuse the same opt-in for e.g. error logs in future. This might be by design, but wanted to call this out.
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
mrgrain
left a comment
mrgrain
left a comment
There was a problem hiding this comment.
Looks good. Could make a note somewhere that performance-counters.json is an inherently temporary file.
5 participants
If the CDK app emits a performance profile (a set of counters explaining where time was spent in the CDK app), the CLI will attach those to the SYNTH event.
The data itself depends on what the CDK app is going to emit (not fully written yet), but is going to look something like this (feature added here):
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license