[SVLS-8230] Fix SnapStart cold_start tag using restore_time#1139
Conversation
a128599 to
cd3f0af
Compare
415f7e7 to
1a7ce63
Compare
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug in SnapStart cold_start tag detection by using restore_time instead of sandbox_init_time to measure the time since the restore event for cold_start classification. Previously, SnapStart restore invocations were misclassified as proactive_initialization=true, cold_start=false.
Changes:
- Added
is_snapstart()method toAwsConfigto check if the function uses SnapStart initialization - Added
restore_time: Option<Instant>field toProcessorthat tracks when the SnapStart restore event occurs - Modified cold_start detection logic in
set_init_tags()to measure time since restore for SnapStart functions instead of time since sandbox initialization - Added integration test assertions for SnapStart cold_start tag values for both restore and warm invocations
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| bottlecap/src/config/aws.rs | Added is_snapstart() method to detect SnapStart functions |
| bottlecap/src/lifecycle/invocation/processor.rs | Added restore_time tracking and SnapStart-aware cold start detection logic |
| integration-tests/tests/snapstart.test.ts | Added assertions for correct cold_start tag values on restore and warm invocations |
| if self.aws_config.is_snapstart() { | ||
| match self.restore_time { | ||
| None => { | ||
| // PlatformRestoreStart hasn't arrived yet — restore and invoke | ||
| // happened close together, so this is a cold start (not proactive). | ||
| cold_start = true; | ||
| } | ||
| Some(restore_time) => { | ||
| let now = Instant::now(); | ||
| let time_since_restore = now.duration_since(restore_time); | ||
| if time_since_restore.as_millis() | ||
| > PROACTIVE_INITIALIZATION_THRESHOLD_MS.into() | ||
| { | ||
| proactive_initialization = true; | ||
| } else { | ||
| cold_start = true; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The SnapStart cold_start detection logic incorrectly tags warm invocations as cold starts. On the first SnapStart invocation, restore_time is set and time since restore is near 0ms, correctly setting cold_start=true. However, on subsequent warm invocations on the same container (which reuse the same restore_time value), the time since restore keeps increasing. For warm invocations that occur within 10 seconds of the restore, this causes them to incorrectly have cold_start=true instead of cold_start=false.
The issue is that restore_time should be cleared or the logic should distinguish between the first invocation and subsequent warm invocations. Consider checking if this is the first time set_init_tags() is being called using a flag or by detecting context transitions.
There was a problem hiding this comment.
This is inaccurate since we first check 'context_buffer.is_empty()'
| @@ -252,12 +255,35 @@ impl Processor { | |||
|
|
|||
| // If it's empty, then we are in a cold start | |||
| let now = Instant::now(); | ||
| let time_since_restore = now.duration_since(restore_time); | ||
| if time_since_restore.as_millis() | ||
| > PROACTIVE_INITIALIZATION_THRESHOLD_MS.into() |
There was a problem hiding this comment.
What does this const mean? Where does the value come from? Can we add some comment?
There was a problem hiding this comment.
Added a comment to PROACTIVE_INITIALIZATION_THRESHOLD_MS.
1a7ce63 to
8b6704c
Compare
SnapStart restore invocations were misclassified as proactive_initialization because sandbox_init_time (from snapshot creation) always exceeded the 10s threshold. Fix by tracking restore_time from PlatformRestoreStart telemetry and using it for proactive init detection in SnapStart functions. When restore_time is None (telemetry not yet delivered), assume cold start since the restore and invoke happened close together. https://datadoghq.atlassian.net/browse/SVLS-8230
8b6704c to
580d5c2
Compare
What does this PR do?
SnapStart restore invocations were being misclassified as
proactive_initialization=true, cold_start=false. The root cause is inset_init_tags(): it usessandbox_init_time(captured at snapshot creation) to measure how long ago the sandbox was initialized. For SnapStart, this gap is always large (snapshot creation vs. restore), so the >10s threshold is always exceeded.Fix: Add a
restore_timefield toProcessor, set from the existingPlatformRestoreStarttelemetry event. For SnapStart functions,set_init_tags()now measures time since the restore — not since snapshot creation.Changes
bottlecap/src/config/aws.rs— Addedis_snapstart()method (mirrorsis_managed_instance_mode()pattern)bottlecap/src/lifecycle/invocation/processor.rs— Addedrestore_time: Option<Instant>, set inon_platform_restore_start(), used inset_init_tags()for SnapStart-aware cold start detectionintegration-tests/tests/snapstart.test.ts— Addedcold_start=trueassertion for restore invocations andcold_start=falsefor warm invocations (Java + .NET, 2 threads × 2 invocations each with 5s delay)Scope
Tests
Integration test suite:
integration-tests/tests/snapstart.test.tsAssertions added:
cold_start=true,init_type=snap-start,snapstart_restorespan present, nocold_startspancold_start=false,init_type=snap-start, nosnapstart_restorespanJira
SVLS-8230