-
Notifications
You must be signed in to change notification settings - Fork 8
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
Convert local root span id to u64, fix clippy lints #80
Changes from 2 commits
4990cc4
85cdec2
e391261
3eb917a
0a83855
d6ab0bd
344b3e3
e131455
2d662d4
1d3096e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -353,22 +353,20 @@ pub extern "C" fn ddog_prof_Profile_add( | |
/// | ||
/// # Arguments | ||
/// * `profile` - a reference to the profile that will contain the samples. | ||
/// * `local_root_span_id` - the value of the local root span id label to look for. | ||
/// * `local_root_span_id` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor: This is a weird change -- was it intentional? |
||
/// * `endpoint` - the value of the endpoint label to add for matching samples. | ||
/// | ||
/// # Safety | ||
/// The `profile` ptr must point to a valid Profile object created by this | ||
/// module. | ||
/// This call is _NOT_ thread-safe. | ||
#[no_mangle] | ||
pub unsafe extern "C" fn ddog_prof_Profile_set_endpoint<'a>( | ||
pub unsafe extern "C" fn ddog_prof_Profile_set_endpoint( | ||
profile: &mut datadog_profiling::profile::Profile, | ||
local_root_span_id: CharSlice<'a>, | ||
endpoint: CharSlice<'a>, | ||
local_root_span_id: u64, | ||
endpoint: CharSlice, | ||
) { | ||
let local_root_span_id = local_root_span_id.to_utf8_lossy(); | ||
let endpoint = endpoint.to_utf8_lossy(); | ||
|
||
profile.add_endpoint(local_root_span_id, endpoint); | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -91,7 +91,7 @@ pub struct Profile { | |
} | ||
|
||
pub struct Endpoints { | ||
mappings: FxIndexMap<i64, i64>, | ||
mappings: FxIndexMap<u64, i64>, | ||
local_root_span_id_label: i64, | ||
endpoint_label: i64, | ||
stats: ProfiledEndpointsStats, | ||
|
@@ -426,18 +426,19 @@ impl Profile { | |
Some(profile) | ||
} | ||
|
||
pub fn add_endpoint(&mut self, local_root_span_id: Cow<str>, endpoint: Cow<str>) { | ||
/// Add the endpoint data to the endpoint mappings. The `endpoint` will be | ||
/// interned, as will the strings "local root span id" and "trace endpoint". | ||
pub fn add_endpoint(&mut self, local_root_span_id: u64, endpoint: Cow<str>) { | ||
if self.endpoints.mappings.is_empty() { | ||
self.endpoints.local_root_span_id_label = self.intern("local root span id"); | ||
self.endpoints.endpoint_label = self.intern("trace endpoint"); | ||
} | ||
|
||
let interned_span_id = self.intern(local_root_span_id.as_ref()); | ||
let interned_endpoint = self.intern(endpoint.as_ref()); | ||
|
||
self.endpoints | ||
.mappings | ||
.insert(interned_span_id, interned_endpoint); | ||
.insert(local_root_span_id, interned_endpoint); | ||
} | ||
|
||
pub fn add_endpoint_count(&mut self, endpoint: Cow<str>, value: i64) { | ||
|
@@ -488,6 +489,35 @@ impl Profile { | |
pub fn get_string(&self, id: i64) -> Option<&String> { | ||
self.strings.get_index(id as usize) | ||
} | ||
|
||
fn get_endpoint_for_label(&self, label: &Label) -> anyhow::Result<&i64> { | ||
let local_root_span_id: u64 = if label.str == 0 { | ||
/* Safety: the value is a u64, but pprof only has signed values, | ||
* so we transmute it; the backend does the same. | ||
*/ | ||
unsafe { std::intrinsics::transmute(label.num) } | ||
} else { | ||
/* Sad path: have to fetch the string from the table and convert | ||
* it back into a u64. Clients should change to use numbers! | ||
*/ | ||
let string = match self.strings.get_index(label.str as usize) { | ||
None => anyhow::bail!( | ||
"failed to retrieve index {} from the string table when looking for endpoint data", | ||
label.str | ||
), | ||
Some(string) => string, | ||
}; | ||
string.as_str().parse()? | ||
}; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As this PR changes E.g., what clients would use the new numberic There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is coming from a label, and there's nothing that stops a client from attaching the label value as a string instead of a number. So it seems prudent to have the path, although it shouldn't be used. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm... I think having the fallback path is a sharper edge, as it makes things work even when they're not set up the way they should. E.g., if someone does it wrong, they will still get the right output, which is dangerous... One suggestion is to consider failing serialization and complaining that this label should not be a string. The There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay, I've started working on fixing this. There will be two changes:
|
||
|
||
match self.endpoints.mappings.get(&local_root_span_id) { | ||
None => Err(anyhow::anyhow!( | ||
"failed to retrieve endpoint information for local root span id {}", | ||
local_root_span_id | ||
)), | ||
Some(endpoint_string_id) => Ok(endpoint_string_id), | ||
} | ||
} | ||
} | ||
|
||
impl From<&Profile> for pprof::Profile { | ||
|
@@ -509,14 +539,14 @@ impl From<&Profile> for pprof::Profile { | |
|
||
if !profile.endpoints.mappings.is_empty() { | ||
for sample in samples.iter_mut() { | ||
let mut endpoint: Option<&i64> = None; | ||
|
||
for label in &sample.labels { | ||
let endpoint = sample.labels.iter().find_map(|label| { | ||
// todo: log errors in get_endpoint_for_label (should not happen) | ||
if label.key == profile.endpoints.local_root_span_id_label { | ||
endpoint = profile.endpoints.mappings.get(&label.str); | ||
break; | ||
profile.get_endpoint_for_label(label).ok() | ||
} else { | ||
None | ||
} | ||
} | ||
}); | ||
|
||
if let Some(endpoint_value) = endpoint { | ||
sample.labels.push(pprof::Label { | ||
|
@@ -907,7 +937,7 @@ mod api_test { | |
|
||
profile.add(sample2).expect("add to success"); | ||
|
||
profile.add_endpoint(Cow::from("10"), Cow::from("my endpoint")); | ||
profile.add_endpoint(10, Cow::from("my endpoint")); | ||
|
||
let serialized_profile: pprof::Profile = (&profile).into(); | ||
|
||
|
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.
Interesting -- is there no extended way for
format!
that can embed&agent_url
directly?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.
No, the inline format values can only contain simple identifiers https://rust-lang.github.io/rfcs/2795-format-args-implicit-identifiers.html
For the sake of uniformity, maybe named arguments would be better