fix(profiling): drop malformed info / internal_metadata JSON instead of failing the upload#1984
Open
taegyunkim wants to merge 2 commits into
Open
fix(profiling): drop malformed info / internal_metadata JSON instead of failing the upload#1984taegyunkim wants to merge 2 commits into
taegyunkim wants to merge 2 commits into
Conversation
…of failing the upload The `ddog_prof_Exporter_send_blocking` and async `send` entry points used to call `parse_json(...)?` for both `optional_internal_metadata_json` and `optional_info_json`, which propagated a parse error all the way back to the caller. In practice that meant a malformed supplementary JSON blob (e.g. a stray `Infinity` from Python's permissive `json.dumps`, or an integer that exceeds the range serde_json can represent) caused the entire pprof profile to be dropped on the floor. Replace `parse_json` with `parse_json_lossy`, which logs a warning and returns `None` on either invalid UTF-8 or a serde_json parse failure. The profile, which is the primary artifact, still uploads with whatever supplementary fields parsed successfully. This matches what callers already do on their side: dd-trace-py wraps the `json.dumps` of its `info.profiler.settings` payload in a try/except and swallows failures, on the assumption that a bad metadata blob is not worth losing a profile over. The native side now has the same semantics. * `parse_json_lossy` is private to the module and replaces the previous `parse_json`. No other crate referenced the old function. * Logging uses `eprintln!` to match the style in `libdd-profiling/src/exporter/exporter_manager.rs`. * Three new unit tests cover the None / valid / invalid paths. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
📚 Documentation Check Results📦
|
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. |
Contributor
🔒 Cargo Deny Results📦
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 229251b686
ℹ️ 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".
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1984 +/- ##
=======================================
Coverage 72.54% 72.55%
=======================================
Files 451 451
Lines 74481 74500 +19
=======================================
+ Hits 54030 54050 +20
+ Misses 20451 20450 -1
🚀 New features to boost your workflow:
|
Contributor
🎉 All green!❄️ No new flaky tests detected 🎯 Code Coverage (details) 🔗 Commit SHA: f5b1680 | 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
|
…ranch Addresses PR review feedback: - Avoid logging the entire caller-provided JSON blob on parse failure; it may contain runtime/profiler configuration. Log only the field name and the serde_json error. - Add a test for the invalid-UTF-8 branch of parse_json_lossy so the early-return path is covered. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
2 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.
What does this PR do?
Replaces the strict
parse_jsonhelper used byddog_prof_Exporter_send_blockingand the asyncsendentry point withparse_json_lossy, which logs a warning to stderr and returnsNoneinstead of propagating an error when eitheroptional_internal_metadata_jsonoroptional_info_jsonfails to parse.The 4 internal call sites in
libdd-profiling-ffi/src/exporter.rs(lines 302-303 insend_blocking, 456-457 insend) drop the?and let the lossy helper handle errors locally.Motivation
Before this change, the FFI entry points used
parse_json(...)?for both supplementary JSON channels. A parse failure aborted the whole upload and the caller observed aResult::Err, meaning the entire pprof profile was dropped because of a bad metadata blob.This is asymmetric with what callers already do on their own side. dd-trace-py wraps
json.dumps(info_payload)in atry/except(profiler.py:209-217) and continues on failure. dd-trace-go and dd-trace-php take similar best-effort approaches when building the upload event. The native FFI layer was the one place where a malformed supplementary blob could escalate to a dropped profile.Realistic ways the parse can fail even when the caller's own serializer succeeded:
json.dumpsemits the literalInfinity/NaNby default (non-standard JSON).serde_json::from_strrejects them.f64range silently round in serde_json; integers above that fail outright.The pprof is the primary artifact.
infoandinternal_metadataare supplementary signals attached to the multipart event. A bad supplementary blob should not be allowed to take down the primary artifact.Additional Notes
parse_json_lossyreplaces the oldparse_jsonoutright. The old function was private to the module (unsafe fn, nopub) and was not referenced from any other crate (verified with a repo-wide grep; the otherparse_jsonmatches underdatadog-remote-config/are unrelated functions in different modules).eprintln!to match the existing style inlibdd-profiling/src/exporter/exporter_manager.rs:476/509/579. Happy to switch totracing::warn!if that's the desired direction for this crate.info.profiler.settingssnapshot ofProfilingConfigto each upload. The downstream side already wraps itsjson.dumpsin a try/except, so this PR closes the only remaining gap.How to test the change?
Three new tests cover the contract:
exporter::tests::test_parse_json_lossy_none-Noneinput passes through unchanged.exporter::tests::test_parse_json_lossy_valid- valid JSON round-trips intoserde_json::Value.exporter::tests::test_parse_json_lossy_drops_invalid- malformed JSON (trailing comma, missing brace) returnsNonerather than panicking or propagating an error.All 57 tests in the crate pass, including the existing
test_send_blocking_with_metadatawhich exercises the FFI end-to-end with valid metadata + info JSON.