Skip to content
Open
Changes from 1 commit
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
71 changes: 52 additions & 19 deletions libdd-profiling-ffi/src/exporter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -201,23 +201,31 @@ unsafe fn into_vec_files<'a>(slice: Slice<'a, File>) -> Vec<exporter::File<'a>>
.collect()
}

unsafe fn parse_json(
/// Best-effort parse of an optional caller-supplied JSON blob. On invalid
/// UTF-8 or a `serde_json` parse error, log a warning and return `None`
/// instead of propagating the error. The `info` and `internal_metadata`
/// channels are supplementary signals attached to the profile upload event;
/// a malformed payload there should not cause us to drop the pprof itself,
/// which is the primary artifact.
unsafe fn parse_json_lossy(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nitpick: it's not really lossy (in that, if there's an error, it actually short-circuits as before, it just doesn't bubble error information and returns None instead, while "lossy" would indicate that we can get a partial result still)

string_id: &str,
json_string: Option<&CharSlice>,
) -> anyhow::Result<Option<serde_json::Value>> {
match json_string {
None => Ok(None),
Some(json_string) => {
let json = json_string.try_to_utf8()?;
match serde_json::from_str(json) {
Ok(parsed) => Ok(Some(parsed)),
Err(error) => Err(anyhow::anyhow!(
"Failed to parse contents of {} json string (`{}`): {}.",
string_id,
json,
error
)),
}
) -> Option<serde_json::Value> {
let cs = json_string?;
let json = match cs.try_to_utf8() {
Ok(s) => s,
Err(err) => {
eprintln!("warning: profile {string_id} JSON is not valid UTF-8, dropping it: {err}");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

eprintln could mess up customer logging

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Better approach: attach an json log to the profile, record this and other errors there

return None;
}
};
match serde_json::from_str(json) {
Ok(parsed) => Some(parsed),
Err(err) => {
eprintln!(
"warning: failed to parse profile {string_id} JSON (`{json}`), dropping it: {err}"
);
Comment thread
taegyunkim marked this conversation as resolved.
Outdated
None
}
}
}
Expand Down Expand Up @@ -289,8 +297,9 @@ pub unsafe extern "C" fn ddog_prof_Exporter_send_blocking(
let process_tags_str = optional_process_tags
.map(|cs| cs.try_to_utf8())
.transpose()?;
let internal_metadata = parse_json("internal_metadata", optional_internal_metadata_json)?;
let info = parse_json("info", optional_info_json)?;
let internal_metadata =
parse_json_lossy("internal_metadata", optional_internal_metadata_json);
let info = parse_json_lossy("info", optional_info_json);

let cancel = cancel.to_inner_mut().ok();
let status = exporter.send_blocking(
Expand Down Expand Up @@ -443,8 +452,9 @@ pub unsafe extern "C" fn ddog_prof_ExporterManager_queue(
let process_tags_str = optional_process_tags
.map(|cs| cs.try_to_utf8())
.transpose()?;
let internal_metadata = parse_json("internal_metadata", optional_internal_metadata_json)?;
let info = parse_json("info", optional_info_json)?;
let internal_metadata =
parse_json_lossy("internal_metadata", optional_internal_metadata_json);
let info = parse_json_lossy("info", optional_info_json);

manager.queue(
profile,
Expand Down Expand Up @@ -766,4 +776,27 @@ mod tests {
}
}
}

#[test]
fn test_parse_json_lossy_none() {
let parsed = unsafe { parse_json_lossy("info", None) };
assert!(parsed.is_none());
}

#[test]
fn test_parse_json_lossy_valid() {
let s = CharSlice::from(r#"{"runtime": {"engine": "test"}}"#);
let parsed = unsafe { parse_json_lossy("info", Some(&s)) }.expect("valid JSON");
assert_eq!(parsed["runtime"]["engine"], "test");
}

#[test]
fn test_parse_json_lossy_drops_invalid() {
// Malformed JSON: trailing comma + missing closing brace. Must not
// propagate an error; we should silently drop the payload so the
// pprof itself can still upload.
let s = CharSlice::from(r#"{"runtime": {"engine": "test",}"#);
let parsed = unsafe { parse_json_lossy("info", Some(&s)) };
assert!(parsed.is_none());
}
}
Loading