Skip to content

Commit a46ee9c

Browse files
duncanistazarirhamza
authored andcommitted
revert(traces): revert Datadog-Client-Computed-Stats header support (#1176)
- Reverts #1118 (`feat(traces): [SVLS-8734] respect Datadog-Client-Computed-Stats header`) which introduced logic to skip backend stats when the tracer claims client-computed-stats, causing stats to vanish entirely in some scenarios. - Reverts #1136 (`fix(tests): remove stale _dd.compute_stats:1 assertion from logs integration test`) which was a follow-up test adjustment for Restores the pre-v95 behavior where `_dd.compute_stats` is set solely by `DD_COMPUTE_TRACE_STATS_ON_EXTENSION`, keeping the backend as a safety net. - [x] `cargo fmt --check` passes - [x] `cargo clippy --all-targets -- -D warnings` passes - [x] `cargo test` — all 513 tests pass [SVLS-8734]: https://datadoghq.atlassian.net/browse/SVLS-8734?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
1 parent 168cc24 commit a46ee9c

File tree

7 files changed

+26
-161
lines changed

7 files changed

+26
-161
lines changed

bottlecap/src/lifecycle/invocation/context.rs

Lines changed: 1 addition & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -43,16 +43,6 @@ pub struct Context {
4343
/// tracing.
4444
///
4545
pub extracted_span_context: Option<SpanContext>,
46-
/// Whether the tracer has already generated stats for the dummy `aws.lambda` span
47-
/// for this invocation (which only exists for universal instrumentation
48-
/// languages).
49-
///
50-
/// Set from the `Datadog-Client-Computed-Stats` header when the tracer's
51-
/// dummy `aws.lambda` span (with `resource_name="dd-tracer-serverless-span"`) is received.
52-
/// The value will be propagated to the same field of the real `aws.lambda` span generated by
53-
/// the extension, which will be used to decide whether stats should be generated on extension
54-
/// or backend.
55-
pub client_computed_stats: bool,
5646
}
5747

5848
/// Struct containing the information needed to reparent a span.
@@ -104,7 +94,6 @@ impl Default for Context {
10494
snapstart_restore_span: None,
10595
tracer_span: None,
10696
extracted_span_context: None,
107-
client_computed_stats: false,
10897
}
10998
}
11099
}
@@ -519,12 +508,7 @@ impl ContextBuffer {
519508

520509
/// Adds the tracer span to a `Context` in the buffer.
521510
///
522-
pub fn add_tracer_span(
523-
&mut self,
524-
request_id: &String,
525-
tracer_span: &Span,
526-
client_computed_stats: bool,
527-
) {
511+
pub fn add_tracer_span(&mut self, request_id: &String, tracer_span: &Span) {
528512
if let Some(context) = self
529513
.buffer
530514
.iter_mut()
@@ -544,7 +528,6 @@ impl ContextBuffer {
544528
.extend(tracer_span.metrics.clone());
545529

546530
context.tracer_span = Some(tracer_span.clone());
547-
context.client_computed_stats = client_computed_stats;
548531
} else {
549532
debug!("Could not add tracer span - context not found");
550533
}

bottlecap/src/lifecycle/invocation/processor.rs

Lines changed: 6 additions & 107 deletions
Original file line numberDiff line numberDiff line change
@@ -653,16 +653,9 @@ impl Processor {
653653
trace_sender: &Arc<SendingTraceProcessor>,
654654
context: Context,
655655
) {
656-
let client_computed_stats = context.client_computed_stats;
657656
let (traces, body_size) = self.get_ctx_spans(context);
658-
self.send_spans(
659-
traces,
660-
body_size,
661-
client_computed_stats,
662-
tags_provider,
663-
trace_sender,
664-
)
665-
.await;
657+
self.send_spans(traces, body_size, tags_provider, trace_sender)
658+
.await;
666659
}
667660

668661
fn get_ctx_spans(&mut self, context: Context) -> (Vec<Span>, usize) {
@@ -727,7 +720,7 @@ impl Processor {
727720
let traces = vec![cold_start_span.clone()];
728721
let body_size = size_of_val(cold_start_span);
729722

730-
self.send_spans(traces, body_size, false, tags_provider, trace_sender)
723+
self.send_spans(traces, body_size, tags_provider, trace_sender)
731724
.await;
732725
}
733726
}
@@ -739,7 +732,6 @@ impl Processor {
739732
&mut self,
740733
traces: Vec<Span>,
741734
body_size: usize,
742-
client_computed_stats: bool,
743735
tags_provider: &Arc<provider::Provider>,
744736
trace_sender: &Arc<SendingTraceProcessor>,
745737
) {
@@ -752,7 +744,7 @@ impl Processor {
752744
tracer_version: "",
753745
container_id: "",
754746
client_computed_top_level: false,
755-
client_computed_stats,
747+
client_computed_stats: false,
756748
dropped_p0_traces: 0,
757749
dropped_p0_spans: 0,
758750
};
@@ -1399,10 +1391,9 @@ impl Processor {
13991391
///
14001392
/// This is used to enrich the invocation span with additional metadata from the tracers
14011393
/// top level span, since we discard the tracer span when we create the invocation span.
1402-
pub fn add_tracer_span(&mut self, span: &Span, client_computed_stats: bool) {
1394+
pub fn add_tracer_span(&mut self, span: &Span) {
14031395
if let Some(request_id) = span.meta.get("request_id") {
1404-
self.context_buffer
1405-
.add_tracer_span(request_id, span, client_computed_stats);
1396+
self.context_buffer.add_tracer_span(request_id, span);
14061397
}
14071398
}
14081399

@@ -2213,98 +2204,6 @@ mod tests {
22132204
);
22142205
}
22152206

2216-
/// Verifies that `client_computed_stats` set on a context via `add_tracer_span` is
2217-
/// propagated all the way through `send_ctx_spans` to the `aws.lambda` payload sent
2218-
/// to the backend, so the extension does not generate duplicate stats.
2219-
#[tokio::test]
2220-
#[allow(clippy::unwrap_used)]
2221-
async fn test_client_computed_stats_propagated_to_aws_lambda_span() {
2222-
use crate::traces::stats_concentrator_service::StatsConcentratorService;
2223-
use crate::traces::stats_generator::StatsGenerator;
2224-
use libdd_trace_obfuscation::obfuscation_config::ObfuscationConfig;
2225-
use tokio::sync::mpsc;
2226-
2227-
let config = Arc::new(config::Config {
2228-
apm_dd_url: "https://trace.agent.datadoghq.com".to_string(),
2229-
..config::Config::default()
2230-
});
2231-
let tags_provider = Arc::new(provider::Provider::new(
2232-
Arc::clone(&config),
2233-
LAMBDA_RUNTIME_SLUG.to_string(),
2234-
&HashMap::from([("function_arn".to_string(), "test-arn".to_string())]),
2235-
));
2236-
let aws_config = Arc::new(AwsConfig {
2237-
region: "us-east-1".into(),
2238-
aws_lwa_proxy_lambda_runtime_api: None,
2239-
function_name: "test-function".into(),
2240-
sandbox_init_time: Instant::now(),
2241-
runtime_api: "***".into(),
2242-
exec_wrapper: None,
2243-
initialization_type: "on-demand".into(),
2244-
});
2245-
let (aggregator_service, aggregator_handle) =
2246-
AggregatorService::new(EMPTY_TAGS, 1024).expect("failed to create aggregator service");
2247-
tokio::spawn(aggregator_service.run());
2248-
let propagator = Arc::new(DatadogCompositePropagator::new(Arc::clone(&config)));
2249-
let (durable_context_tx, _) = tokio::sync::mpsc::channel(1);
2250-
let mut p = Processor::new(
2251-
Arc::clone(&tags_provider),
2252-
Arc::clone(&config),
2253-
aws_config,
2254-
aggregator_handle,
2255-
propagator,
2256-
durable_context_tx,
2257-
);
2258-
2259-
let (trace_tx, mut trace_rx) = mpsc::channel(10);
2260-
let (stats_concentrator_service, stats_concentrator_handle) =
2261-
StatsConcentratorService::new(Arc::clone(&config));
2262-
tokio::spawn(stats_concentrator_service.run());
2263-
let trace_sender = Arc::new(SendingTraceProcessor {
2264-
appsec: None,
2265-
processor: Arc::new(trace_processor::ServerlessTraceProcessor {
2266-
obfuscation_config: Arc::new(
2267-
ObfuscationConfig::new().expect("Failed to create ObfuscationConfig"),
2268-
),
2269-
}),
2270-
trace_tx,
2271-
stats_generator: Arc::new(StatsGenerator::new(stats_concentrator_handle)),
2272-
});
2273-
2274-
let mut context = Context::from_request_id("req-1");
2275-
context.invocation_span.trace_id = 1;
2276-
context.invocation_span.span_id = 2;
2277-
context.client_computed_stats = true;
2278-
2279-
p.send_ctx_spans(&tags_provider, &trace_sender, context)
2280-
.await;
2281-
2282-
let payload = trace_rx
2283-
.recv()
2284-
.await
2285-
.expect("expected payload from trace_tx");
2286-
assert!(
2287-
payload.header_tags.client_computed_stats,
2288-
"client_computed_stats must be propagated to the aws.lambda span payload"
2289-
);
2290-
2291-
// Verify _dd.compute_stats is "0" in the built payload tags: client_computed_stats=true
2292-
// means the tracer has already computed stats, so neither extension nor backend should.
2293-
let send_data = payload.builder.build();
2294-
let libdd_trace_utils::tracer_payload::TracerPayloadCollection::V07(payloads) =
2295-
send_data.get_payloads()
2296-
else {
2297-
panic!("expected V07 payload");
2298-
};
2299-
for p in payloads {
2300-
assert_eq!(
2301-
p.tags.get(crate::tags::lambda::tags::COMPUTE_STATS_KEY),
2302-
Some(&"0".to_string()),
2303-
"_dd.compute_stats must be 0 when client_computed_stats is true"
2304-
);
2305-
}
2306-
}
2307-
23082207
fn make_trace_sender(config: Arc<config::Config>) -> Arc<SendingTraceProcessor> {
23092208
use libdd_trace_obfuscation::obfuscation_config::ObfuscationConfig;
23102209
let (stats_concentrator_service, stats_concentrator_handle) =

bottlecap/src/lifecycle/invocation/processor_service.rs

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -109,7 +109,6 @@ pub enum ProcessorCommand {
109109
},
110110
AddTracerSpan {
111111
span: Box<Span>,
112-
client_computed_stats: bool,
113112
},
114113
ForwardDurableContext {
115114
request_id: String,
@@ -378,12 +377,10 @@ impl InvocationProcessorHandle {
378377
pub async fn add_tracer_span(
379378
&self,
380379
span: Span,
381-
client_computed_stats: bool,
382380
) -> Result<(), mpsc::error::SendError<ProcessorCommand>> {
383381
self.sender
384382
.send(ProcessorCommand::AddTracerSpan {
385383
span: Box::new(span),
386-
client_computed_stats,
387384
})
388385
.await
389386
}
@@ -612,11 +609,8 @@ impl InvocationProcessorService {
612609
let result = Ok(self.processor.set_cold_start_span_trace_id(trace_id));
613610
let _ = response.send(result);
614611
}
615-
ProcessorCommand::AddTracerSpan {
616-
span,
617-
client_computed_stats,
618-
} => {
619-
self.processor.add_tracer_span(&span, client_computed_stats);
612+
ProcessorCommand::AddTracerSpan { span } => {
613+
self.processor.add_tracer_span(&span);
620614
}
621615
ProcessorCommand::ForwardDurableContext {
622616
request_id,

bottlecap/src/tags/lambda/tags.rs

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ const VERSION_KEY: &str = "version";
3939
const SERVICE_KEY: &str = "service";
4040

4141
// ComputeStatsKey is the tag key indicating whether trace stats should be computed
42-
pub(crate) const COMPUTE_STATS_KEY: &str = "_dd.compute_stats";
42+
const COMPUTE_STATS_KEY: &str = "_dd.compute_stats";
4343
// FunctionTagsKey is the tag key for a function's tags to be set on the top level tracepayload
4444
const FUNCTION_TAGS_KEY: &str = "_dd.tags.function";
4545
// TODO(astuyve) decide what to do with the version
@@ -135,6 +135,12 @@ fn tags_from_env(
135135
tags_map.extend(config.tags.clone());
136136
}
137137

138+
// The value of _dd.compute_stats is the opposite of config.compute_trace_stats_on_extension.
139+
// "config.compute_trace_stats_on_extension == true" means computing stats on the extension side,
140+
// so we set _dd.compute_stats to 0 so stats won't be computed on the backend side.
141+
let compute_stats = i32::from(!config.compute_trace_stats_on_extension);
142+
tags_map.insert(COMPUTE_STATS_KEY.to_string(), compute_stats.to_string());
143+
138144
tags_map
139145
}
140146

@@ -294,7 +300,8 @@ mod tests {
294300
fn test_new_from_config() {
295301
let metadata = HashMap::new();
296302
let tags = Lambda::new_from_config(Arc::new(Config::default()), &metadata);
297-
assert_eq!(tags.tags_map.len(), 2);
303+
assert_eq!(tags.tags_map.len(), 3);
304+
assert_eq!(tags.tags_map.get(COMPUTE_STATS_KEY).unwrap(), "1");
298305
let arch = arch_to_platform();
299306
assert_eq!(
300307
tags.tags_map.get(ARCHITECTURE_KEY).unwrap(),
@@ -429,7 +436,7 @@ mod tests {
429436
(parts[0].to_string(), parts[1].to_string())
430437
})
431438
.collect();
432-
assert_eq!(fn_tags_map.len(), 13);
439+
assert_eq!(fn_tags_map.len(), 14);
433440
assert_eq!(fn_tags_map.get("key1").unwrap(), "value1");
434441
assert_eq!(fn_tags_map.get("key2").unwrap(), "value2");
435442
assert_eq!(fn_tags_map.get(ACCOUNT_ID_KEY).unwrap(), "123456789012");
@@ -471,7 +478,7 @@ mod tests {
471478
(parts[0].to_string(), parts[1].to_string())
472479
})
473480
.collect();
474-
assert_eq!(fn_tags_map.len(), 13);
481+
assert_eq!(fn_tags_map.len(), 14);
475482
assert_eq!(fn_tags_map.get("key1").unwrap(), "value1");
476483
assert_eq!(fn_tags_map.get("key2").unwrap(), "value2");
477484
assert_eq!(fn_tags_map.get(ACCOUNT_ID_KEY).unwrap(), "123456789012");

bottlecap/src/traces/trace_agent.rs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -501,8 +501,7 @@ impl TraceAgent {
501501
);
502502
}
503503

504-
let tracer_header_tags: libdd_trace_utils::tracer_header_tags::TracerHeaderTags<'_> =
505-
(&parts.headers).into();
504+
let tracer_header_tags = (&parts.headers).into();
506505

507506
let (body_size, mut traces): (usize, Vec<Vec<pb::Span>>) = match version {
508507
ApiVersion::V04 => {
@@ -532,6 +531,7 @@ impl TraceAgent {
532531
}
533532
},
534533
};
534+
535535
let mut reparenting_info = match invocation_processor_handle.get_reparenting_info().await {
536536
Ok(info) => info,
537537
Err(e) => {
@@ -586,7 +586,7 @@ impl TraceAgent {
586586

587587
if span.resource == INVOCATION_SPAN_RESOURCE
588588
&& let Err(e) = invocation_processor_handle
589-
.add_tracer_span(span.clone(), tracer_header_tags.client_computed_stats)
589+
.add_tracer_span(span.clone())
590590
.await
591591
{
592592
error!("Failed to add tracer span to processor: {}", e);

bottlecap/src/traces/trace_processor.rs

Lines changed: 1 addition & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -368,19 +368,6 @@ impl TraceProcessor for ServerlessTraceProcessor {
368368
let tags = tags_provider.get_function_tags_map();
369369
for tracer_payload in collection.iter_mut() {
370370
tracer_payload.tags.extend(tags.clone());
371-
// Tell the backend whether to compute stats:
372-
// - "1" (compute on backend) if neither the tracer nor the extension is computing them
373-
// - "0" (skip on backend) if the extension or the tracer has already computed them
374-
let compute_stats = if !config.compute_trace_stats_on_extension
375-
&& !header_tags.client_computed_stats
376-
{
377-
"1"
378-
} else {
379-
"0"
380-
};
381-
tracer_payload
382-
.tags
383-
.insert(COMPUTE_STATS_KEY.to_string(), compute_stats.to_string());
384371
}
385372
}
386373
let endpoint = Endpoint {
@@ -534,7 +521,6 @@ impl SendingTraceProcessor {
534521
return Ok(());
535522
}
536523

537-
let client_computed_stats = header_tags.client_computed_stats;
538524
let (payload, processed_traces) = self.processor.process_traces(
539525
config.clone(),
540526
tags_provider,
@@ -546,9 +532,7 @@ impl SendingTraceProcessor {
546532

547533
// This needs to be after process_traces() because process_traces()
548534
// performs obfuscation, and we need to compute stats on the obfuscated traces.
549-
// Skip if the tracer has already computed stats (Datadog-Client-Computed-Stats header).
550535
if config.compute_trace_stats_on_extension
551-
&& !client_computed_stats
552536
&& let Err(err) = self.stats_generator.send(&processed_traces)
553537
{
554538
// Just log the error. We don't think trace stats are critical, so we don't want to
@@ -692,10 +676,6 @@ mod tests {
692676
);
693677
let tracer_payload = tracer_payload.expect("expected Some payload");
694678

695-
let mut expected_tags = tags_provider.get_function_tags_map();
696-
// process_traces always sets _dd.compute_stats:"1"
697-
// because compute_trace_stats_on_extension is false and client_computed_stats is false.
698-
expected_tags.insert(COMPUTE_STATS_KEY.to_string(), "1".to_string());
699679
let expected_tracer_payload = pb::TracerPayload {
700680
container_id: "33".to_string(),
701681
language_name: "nodejs".to_string(),
@@ -709,7 +689,7 @@ mod tests {
709689
tags: HashMap::new(),
710690
dropped_trace: false,
711691
}],
712-
tags: expected_tags,
692+
tags: tags_provider.get_function_tags_map(),
713693
env: "test-env".to_string(),
714694
hostname: String::new(),
715695
app_version: String::new(),

bottlecap/tests/logs_integration_test.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ async fn test_logs() {
2222
// protobuf is using hashmap, can't set a btreemap to have sorted keys. Using multiple regexp since
2323
// Can't do look around since -> error: look-around, including look-ahead and look-behind, is not supported
2424
let regexp_message = r#"[{"message":{"message":"START RequestId: 459921b5-681c-4a96-beb0-81e0aa586026 Version: $LATEST","lambda":{"arn":"test-arn","request_id":"459921b5-681c-4a96-beb0-81e0aa586026"},"timestamp":1666361103165,"status":"info"},"hostname":"test-arn","service":"","#;
25+
let regexp_compute_state = r#"_dd.compute_stats:1"#;
2526
let regexp_arch = format!(r#"architecture:{}"#, arch);
2627
let regexp_function_arn = r#"function_arn:test-arn"#;
2728
let regexp_extension_version = r#"dd_extension_version"#;
@@ -33,6 +34,7 @@ async fn test_logs() {
3334
.header("DD-API-KEY", dd_api_key)
3435
.header("Content-Type", "application/json")
3536
.body_contains(regexp_message)
37+
.body_contains(regexp_compute_state)
3638
.body_contains(regexp_arch)
3739
.body_contains(regexp_function_arn)
3840
.body_contains(regexp_extension_version);

0 commit comments

Comments
 (0)