-
Notifications
You must be signed in to change notification settings - Fork 19
feat: single source of truth for headers (fixes issue in profiling with missing headers) #1493
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
e677049
fix(profiling): missing headers in exporter
danielsn 164f002
Update libdd-common/src/lib.rs
danielsn 598e62c
Merge branch 'main' into dsn/fix-headers
danielsn 8f06e97
fix c example
danielsn 238505b
resolve conflicts
danielsn 7ac277e
comment test
danielsn File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,81 @@ | ||
| // Copyright 2024-Present Datadog, Inc. https://www.datadoghq.com/ | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| //! Common test utilities | ||
|
|
||
| use std::collections::HashMap; | ||
|
|
||
| /// Validates that entity headers (container-id, entity-id, external-env) match | ||
| /// the values provided by libdd_common::entity_id | ||
| /// | ||
| /// # Current Limitations | ||
| /// | ||
| /// **NOTE:** This test helper has known limitations that should be addressed in a follow-up PR: | ||
| /// | ||
| /// 1. **Environment-dependent behavior**: The test changes its behavior dynamically based on the | ||
| /// exact execution environment of the test runner (e.g., whether running in a container, whether | ||
| /// certain environment variables are set). | ||
| /// | ||
| /// 2. **Non-deterministic across environments**: What passes on a local machine may fail in CI (or | ||
| /// vice versa) because the underlying entity detection functions return different values in | ||
| /// different environments. | ||
| /// | ||
| /// 3. **Incomplete test coverage**: We only exercise the codepaths that happen to be triggered in | ||
| /// the current test environment, not all possible combinations of entity headers being | ||
| /// present/absent. | ||
| /// | ||
| /// **Future improvement**: The ideal approach would be to refactor the underlying code | ||
| /// (`libdd_common::entity_id::get_container_id()`, `get_entity_id()`, etc.) to be more testable, | ||
| /// perhaps by making them accept injectable dependencies or configuration. Then we could test all | ||
| /// combinations: container-id [Some/None] × entity-id [Some/None] × external-env [Some/None] to | ||
| /// verify correct header inclusion/exclusion in all 8 cases. | ||
| /// | ||
| /// See discussion: https://github.com/DataDog/libdatadog/pull/1493#discussion_r2745712029 | ||
| pub fn assert_entity_headers_match(headers: &HashMap<String, String>) { | ||
| // Check for entity headers and validate their values match what libdd_common provides | ||
| let expected_container_id = libdd_common::entity_id::get_container_id(); | ||
| let expected_entity_id = libdd_common::entity_id::get_entity_id(); | ||
| let expected_external_env = *libdd_common::entity_id::DD_EXTERNAL_ENV; | ||
|
|
||
| // Validate container ID | ||
| if let Some(expected) = expected_container_id { | ||
| assert_eq!( | ||
| headers.get("datadog-container-id"), | ||
| Some(&expected.to_string()), | ||
| "datadog-container-id header should match the value from entity_id::get_container_id()" | ||
| ); | ||
| } else { | ||
| assert!( | ||
| !headers.contains_key("datadog-container-id"), | ||
| "datadog-container-id header should not be present when entity_id::get_container_id() is None" | ||
| ); | ||
| } | ||
|
|
||
| // Validate entity ID | ||
| if let Some(expected) = expected_entity_id { | ||
| assert_eq!( | ||
| headers.get("datadog-entity-id"), | ||
| Some(&expected.to_string()), | ||
| "datadog-entity-id header should match the value from entity_id::get_entity_id()" | ||
| ); | ||
| } else { | ||
| assert!( | ||
| !headers.contains_key("datadog-entity-id"), | ||
| "datadog-entity-id header should not be present when entity_id::get_entity_id() is None" | ||
| ); | ||
| } | ||
|
|
||
| // Validate external env | ||
| if let Some(expected) = expected_external_env { | ||
| assert_eq!( | ||
| headers.get("datadog-external-env"), | ||
| Some(&expected.to_string()), | ||
| "datadog-external-env header should match the value from entity_id::DD_EXTERNAL_ENV" | ||
| ); | ||
| } else { | ||
| assert!( | ||
| !headers.contains_key("datadog-external-env"), | ||
| "datadog-external-env header should not be present when entity_id::DD_EXTERNAL_ENV is None" | ||
| ); | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm I'm not the biggest fan of these tests, since they change their behavior dynamically based on the exact execution environment of the test runner, which means that
a) Whatever I get running on my machine may be different -- so maybe it fails in CI but never for me
b) We don't get full test coverage -- only whatever codepaths end up being picked
Is it possible to maybe mock the results of those functions for our tests?
That is, what I would do if this were Ruby would be to zoom out and say "the behavior in my headers is -- if each of these entries is available, the headers contain what got returned, and if there was nothing than the headers don't exist"
Then I'd mock container-id: [dummy-container-id, none], entity-id: [dummy-entity-id, none], external-env: [dummy-external-env, none] and check that I get the correct behavior in each of the 6 cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rust doesn't have great capabilities for mocking. I'm going to merge this, then post a followup PR with the mocking code since its somewhat intrusive and we can discuss the benefits there.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks a bit like the underlying code, e.g.
get_entity_idand such were not written in such a way to be testable. Maybe we should re-examine that angle in the follow-up PR, rather than mocking specifically? It looks likeget_entity_idspecifically might not be too hard based on looking at the source code.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree that improving the test separately isn't a blocker. I would in any case leave a big comment above
assert_entity_headers_matchexplaining the situation until we fix it?