Skip to content

Commit 9f2417e

Browse files
authored
feat: single source of truth for headers (fixes issue in profiling with missing headers) (#1493)
# What does this PR do? Unifies the headers between common and profiling # Motivation We had missed some when we implemented our own exporter, this makes sure the two implementations stay in sync. # 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: daniel.schwartznarbonne <[email protected]>
1 parent b44bb77 commit 9f2417e

File tree

7 files changed

+131
-38
lines changed

7 files changed

+131
-38
lines changed

examples/ffi/exporter_manager.c

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
#include <stdio.h>
1010
#include <stdlib.h>
1111
#include <string.h>
12+
#include <sys/wait.h>
1213
#include <unistd.h>
1314

1415
static ddog_CharSlice to_slice_c_char(const char *s) { return (ddog_CharSlice){.ptr = s, .len = strlen(s)}; }

libdd-common/src/entity_id/mod.rs

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -99,3 +99,15 @@ pub static DD_EXTERNAL_ENV: LazyLock<Option<&'static str>> = LazyLock::new(|| {
9999

100100
leaked
101101
});
102+
103+
/// Returns an iterator of entity-related headers (container-id, entity-id, external-env)
104+
/// as (header_name, header_value) string tuples for any that are available.
105+
pub fn get_entity_headers() -> impl Iterator<Item = (&'static str, &'static str)> {
106+
[
107+
get_container_id().map(|v| ("datadog-container-id", v)),
108+
get_entity_id().map(|v| ("datadog-entity-id", v)),
109+
(*DD_EXTERNAL_ENV).map(|v| ("datadog-external-env", v)),
110+
]
111+
.into_iter()
112+
.flatten()
113+
}

libdd-common/src/lib.rs

Lines changed: 20 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77
#![cfg_attr(not(test), deny(clippy::unimplemented))]
88

99
use anyhow::Context;
10-
use hyper::{header::HeaderValue, http::uri};
10+
use hyper::http::uri;
1111
use serde::de::Error;
1212
use serde::{Deserialize, Deserializer, Serialize, Serializer};
1313
use std::sync::{Mutex, MutexGuard};
@@ -124,7 +124,6 @@ impl<C: hyper_util::client::legacy::connect::Connect + Clone + Send + Sync + 'st
124124
}
125125

126126
// Used by tag! macro
127-
use crate::entity_id::DD_EXTERNAL_ENV;
128127
pub use const_format;
129128

