Skip to content

Commit b1d5bcf

Browse files
VianneyRuhlmannVianneyRuhlmann
authored
fix(stats): align with css spec (#1790)
The CSS implementation has several discrepancies with the [CSS spec](https://datadoghq.atlassian.net/wiki/spaces/APM/pages/6378947571/Client-Side+Stats+v1.3.0) this PR fixes some of them: Related to [peer tags section](https://datadoghq.atlassian.net/wiki/spaces/APM/pages/6378947571/Client-Side+Stats+v1.3.0#Peer-Tags-in-Aggregation) - consumer span kind was not using peer tags - internal spans were not using base_service for peer tags Related to [payload section](https://datadoghq.atlassian.net/wiki/spaces/APM/pages/6378947571/Client-Side+Stats+v1.3.0#Deployment-Level-Characteristics) - the service field on the ClientStatsPayload was left empty - empty env was using "" instead of "unknown-env" - lang and tracer version were not left empty for the agent to populate # Motivation Libdatadog should be align with the css spec # Additional Notes Anything else we should know when reviewing? # How to test the change? Describe here in detail how the change can be validated. Co-authored-by: vianney.ruhlmann <[email protected]>
1 parent db9b9f4 commit b1d5bcf

File tree

3 files changed

+207
-9
lines changed

3 files changed

+207
-9
lines changed

‎libdd-data-pipeline/src/stats_exporter.rs‎

Lines changed: 33 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -159,22 +159,26 @@ fn encode_stats_payload(
159159
) -> pb::ClientStatsPayload {
160160
pb::ClientStatsPayload {
161161
hostname: meta.hostname.clone(),
162-
env: meta.env.clone(),
163-
lang: meta.language.clone(),
162+
env: if meta.env.is_empty() {
163+
"unknown-env".to_string()
164+
} else {
165+
meta.env.clone()
166+
},
164167
version: meta.app_version.clone(),
165168
runtime_id: meta.runtime_id.clone(),
166-
tracer_version: meta.tracer_version.clone(),
167169
sequence,
170+
service: meta.service.clone(),
168171
stats: buckets,
169172
git_commit_sha: meta.git_commit_sha.clone(),
170173
process_tags: meta.process_tags.clone(),
171-
// These fields are unused or will be set by the Agent
172-
service: String::new(),
174+
// These fields will be set by the Agent
173175
container_id: String::new(),
174176
tags: Vec::new(),
175177
agent_aggregation: String::new(),
176178
image_tag: String::new(),
177179
process_tags_hash: 0,
180+
lang: String::new(),
181+
tracer_version: String::new(),
178182
}
179183
}
180184

@@ -387,4 +391,28 @@ mod tests {
387391
"Expected max retry attempts"
388392
);
389393
}
394+
395+
#[test]
396+
fn test_encode_stats_payload_defaults_empty_env() {
397+
// Test that empty env defaults to "unknown-env"
398+
let mut meta_with_empty_env = get_test_metadata();
399+
meta_with_empty_env.env = "".to_string();
400+
401+
let buckets = vec![];
402+
let payload = encode_stats_payload(&meta_with_empty_env, 1, buckets.clone());
403+
404+
assert_eq!(
405+
payload.env, "unknown-env",
406+
"Empty env should default to 'unknown-env'"
407+
);
408+
409+
// Test that non-empty env is preserved
410+
let meta_with_env = get_test_metadata();
411+
let payload_with_env = encode_stats_payload(&meta_with_env, 2, buckets);
412+
413+
assert_eq!(
414+
payload_with_env.env, "test",
415+
"Non-empty env should be preserved"
416+
);
417+
}
390418
}

‎libdd-trace-stats/src/span_concentrator/aggregation.rs‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -206,13 +206,16 @@ impl<'a> BorrowedAggregationKey<'a> {
206206
/// key.
207207
pub(super) fn from_span<T: StatSpan<'a>>(span: &'a T, peer_tag_keys: &'a [String]) -> Self {
208208
let span_kind = span.get_meta(TAG_SPANKIND).unwrap_or_default();
209-
let peer_tags = if client_or_producer(span_kind) {
209+
let peer_tags = if should_track_peer_tags(span_kind) {
210210
// Parse the meta tags of the span and return a list of the peer tags based on the list
211211
// of `peer_tag_keys`
212212
peer_tag_keys
213213
.iter()
214214
.filter_map(|key| Some(((key.as_str()), (span.get_meta(key.as_str())?))))
215215
.collect()
216+
} else if let Some(base_service) = span.get_meta("_dd.base_service") {
217+
// Internal spans with a base service override use only _dd.base_service as peer tag
218+
vec![("_dd.base_service", base_service)]
216219
} else {
217220
vec![]
218221
};
@@ -279,14 +282,14 @@ impl From<pb::ClientGroupedStats> for OwnedAggregationKey {
279282
}
280283
}
281284

282-
/// Return true if the span kind is "client" or "producer"
283-
fn client_or_producer<T>(span_kind: T) -> bool
285+
/// Return true if we care about peer tags on the span
286+
fn should_track_peer_tags<T>(span_kind: T) -> bool
284287
where
285288
T: SpanText,
286289
{
287290
matches!(
288291
span_kind.borrow().to_lowercase().as_str(),
289-
"client" | "producer"
292+
"client" | "producer" | "consumer"
290293
)
291294
}
292295

‎libdd-trace-stats/src/span_concentrator/tests.rs‎

Lines changed: 167 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -877,6 +877,173 @@ fn test_peer_tags_aggregation() {
877877
);
878878
}
879879

