fix(libdd-telemetry-ffi): logic#1897
Open
Aaalibaba42 wants to merge 4 commits intomainfrom
Open
Conversation
Aaalibaba42
changed the title
Contributor
Clippy Allow Annotation ReportComparing clippy allow annotations between branches:
Summary by Rule
Annotation Counts by File
Annotation Stats by Crate
About This ReportThis report tracks Clippy allow annotations for specific rules, showing how they've changed in this PR. Decreasing the number of these annotations generally improves code quality. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1897 +/- ##
==========================================
+ Coverage 71.99% 72.16% +0.16%
==========================================
Files 434 434
Lines 70450 70609 +159
==========================================
+ Hits 50718 50952 +234
+ Misses 19732 19657 -75
🚀 New features to boost your workflow:
|
🎉 All green!❄️ No new flaky tests detected 🎯 Code Coverage (details) 🔗 Commit SHA: e467300 | Docs | Datadog PR Page | Give us feedback! |
Contributor
Artifact Size Benchmark Reportaarch64-alpine-linux-musl
aarch64-unknown-linux-gnu
libdatadog-x64-windows
libdatadog-x86-windows
x86_64-alpine-linux-musl
x86_64-unknown-linux-gnu
|
ekump
reviewed
May 5, 2026
| let version = dependency_version | ||
| .is_empty() | ||
| .then(|| dependency_version.to_utf8_lossy().into_owned()); | ||
| let version = |
Contributor
There was a problem hiding this comment.
There is already something similar here. You could just add another function to support lossy (I'd question why we're ok with lossy here, but that's perhaps beyond the scope of the PR). If the logic lives in libdd-common-ffi, it should theoretically make testing a bit easier too.
Contributor
Author
There was a problem hiding this comment.
Looking a bit into it, I agree this probably shouldn't use the lossy version. Other crates in libdatadog don't accept lossy except in the demangler where mangled symbols can genuinely be non-UTF-8. Here, it would mean accepting a version number or something like this which contain non utf-8 compliant bytes, and ending up accepting things with 1.2.� at the end of the chain, which would be harder to debug etc.
ekump
reviewed
May 5, 2026
| stack_trace | ||
| .is_empty() | ||
| .then(|| stack_trace.to_utf8_lossy().into_owned()), | ||
| (!stack_trace.is_empty()).then(|| stack_trace.to_utf8_lossy().into_owned()), |
Contributor
There was a problem hiding this comment.
I don't think this change is covered by your existing tests.
Contributor
📚 Documentation Check Results📦
|
Contributor
🔒 Cargo Deny Results📦
|
Aaalibaba42
force-pushed
the
jwiriath/telemetry-fix-logic
branch
from
de2138c to
e467300
Compare
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Bug
The FFI functions ddog_telemetry_handle_add_dependency, ddog_telemetry_handle_add_integration, and ddog_telemetry_handle_add_log had an inverted condition when converting optional string fields (version, stack_trace) from C slices to Rust Option.
The code was using .is_empty().then(|| ...), which would populate the Option only when the input was empty — the exact opposite of the intended behavior. Non-empty values were silently dropped, and empty strings were incorrectly converted and passed through.
Fix
Changed the condition to (!slice.is_empty()).then(|| ...) so that the Option is Some(...) when the input is non-empty and None when it is empty, as intended.
Additional Notes
Addresses APMSP-2937
How to test the change?
96844c8 show tests with failing stuff, 14b6d1e show the fix and the tests passing