130129
#[derive(Clone, PartialEq, Eq, Hash, Debug, Serialize, Deserialize)]
@@ -241,6 +240,19 @@ impl Endpoint {
241240
/// Default value for the timeout field in milliseconds.
242241
pub const DEFAULT_TIMEOUT: u64 = 3_000;
243242

243+
/// Returns an iterator of optional endpoint-specific headers (api-key, test-token)
244+
/// as (header_name, header_value) string tuples for any that are available.
245+
pub fn get_optional_headers(&self) -> impl Iterator<Item = (&'static str, &str)> {
246+
[
247+
self.api_key.as_ref().map(|v| ("dd-api-key", v.as_ref())),
248+
self.test_token
249+
.as_ref()
250+
.map(|v| ("x-datadog-test-session-token", v.as_ref())),
251+
]
252+
.into_iter()
253+
.flatten()
254+
}
255+
244256
/// Return a request builder with the following headers:
245257
/// - User agent
246258
/// - Api key
@@ -250,32 +262,14 @@ impl Endpoint {
250262
.uri(self.url.clone())
251263
.header(hyper::header::USER_AGENT, user_agent);
252264

253-
// Add the Api key header if available
254-
if let Some(api_key) = &self.api_key {
255-
builder = builder.header(header::DATADOG_API_KEY, HeaderValue::from_str(api_key)?);
256-
}
257-
258-
// Add the test session token if available
259-
if let Some(token) = &self.test_token {
260-
builder = builder.header(
261-
header::X_DATADOG_TEST_SESSION_TOKEN,
262-
HeaderValue::from_str(token)?,
263-
);
264-
}
265-
266-
// Add the Container Id header if available
267-
if let Some(container_id) = entity_id::get_container_id() {
268-
builder = builder.header(header::DATADOG_CONTAINER_ID, container_id);
269-
}
270-
271-
// Add the Entity Id header if available
272-
if let Some(entity_id) = entity_id::get_entity_id() {
273-
builder = builder.header(header::DATADOG_ENTITY_ID, entity_id);
265+
// Add optional endpoint headers (api-key, test-token)
266+
for (name, value) in self.get_optional_headers() {
267+
builder = builder.header(name, value);
274268
}
275269

276-
// Add the External Env header if available
277-
if let Some(external_env) = *DD_EXTERNAL_ENV {
278-
builder = builder.header(header::DATADOG_EXTERNAL_ENV, external_env);
270+
// Add entity-related headers (container-id, entity-id, external-env)
271+
for (name, value) in entity_id::get_entity_headers() {
272+
builder = builder.header(name, value);
279273
}
280274

281275
Ok(builder)

libdd-profiling/src/exporter/profile_exporter.rs

Lines changed: 7 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -102,7 +102,6 @@ impl ProfileExporter {
102102
// Pre-build all static headers
103103
let mut headers = reqwest::header::HeaderMap::new();
104104

105-
// Always-present headers
106105
headers.insert(
107106
"Connection",
108107
reqwest::header::HeaderValue::from_static("close"),
@@ -123,18 +122,14 @@ impl ProfileExporter {
123122
))?,
124123
);
125124

126-
// Optional headers (API key, test token)
127-
if let Some(api_key) = &endpoint.api_key {
128-
headers.insert(
129-
"DD-API-KEY",
130-
reqwest::header::HeaderValue::from_str(api_key)?,
131-
);
125+
// Add optional endpoint headers (api-key, test-token)
126+
for (name, value) in endpoint.get_optional_headers() {
127+
headers.insert(name, reqwest::header::HeaderValue::from_str(value)?);
132128
}
133-
if let Some(test_token) = &endpoint.test_token {
134-
headers.insert(
135-
"X-Datadog-Test-Session-Token",
136-
reqwest::header::HeaderValue::from_str(test_token)?,
137-
);
129+
130+
// Add entity-related headers (container-id, entity-id, external-env)
131+
for (name, value) in libdd_common::entity_id::get_entity_headers() {
132+
headers.insert(name, reqwest::header::HeaderValue::from_static(value));
138133
}
139134

140135
// Add Azure App Services tags if available

libdd-profiling/tests/common.rs

Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,81 @@
1+
// Copyright 2024-Present Datadog, Inc. https://www.datadoghq.com/
2+
// SPDX-License-Identifier: Apache-2.0
3+
4+
//! Common test utilities
5+
6+
use std::collections::HashMap;
7+
8+
/// Validates that entity headers (container-id, entity-id, external-env) match
9+
/// the values provided by libdd_common::entity_id
10+
///
11+
/// # Current Limitations
12+
///
13+
/// **NOTE:** This test helper has known limitations that should be addressed in a follow-up PR:
14+
///
15+
/// 1. **Environment-dependent behavior**: The test changes its behavior dynamically based on the
16+
/// exact execution environment of the test runner (e.g., whether running in a container, whether
17+
/// certain environment variables are set).
18+
///
19+
/// 2. **Non-deterministic across environments**: What passes on a local machine may fail in CI (or
20+
/// vice versa) because the underlying entity detection functions return different values in
21+
/// different environments.
22+
///
23+
/// 3. **Incomplete test coverage**: We only exercise the codepaths that happen to be triggered in
24+
/// the current test environment, not all possible combinations of entity headers being
25+
/// present/absent.
26+
///
27+
/// **Future improvement**: The ideal approach would be to refactor the underlying code
28+
/// (`libdd_common::entity_id::get_container_id()`, `get_entity_id()`, etc.) to be more testable,
29+
/// perhaps by making them accept injectable dependencies or configuration. Then we could test all
30+
/// combinations: container-id [Some/None] × entity-id [Some/None] × external-env [Some/None] to
31+
/// verify correct header inclusion/exclusion in all 8 cases.
32+
///
33+
/// See discussion: https://github.com/DataDog/libdatadog/pull/1493#discussion_r2745712029
34+
pub fn assert_entity_headers_match(headers: &HashMap<String, String>) {
35+
// Check for entity headers and validate their values match what libdd_common provides
36+
let expected_container_id = libdd_common::entity_id::get_container_id();
37+
let expected_entity_id = libdd_common::entity_id::get_entity_id();
38+
let expected_external_env = *libdd_common::entity_id::DD_EXTERNAL_ENV;
39+
40+
// Validate container ID
41+
if let Some(expected) = expected_container_id {
42+
assert_eq!(
43+
headers.get("datadog-container-id"),
44+
Some(&expected.to_string()),
45+
"datadog-container-id header should match the value from entity_id::get_container_id()"
46+
);
47+
} else {
48+
assert!(
49+
!headers.contains_key("datadog-container-id"),
50+
"datadog-container-id header should not be present when entity_id::get_container_id() is None"
51+
);
52+
}
53+
54+
// Validate entity ID
55+
if let Some(expected) = expected_entity_id {
56+
assert_eq!(
57+
headers.get("datadog-entity-id"),
58+
Some(&expected.to_string()),
59+
"datadog-entity-id header should match the value from entity_id::get_entity_id()"
60+
);
61+
} else {
62+
assert!(
63+
!headers.contains_key("datadog-entity-id"),
64+
"datadog-entity-id header should not be present when entity_id::get_entity_id() is None"
65+
);
66+
}
67+
68+
// Validate external env
69+
if let Some(expected) = expected_external_env {
70+
assert_eq!(
71+
headers.get("datadog-external-env"),
72+
Some(&expected.to_string()),
73+
"datadog-external-env header should match the value from entity_id::DD_EXTERNAL_ENV"
74+
);
75+
} else {
76+
assert!(
77+
!headers.contains_key("datadog-external-env"),
78+
"datadog-external-env header should not be present when entity_id::DD_EXTERNAL_ENV is None"
79+
);
80+
}
81+
}

libdd-profiling/tests/exporter_e2e.rs

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,8 @@
55
//!
66
//! These tests validate the full export flow across different endpoint types.
77
8+
mod common;
9+
810
use libdd_common::test_utils::parse_http_request;
911
use libdd_profiling::exporter::config;
1012
use libdd_profiling::exporter::{File, MimeType, ProfileExporter};
@@ -303,6 +305,9 @@ fn validate_full_export(req: &ReceivedRequest, expected_path: &str) -> anyhow::R
303305
assert_eq!(req.method, "POST");
304306
assert_eq!(req.path, expected_path);
305307

308+
// Check for entity headers and validate their values match what libdd_common provides
309+
common::assert_entity_headers_match(&req.headers);
310+
306311
// Parse the request to get multipart parts
307312
// We need to reconstruct a minimal HTTP request to parse
308313
let mut http_request_bytes = Vec::new();

libdd-profiling/tests/file_exporter_test.rs

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
// Copyright 2021-Present Datadog, Inc. https://www.datadoghq.com/
22
// SPDX-License-Identifier: Apache-2.0
33

4+
mod common;
5+
46
use libdd_common::test_utils::{create_temp_file_path, parse_http_request, TempFileGuard};
57
use libdd_profiling::exporter::ProfileExporter;
68
use libdd_profiling::internal::EncodedProfile;
@@ -337,5 +339,8 @@ mod tests {
337339
request.headers.get("dd-evp-origin-version").unwrap(),
338340
profiling_library_version
339341
);
342+
343+
// Check for entity headers and validate their values match what libdd_common provides
344+
common::assert_entity_headers_match(&request.headers);
340345
}
341346
}

0 commit comments

Comments
 (0)