Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/verify-proto-files.yml
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ on:
pull_request:
types: [ opened, synchronize, reopened ]
env:
DATADOG_AGENT_TAG: "bdcdd8cf1ba4090a29b96d5669cfab5dd81814b1"
DATADOG_AGENT_TAG: "fdc29d4428db855950228a642c43a5595e56bb09"
CARGO_TERM_COLOR: always
CARGO_INCREMENTAL: 0
jobs:
Expand Down
1 change: 1 addition & 0 deletions datadog-ipc/src/shm_stats.rs
Original file line number Diff line number Diff line change
Expand Up @@ -818,6 +818,7 @@ impl ShmSpanConcentrator {
.unwrap_or_default(),
service_source: read_str!(f.service_source),
span_derived_primary_tags: vec![],
additional_metric_tags: vec![],
}
}
}
Expand Down
17 changes: 17 additions & 0 deletions libdd-trace-protobuf/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,19 @@ fn generate_protobuf() {
// intake expects the name ContainerID rather than the PascalCase ContainerId

config.type_attribute("TracerPayload", "#[derive(Deserialize, Serialize)]");
config.field_attribute(
".pb.TracerPayload.containerDebug",
"#[serde(skip_serializing_if = \"Option::is_none\")]",
);
config.type_attribute(
"ContainerDebug",
"#[derive(Deserialize, Serialize, PartialOrd, Ord)]",
);
config.field_attribute("ContainerDebug.error", "#[serde(default)]");
config.field_attribute("ContainerDebug.latencyMs", "#[serde(default)]");
config.field_attribute("ContainerDebug.wasBuffered", "#[serde(default)]");
config.field_attribute("ContainerDebug.bufferMs", "#[serde(default)]");
config.field_attribute("ContainerDebug.bufferEvictionReason", "#[serde(default)]");
config.type_attribute("TraceChunk", "#[derive(Deserialize, Serialize)]");

config.type_attribute("SpanLink", "#[derive(Deserialize, Serialize)]");
Expand Down Expand Up @@ -216,6 +229,10 @@ fn generate_protobuf() {
"ClientGroupedStats.span_derived_primary_tags",
"#[serde(default)]",
);
config.field_attribute(
"ClientGroupedStats.additional_metric_tags",
"#[serde(default)]",
);

config.field_attribute(
"ClientGroupedStats.okSummary",
Expand Down
28 changes: 28 additions & 0 deletions libdd-trace-protobuf/src/pb.idx.rs
Original file line number Diff line number Diff line change
Expand Up @@ -276,4 +276,32 @@ pub struct TracerPayload {
/// chunks specifies list of containing trace chunks.
#[prost(message, repeated, tag = "11")]
pub chunks: ::prost::alloc::vec::Vec<TraceChunk>,
/// containerDebug holds debug information about the container tags resolution.
#[prost(message, optional, tag = "12")]
pub container_debug: ::core::option::Option<ContainerDebug>,
}
/// ContainerDebug holds debug information about the container tags resolution process.
#[derive(Deserialize, Serialize, PartialOrd, Ord)]
#[derive(Clone, PartialEq, Eq, Hash, ::prost::Message)]
pub struct ContainerDebug {
/// error specifies any error that occurred during container tag resolution.
#[prost(string, tag = "1")]
#[serde(default)]
pub error: ::prost::alloc::string::String,
/// latencyMs specifies the latency in milliseconds of the container tag resolution.
#[prost(int64, tag = "2")]
#[serde(default)]
pub latency_ms: i64,
/// wasBuffered specifies whether the payload was buffered while waiting for container tags.
#[prost(bool, tag = "3")]
#[serde(default)]
pub was_buffered: bool,
/// bufferMs specifies how long the payload was buffered in milliseconds.
#[prost(int64, tag = "4")]
#[serde(default)]
pub buffer_ms: i64,
/// bufferEvictionReason specifies why the payload was evicted from the buffer.
#[prost(string, tag = "5")]
#[serde(default)]
pub buffer_eviction_reason: ::prost::alloc::string::String,
}
42 changes: 40 additions & 2 deletions libdd-trace-protobuf/src/pb.rs
Original file line number Diff line number Diff line change
Expand Up @@ -426,6 +426,41 @@ pub struct TracerPayload {
/// @gotags: json:"app_version" msg:"app_version"
#[prost(string, tag = "10")]
pub app_version: ::prost::alloc::string::String,
/// containerDebug holds debug information about the container tags resolution.
/// @gotags: json:"container_debug,omitempty" msg:"container_debug,omitempty"
#[prost(message, optional, tag = "11")]
#[serde(skip_serializing_if = "Option::is_none")]
pub container_debug: ::core::option::Option<ContainerDebug>,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Compare container_debug before merging payloads

When callers pass V07 payloads with this field populated, coalesce_send_data() still invokes TracerPayloadCollection::merge(), which treats payloads as equal via cmp_send_data_payloads() without looking at container_debug and then appends one payload's chunks into the other while dropping its top-level fields. Two payloads with different container tag resolution errors/latencies will therefore be sent as one payload with only the retained payload's debug value, misreporting the new diagnostics for the chunks from the dropped payload; include container_debug in the merge key or avoid merging when it differs.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in f8b4091.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@codex review

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Omit absent container debug from msgpack payloads

When sending V07 payloads through send_with_msgpack, every locally constructed tracer payload currently sets this field to None; because the generated serde field has no skip_serializing_if, rmp_serde::to_vec_named emits a container_debug: nil entry instead of omitting it, despite the new proto tag being omitempty. That changes the v0.7 wire map for all agentless sends even when no debug data exists, so please generate #[serde(skip_serializing_if = "Option::is_none")] for this field.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in f97eaf5.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@codex review

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Default omitted ContainerDebug subfields

When a Serde-encoded v0.7 payload includes container_debug but omits any of its omitempty subfields (for example a successful lookup may send only latency/buffering data and omit error), deserialization will fail because the nested ContainerDebug fields are still required. The Option here only handles the whole container_debug field being absent; please add #[serde(default)] for the ContainerDebug fields/type in the generator so Rust accepts the same partial payloads allowed by the proto/go tags.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in fbb6a57.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@codex review

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Codex Review: Didn't find any major issues. Delightful!

Reviewed commit: fbb6a5753b

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

}
/// ContainerDebug holds debug information about the container tags resolution process.
#[derive(Deserialize, Serialize, PartialOrd, Ord)]
#[derive(Clone, PartialEq, Eq, Hash, ::prost::Message)]
pub struct ContainerDebug {
/// error specifies any error that occurred during container tag resolution.
/// @gotags: json:"error,omitempty" msg:"error,omitempty"
#[prost(string, tag = "1")]
#[serde(default)]
pub error: ::prost::alloc::string::String,
/// latencyMs specifies the latency in milliseconds of the container tag resolution.
/// @gotags: json:"latency_ms,omitempty" msg:"latency_ms,omitempty"
#[prost(int64, tag = "2")]
#[serde(default)]
pub latency_ms: i64,
/// wasBuffered specifies whether the payload was buffered while waiting for container tags.
/// @gotags: json:"was_buffered,omitempty" msg:"was_buffered,omitempty"
#[prost(bool, tag = "3")]
#[serde(default)]
pub was_buffered: bool,
/// bufferMs specifies how long the payload was buffered in milliseconds.
/// @gotags: json:"buffer_ms,omitempty" msg:"buffer_ms,omitempty"
#[prost(int64, tag = "4")]
#[serde(default)]
pub buffer_ms: i64,
/// bufferEvictionReason specifies why the payload was evicted from the buffer.
/// @gotags: json:"buffer_eviction_reason,omitempty" msg:"buffer_eviction_reason,omitempty"
#[prost(string, tag = "5")]
#[serde(default)]
pub buffer_eviction_reason: ::prost::alloc::string::String,
}
/// AgentPayload represents payload the agent sends to the intake.
#[derive(Clone, PartialEq, ::prost::Message)]
Expand Down Expand Up @@ -659,13 +694,16 @@ pub struct ClientGroupedStats {
#[serde(rename = "srv_src")]
pub service_source: ::prost::alloc::string::String,
/// used to identify service override origin
/// span_derived_primary_tags are user-configured tags that are extracted from spans and used for stats aggregation
/// E.g., `aws.s3.bucket`, `http.url`, or any custom tag
/// Deprecated: use additional_metric_tags (field 23) instead.
#[prost(string, repeated, tag = "22")]
#[serde(default)]
pub span_derived_primary_tags: ::prost::alloc::vec::Vec<
::prost::alloc::string::String,
>,
/// additional_metric_tags are tags sent by tracers to be used as additional dimensions for stats aggregation
#[prost(string, repeated, tag = "23")]
#[serde(default)]
pub additional_metric_tags: ::prost::alloc::vec::Vec<::prost::alloc::string::String>,
}
/// Trilean is an expanded boolean type that is meant to differentiate between being unset and false.
#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash, PartialOrd, Ord, ::prost::Enumeration)]
Expand Down
16 changes: 16 additions & 0 deletions libdd-trace-protobuf/src/pb/idx/tracer_payload.proto
Original file line number Diff line number Diff line change
Expand Up @@ -48,4 +48,20 @@ message TracerPayload {
map<uint32, AnyValue> attributes = 10;
// chunks specifies list of containing trace chunks.
repeated TraceChunk chunks = 11;
// containerDebug holds debug information about the container tags resolution.
ContainerDebug containerDebug = 12;
}

// ContainerDebug holds debug information about the container tags resolution process.
message ContainerDebug {
// error specifies any error that occurred during container tag resolution.
string error = 1;
// latencyMs specifies the latency in milliseconds of the container tag resolution.
int64 latencyMs = 2;
// wasBuffered specifies whether the payload was buffered while waiting for container tags.
bool wasBuffered = 3;
// bufferMs specifies how long the payload was buffered in milliseconds.
int64 bufferMs = 4;
// bufferEvictionReason specifies why the payload was evicted from the buffer.
string bufferEvictionReason = 5;
}
5 changes: 3 additions & 2 deletions libdd-trace-protobuf/src/pb/stats.proto
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,8 @@ message ClientGroupedStats {
string HTTP_endpoint = 20; // Http route or quantized/simplified URL path
string service_source = 21; // @inject_tag: msg:"srv_src"
// used to identify service override origin
// span_derived_primary_tags are user-configured tags that are extracted from spans and used for stats aggregation
// E.g., `aws.s3.bucket`, `http.url`, or any custom tag
// Deprecated: use additional_metric_tags (field 23) instead.
repeated string span_derived_primary_tags = 22;
// additional_metric_tags are tags sent by tracers to be used as additional dimensions for stats aggregation
repeated string additional_metric_tags = 23;
}
22 changes: 22 additions & 0 deletions libdd-trace-protobuf/src/pb/tracer_payload.proto
Original file line number Diff line number Diff line change
Expand Up @@ -57,4 +57,26 @@ message TracerPayload {
// version specifies `version` tag that set with the tracer.
// @gotags: json:"app_version" msg:"app_version"
string appVersion = 10;
// containerDebug holds debug information about the container tags resolution.
// @gotags: json:"container_debug,omitempty" msg:"container_debug,omitempty"
ContainerDebug containerDebug = 11;
}

// ContainerDebug holds debug information about the container tags resolution process.
message ContainerDebug {
// error specifies any error that occurred during container tag resolution.
// @gotags: json:"error,omitempty" msg:"error,omitempty"
string error = 1;
// latencyMs specifies the latency in milliseconds of the container tag resolution.
// @gotags: json:"latency_ms,omitempty" msg:"latency_ms,omitempty"
int64 latencyMs = 2;
// wasBuffered specifies whether the payload was buffered while waiting for container tags.
// @gotags: json:"was_buffered,omitempty" msg:"was_buffered,omitempty"
bool wasBuffered = 3;
// bufferMs specifies how long the payload was buffered in milliseconds.
// @gotags: json:"buffer_ms,omitempty" msg:"buffer_ms,omitempty"
int64 bufferMs = 4;
// bufferEvictionReason specifies why the payload was evicted from the buffer.
// @gotags: json:"buffer_eviction_reason,omitempty" msg:"buffer_eviction_reason,omitempty"
string bufferEvictionReason = 5;
}
35 changes: 35 additions & 0 deletions libdd-trace-protobuf/src/pb_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,7 @@ mod tests {
http_method: "GET".to_string(),
service_source: "".to_string(),
span_derived_primary_tags: vec![],
additional_metric_tags: vec![],
}],
agent_time_shift: 0,
}],
Expand All @@ -133,4 +134,38 @@ mod tests {

assert_eq!(deserialized_stats_json, client_stats_payload)
}

