remove invocation lock by jchrostek-dd · Pull Request #894 · DataDog/datadog-lambda-extension

jchrostek-dd · 2025-10-14T15:48:41Z

Task

https://datadoghq.atlassian.net/browse/SVLS-7465?atlOrigin=eyJpIjoiYzI2ZWQ3NmU1M2U4NGFiMGI1ZjE3MWY5MTM2MzlkN2MiLCJwIjoiaiJ9

Overview

Remove InvocationProcessor lock and replace with an aggregator service.

litianningdatadog · 2025-10-14T16:31:59Z

+    pub async fn get_reparenting_info(
+        &self,
+    ) -> Result<std::collections::VecDeque<ReparentingInfo>, ProcessorError> {
+        let (response_tx, response_rx) = oneshot::channel();
+
+        self.sender
+            .send(ProcessorCommand::GetReparentingInfo {
+                response: response_tx,
+            })
+            .map_err(|e| ProcessorError::ChannelSend(e.to_string()))?;
+
+        response_rx
+            .await
+            .map_err(|e| ProcessorError::ChannelReceive(e.to_string()))?
+    }
+
+    pub async fn update_reparenting(
+        &self,
+        reparenting_info: std::collections::VecDeque<ReparentingInfo>,
+    ) -> Result<Vec<crate::lifecycle::invocation::context::Context>, ProcessorError> {
+        let (response_tx, response_rx) = oneshot::channel();
+


would be great to refactor these.

what did you have in mind?

something like

async fn send_and_await<T, F>( &self, command_builder: F, ) -> Result<T, ProcessorError> where F: FnOnce(oneshot::Sender<Result<T, ProcessorError>>) -> ProcessorCommand, { let (response_tx, response_rx) = oneshot::channel(); self.sender .send(command_builder(response_tx)) .await .map_err(|e| ProcessorError::ChannelSend(e.to_string()))?; response_rx .await .map_err(|e| ProcessorError::ChannelReceive(e.to_string()))? } pub async fn get_reparenting_info( &self, ) -> Result<std::collections::VecDeque<ReparentingInfo>, ProcessorError> { self.send_and_await(|response| ProcessorCommand::GetReparentingInfo { response }) .await } pub async fn update_reparenting( &self, reparenting_info: std::collections::VecDeque<ReparentingInfo>, ) -> Result<Vec<Context>, ProcessorError> { self.send_and_await(|response| ProcessorCommand::UpdateReparenting { reparenting_info, response, }) .await }

Same for other places. WDYT?

litianningdatadog · 2025-10-14T16:32:44Z

+    ) -> Result<Option<u64>, ProcessorError> {
+        let (response_tx, response_rx) = oneshot::channel();
+
+        self.sender


litianningdatadog · 2025-10-14T16:46:19Z

+                        .on_platform_runtime_done(
+                            request_id.clone(),
+                            metrics,
+                            status,
+                            error_type.clone(),


if wait_for_platform_runtime_done_completion=false, cycle could be wasted due to async. Given that it is a more frequent scenario, would we consider to use async only if necessary?

What do you mean by cycle wasted due to async? If wait_for_platform_runtime_done_completion=false, then invocation_processor_handle.on_platform_runtime_done() should pretty much immediately return.

while it could return right away, since it is an async call, I would imagine it will incur some overhead which doesn't happen in non-async fashion.

litianningdatadog · 2025-10-14T16:56:08Z

+                } => {
+                    self.processor
+                        .send_ctx_spans(&tags_provider, &trace_sender, context)
+                        .await;


why do we wait here, unlike other processor calls?

oops, that is a typo, will remove 'await'

Actually .await is needed since send_ctx_spans() is async.

litianningdatadog

Left some comments

duncanista · 2025-10-14T17:58:31Z

+        metrics_aggregator_handle: AggregatorHandle,
+        propagator: Arc<DatadogCompositePropagator>,
+    ) -> (InvocationProcessorHandle, Self) {
+        let (sender, receiver) = mpsc::unbounded_channel();


Maybe we want to bound it to a 1000? @astuyve what would be a good number?

Yeah, good point.

duncanista · 2025-10-14T19:08:53Z

+            Err(e) => {
+                return error_response(
+                    StatusCode::INTERNAL_SERVER_ERROR,
+                    format!("Failed to get reparenting info: {e}"),
+                );
+            }
        };


Not sure if we should be erroring here, maybe just logging the error is enough? What happens if it failed before?

I guess this error is mostly when failing to send/receive from the channel?

duncanista · 2025-10-14T19:13:02Z

+        timestamp: i64,
+        wait_for_completion: bool,
+    ) -> Result<(), ProcessorError> {
+        // Only when Flush decision is 'End' do we want to wait to finish processing PlatformRuntimeDone


Otherwise aren't we going to start flushing events before potentially processing all of them?

duncanista

LGTM – the only comment is we shouldn't 500 when failing to get reparenting info

jchrostek-dd · 2025-10-21T17:44:27Z

/merge

dd-devflow-routing-codex · 2025-10-21T17:54:03Z

View all feedbacks in Devflow UI.

2025-10-21 17:44:31 UTC ℹ️ Start processing command /merge

2025-10-21 17:44:37 UTC ℹ️ MergeQueue: pull request added to the queue

The expected merge time in main is approximately 0s (p90).

2025-10-21 17:54:03 UTC ℹ️ MergeQueue: This merge request was merged

https://datadoghq.atlassian.net/browse/SVLS-7991 ## Overview PR #894 introduced an asynchronous message-passing architecture that changed the timing of CPU metrics collection. This timing difference exposed a pre-existing issue where per-CPU idle time measurements could exceed the wall-clock uptime delta, resulting in negative CPU utilization values being reported to customers. The root cause is a fundamental mismatch between measurement domains: - `/proc/uptime` measures wall-clock system uptime (single value) - `/proc/stat` measures per-CPU cumulative idle time (one value per core) When these measurements are taken at slightly different times (especially in the async processing model), the per-CPU idle time delta can exceed the uptime delta, causing the formula `((uptime - idle_time) / uptime) * 100` to produce negative results. ## Solution Implemented defensive validation and enforce in the CPU utilization calculation to ensure metrics are always within valid ranges: 1. **Uptime Validation**: Skip metrics if uptime delta is invalid (≤ 0) - Prevents division by zero - Catches timing anomalies early 2. **Per-CPU Idle Time Check**: Enforce each CPU's idle time to non-negative value - Handles cases where idle_time > uptime due to measurement timing - Handles negative idle_time from measurement errors 3. **Utilization Calculation**: Force all utilization values to be non-negative value PS: Since this is a complex issue without a definitive solution yet, this fix serves only as a temporary measure to unblock our release commitment. We’ll continue investigating a long-term solution as a follow-up. ## Test - Deployed the layer w/ the change and applied them to [these stacks](https://docs.google.com/spreadsheets/d/1oF60PBhYvwdOfFn6yz3zBUhZCZUP7Z43VVE6XE2QGWQ/edit?gid=0#gid=0) as they were the main contributors of the negative stats - Observe the stats and expect [no more negative stats](https://ddserverless.datadoghq.com/dashboard/35c-u5f-8jm?fromUser=true&fullscreen_end_ts=1763142342127&fullscreen_paused=false&fullscreen_refresh_mode=sliding&fullscreen_section=overview&fullscreen_start_ts=1763138742127&fullscreen_widget=2129549723517146&refresh_mode=paused&tpl_var_runtime%5B0%5D=python3.10&tpl_var_runtime%5B1%5D=dotnet8&tpl_var_runtime%5B2%5D=java11&tpl_var_runtime%5B3%5D=nodejs20.x&tpl_var_runtime%5B4%5D=python3.13&tpl_var_runtime%5B5%5D=ruby3.2&tpl_var_service%5B0%5D=d1&from_ts=1763136901167&to_ts=1763137625000&live=false) are reported <img width="2262" height="1698" alt="image" src="https://github.com/user-attachments/assets/a4482590-64ac-4322-8169-103f82706ddf" />

remove invocation lock

38fa4b3

jchrostek-dd requested a review from a team as a code owner October 14, 2025 15:48

litianningdatadog reviewed Oct 14, 2025

View reviewed changes

duncanista reviewed Oct 14, 2025

View reviewed changes

jchrostek-dd added 2 commits October 15, 2025 15:29

change to bounded channel, block on runtime report done

4d4d23a

format

8487e40

duncanista approved these changes Oct 20, 2025

View reviewed changes

log error instead of returning

703eaf6

dd-devflow Bot added mergequeue-status: queued mergequeue-status: in_progress and removed mergequeue-status: queued labels Oct 21, 2025

dd-mergequeue Bot merged commit c5ee4fc into main Oct 21, 2025
22 of 31 checks passed

dd-devflow Bot removed the mergequeue-status: in_progress label Oct 21, 2025

dd-mergequeue Bot deleted the john/remove-invocation-processor-lock branch October 21, 2025 17:54

dd-devflow Bot added the mergequeue-status: done label Oct 21, 2025

litianningdatadog mentioned this pull request Nov 14, 2025

fix: Guard against negative CPU utilization metrics #929

Merged

Uh oh!

Conversation

jchrostek-dd commented Oct 14, 2025

Task

Overview

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

litianningdatadog Oct 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

litianningdatadog left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

duncanista left a comment

Choose a reason for hiding this comment

Uh oh!

jchrostek-dd commented Oct 21, 2025

Uh oh!

Uh oh!

dd-devflow-routing-codex Bot commented Oct 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

litianningdatadog Oct 21, 2025 •

edited

Loading

dd-devflow-routing-codex Bot commented Oct 21, 2025 •

edited

Loading