880+
/// Test that internal spans with _dd.base_service use it as their sole peer tag
881+
#[test]
882+
fn test_base_service_peer_tag() {
883+
let now = SystemTime::now();
884+
let mut spans = vec![
885+
// Regular internal span without base_service (no peer tags)
886+
get_test_span_with_meta(
887+
now,
888+
1,
889+
0,
890+
100,
891+
5,
892+
"A1",
893+
"internal.operation",
894+
0,
895+
&[],
896+
&[("_dd.measured", 1.0)],
897+
),
898+
// Internal span with _dd.base_service (should have base_service as peer tag)
899+
get_test_span_with_meta(
900+
now,
901+
2,
902+
0,
903+
75,
904+
5,
905+
"A1",
906+
"internal.with.base.service",
907+
0,
908+
&[("_dd.base_service", "original-service")],
909+
&[("_dd.measured", 1.0)],
910+
),
911+
// Another internal span with same _dd.base_service (should aggregate together)
912+
get_test_span_with_meta(
913+
now,
914+
3,
915+
0,
916+
50,
917+
5,
918+
"A1",
919+
"internal.with.base.service",
920+
0,
921+
&[("_dd.base_service", "original-service")],
922+
&[("_dd.measured", 1.0)],
923+
),
924+
// Internal span with different _dd.base_service (should be separate group)
925+
get_test_span_with_meta(
926+
now,
927+
4,
928+
0,
929+
60,
930+
5,
931+
"A1",
932+
"internal.with.base.service",
933+
0,
934+
&[("_dd.base_service", "other-service")],
935+
&[("_dd.measured", 1.0)],
936+
),
937+
// Client span with _dd.base_service and other peer tags enabled
938+
// (should use configured peer tags, not base_service)
939+
get_test_span_with_meta(
940+
now,
941+
5,
942+
0,
943+
80,
944+
5,
945+
"A1",
946+
"SELECT * FROM users",
947+
0,
948+
&[
949+
("span.kind", "client"),
950+
("_dd.base_service", "ignored-for-client"),
951+
("db.instance", "i-1234"),
952+
("db.system", "postgres"),
953+
],
954+
&[("_dd.measured", 1.0)],
955+
),
956+
];
957+
compute_top_level_span(spans.as_mut_slice());
958+
959+
let mut concentrator = SpanConcentrator::new(
960+
Duration::from_nanos(BUCKET_SIZE),
961+
now,
962+
get_span_kinds(),
963+
vec!["db.instance".to_string(), "db.system".to_string()],
964+
);
965+
966+
for span in &spans {
967+
concentrator.add_span(span);
968+
}
969+
970+
let flushtime =
971+
now + Duration::from_nanos(concentrator.bucket_size * concentrator.buffer_len as u64);
972+
973+
let expected = vec![
974+
// Internal span without base_service - no peer tags
975+
pb::ClientGroupedStats {
976+
service: "A1".to_string(),
977+
resource: "internal.operation".to_string(),
978+
r#type: "db".to_string(),
979+
name: "query".to_string(),
980+
duration: 100,
981+
hits: 1,
982+
top_level_hits: 1,
983+
errors: 0,
984+
is_trace_root: pb::Trilean::True.into(),
985+
..Default::default()
986+
},
987+
// Internal spans with _dd.base_service="original-service" - aggregated with base_service
988+
// peer tag
989+
pb::ClientGroupedStats {
990+
service: "A1".to_string(),
991+
resource: "internal.with.base.service".to_string(),
992+
r#type: "db".to_string(),
993+
name: "query".to_string(),
994+
peer_tags: vec!["_dd.base_service:original-service".to_string()],
995+
duration: 125,
996+
hits: 2,
997+
top_level_hits: 2,
998+
errors: 0,
999+
is_trace_root: pb::Trilean::True.into(),
1000+
..Default::default()
1001+
},
1002+
// Internal span with _dd.base_service="other-service" - separate group
1003+
pb::ClientGroupedStats {
1004+
service: "A1".to_string(),
1005+
resource: "internal.with.base.service".to_string(),
1006+
r#type: "db".to_string(),
1007+
name: "query".to_string(),
1008+
peer_tags: vec!["_dd.base_service:other-service".to_string()],
1009+
duration: 60,
1010+
hits: 1,
1011+
top_level_hits: 1,
1012+
errors: 0,
1013+
is_trace_root: pb::Trilean::True.into(),
1014+
..Default::default()
1015+
},
1016+
// Client span - uses configured peer tags, not base_service
1017+
pb::ClientGroupedStats {
1018+
service: "A1".to_string(),
1019+
resource: "SELECT * FROM users".to_string(),
1020+
r#type: "db".to_string(),
1021+
name: "query".to_string(),
1022+
span_kind: "client".to_string(),
1023+
peer_tags: vec![
1024+
"db.instance:i-1234".to_string(),
1025+
"db.system:postgres".to_string(),
1026+
],
1027+
duration: 80,
1028+
hits: 1,
1029+
top_level_hits: 1,
1030+
errors: 0,
1031+
is_trace_root: pb::Trilean::True.into(),
1032+
..Default::default()
1033+
},
1034+
];
1035+
1036+
let stats = concentrator.flush(flushtime, false);
1037+
assert_counts_equal(
1038+
expected,
1039+
stats
1040+
.first()
1041+
.expect("There should be at least one time bucket")
1042+
.stats
1043+
.clone(),
1044+
);
1045+
}
1046+
8801047
#[test]
8811048
fn test_compute_stats_for_span_kind() {
8821049
let test_cases: Vec<(SpanSlice, bool)> = vec![

0 commit comments

Comments
 (0)