#[test]
fn test_deserialize_tracer_payload_partial_container_debug() {
use crate::pb::{ContainerDebug, TracerPayload};

let json = r#"{
"container_id": "cid",
"language_name": "go",
"language_version": "1.22",
"tracer_version": "1.0",
"runtime_id": "runtime",
"chunks": [],
"tags": {},
"env": "prod",
"hostname": "host",
"app_version": "2.0",
"container_debug": {
"latency_ms": 42,
"was_buffered": true
}
}"#;

let decoded: TracerPayload = serde_json::from_str(json).unwrap();
assert_eq!(
decoded.container_debug,
Some(ContainerDebug {
error: String::new(),
latency_ms: 42,
was_buffered: true,
buffer_ms: 0,
buffer_eviction_reason: String::new(),
})
);
}
}
3 changes: 2 additions & 1 deletion libdd-trace-stats/src/span_concentrator/aggregation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -499,7 +499,8 @@ fn encode_grouped_stats(key: OwnedAggregationKey, group: GroupedStats) -> pb::Cl
.map(|c| c.to_string())
.unwrap_or_default(),
service_source: f.service_source,
span_derived_primary_tags: vec![], // Todo
span_derived_primary_tags: vec![],
additional_metric_tags: vec![],
}
}

Expand Down
1 change: 1 addition & 0 deletions libdd-trace-utils/src/stats_utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,7 @@ mod mini_agent_tests {
http_method: "GET".to_string(),
service_source: "".to_string(),
span_derived_primary_tags: vec![],
additional_metric_tags: vec![],
}],
agent_time_shift: 0,
}],
Expand Down
1 change: 1 addition & 0 deletions libdd-trace-utils/src/test_utils/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -498,6 +498,7 @@ pub fn create_send_data(size: usize, target_endpoint: &Endpoint) -> SendData {
env: "test".to_owned(),
hostname: "test_host".to_owned(),
app_version: "2.0".to_owned(),
container_debug: None,
};

