-
Notifications
You must be signed in to change notification settings - Fork 9
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
Add .pprof to api::Profile helper #75
Conversation
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.
Other than making it clear that the new API is only for testing, I don't see any reason not to merge this in. Here sir, take my 👍
pub struct Profile<'a> { | ||
pub duration: Duration, | ||
pub period: Option<(i64, ValueType<'a>)>, | ||
pub sample_types: Vec<ValueType<'a>>, | ||
pub samples: Vec<Sample<'a>>, | ||
pub start_time: SystemTime, | ||
} |
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.
Given the assumption
There are no changes to shipped code, so there shouldn't be anything to test.
Should this be inside the mod test
? And perhaps also document the assumption in the code, so that other/newer team members know that this isn't expected to be used outside of libdatadog testing.
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.
Maybe. I need to see what configuration the bench
configuration uses -- I know it uses public APIs only, but I'm not sure if it builds with test or not, so that's the only reason I didn't immediately slap it into #[cfg(test)]
.
profiling/tests/wordpress.pprof
Outdated
@@ -0,0 +1,4441 @@ | |||
|
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.
Consider perhaps compressing this file as pprof.gz
or something OR at least telling github not to render them https://thoughtbot.com/blog/github-diff-supression :)
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.
Good idea 👍🏻
What does this PR do?
api::Profile
.TryFrom<&pprof::Profile> for api::Profile
.Motivation
This is a precursor PR for benchmarking hashing functions or other changes. This allows us to read in a pprof from disk and get an
api::Profile
from it, which we can use to add to the regular profile API. This allows us to benchmark adding samples, interning strings, etc.Additional Notes
I wrote the tests for those samples by hand -- they aren't auto-generated. It's a very slow process.
How to test the change?
There are no changes to shipped code, so there shouldn't be anything to test.