fix: hotswap is not discoverable via telemetry#1230
fix: hotswap is not discoverable via telemetry#1230aws-cdk-automation merged 11 commits intomainfrom
Conversation
Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1230 +/- ##
==========================================
+ Coverage 87.94% 87.97% +0.02%
==========================================
Files 74 74
Lines 10339 10363 +24
Branches 1377 1384 +7
==========================================
+ Hits 9093 9117 +24
Misses 1220 1220
Partials 26 26
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:
|
| } | ||
| } | ||
|
|
||
| function hotswapToEventResult(result: HotswapResult): TelemetryEvent { |
There was a problem hiding this comment.
callout in case you disagree:
a hotswap event that results in hotswapped: false (as in, it falls back to regular OR does nothing), will result in state: SUCCEEDED. my opinion is that state is for situations where errors are reported. The way we will differentiate hotswap happening or not is the hotswap counter.
There was a problem hiding this comment.
This sounds reasonable to me, since the deployment technically did not fail.
ShadowCat567
left a comment
There was a problem hiding this comment.
I have a question about hotswap event registration to make sure I am understanding how it works.
| } | ||
| } | ||
|
|
||
| function hotswapToEventResult(result: HotswapResult): TelemetryEvent { |
There was a problem hiding this comment.
This sounds reasonable to me, since the deployment technically did not fail.
| stack, | ||
| mode: hotswapMode, | ||
| hotswapped: true, | ||
| hotswapped: !error, |
There was a problem hiding this comment.
This is a bit confusing to me.
From what I understand, hotswapped: true means that we successfully executed hotswap. However hotswap: false and error being defined means we tried to hotswap but ran into some SDK issue so hotswap deployment was failed and hotswap: false and error: undefined means we did some fallback behavior.
So does that mean is hotswap set to false by default? And the only way it can get set to true is if it passes through here?
There was a problem hiding this comment.
hotswap behavior when nonHotswappableChanges are detected is either 1) fall back to full CFN deployment or 2) ignore nonHotswappableChanges and hotswap what can be hotswapped.
Behavior 1) happens before this line:
if (hotswapMode === 'fall-back') {
if (nonHotswappableChanges.length > 0) {
return {
stack,
mode: hotswapMode,
hotswapped: false,
hotswappableChanges,
nonHotswappableChanges,
};
}
}So this is behavior 2). Previously, if an SDK error is encountered we do not catch and actually this span never ends. This now matters because the end command is what sends telemetry. So I have decided that in this scenario, we say hotswapped: false because although we attempted hotswap, it was unsuccessful. If there's no error, we return hotswapped: true like before.
All that is to say, I am preserving old behavior and also your explanation of what is happening is also right.
So does that mean is hotswap set to false by default? And the only way it can get set to true is if it passes through here?
Yes. the only way we set hotswap: true is if applyAllHotswapOperations is invoked and does not return an error
Adds a new
HOTSWAPtelemetry event type that fires when hotswap deployments are attempted, tracking success/failure state and resource change counts. The motivation behind this is to track hotswap efficiency.For example,
{ "event": { "state": "FAILED", "eventType": "HOTSWAP", "duration": 456, "error": { "name": "UnknownError" }, "counters": { "hotswapped": 0, "hotswappableChanges": 1, "nonHotswappableChanges": 0 } } }By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license