SendData::new(
Expand Down
3 changes: 3 additions & 0 deletions libdd-trace-utils/src/trace_utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -318,6 +318,7 @@ pub(crate) fn construct_tracer_payload(
language_version: tracer_tags.lang_version.to_string(),
tags: HashMap::new(),
tracer_version: tracer_tags.tracer_version.to_string(),
container_debug: None,
}
}

Expand All @@ -331,6 +332,7 @@ pub(crate) fn cmp_send_data_payloads(a: &pb::TracerPayload, b: &pb::TracerPayloa
.then(a.runtime_id.cmp(&b.runtime_id))
.then(a.env.cmp(&b.env))
.then(a.app_version.cmp(&b.app_version))
.then(a.container_debug.cmp(&b.container_debug))
}

pub fn coalesce_send_data(mut data: Vec<SendData>) -> Vec<SendData> {
Expand Down Expand Up @@ -742,6 +744,7 @@ mod tests {
env: "".to_string(),
hostname: "".to_string(),
app_version: "".to_string(),
container_debug: None,
}]),
TracerHeaderTags::default(),
&Endpoint::default(),
Expand Down
1 change: 1 addition & 0 deletions libdd-trace-utils/src/tracer_payload.rs
Original file line number Diff line number Diff line change
Expand Up @@ -264,6 +264,7 @@ mod tests {
env: "".to_string(),
hostname: "".to_string(),
app_version: "".to_string(),
container_debug: None,
}])
}

Expand Down
Loading