Tweaks for datadog compatibility #74
Conversation
Codecov Report
@@ Coverage Diff @@
## master #74 +/- ##
==========================================
+ Coverage 96.61% 96.62% +0.01%
==========================================
Files 3 3
Lines 295 296 +1
==========================================
+ Hits 285 286 +1
Misses 10 10
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| @@ -43,7 +43,7 @@ function pprof(alloc_profile::Profile.Allocs.AllocResults = Profile.Allocs.fetch | |||
| # Allocs-specific arguments: | |||
| frame_for_type::Bool = true, | |||
| ) | |||
| period = UInt64(0x1) | |||
| period = sum(alloc.size for alloc in alloc_profile.allocs, init=0) | |||
There was a problem hiding this comment.
🤔 this seems weird.. Isn't this supposed to basically represent how much whatever does one sample represent?
So for a CPU profile, this would be like how much time in between samples? So for an allocation profile, it should be 1 allocation per sample, right?
There was a problem hiding this comment.
Hm yeah I was misunderstanding this.
Looking at the proto, it says "The number of events between sampled occurrences": https://github.com/google/pprof/blob/131d412537eacd9973045c73835c3ac6ed696765/proto/profile.proto#L85-L86
So I guess it's basically supposed to be the sample interval? E.g. "every 1000th alloc" would be 1000?
| ] | ||
| period_type = ValueType!("heap", "bytes") | ||
| period_type = ValueType!("alloc_space", "bytes") |
There was a problem hiding this comment.
oh maybe the period_type should actually be (alloc_objects", "count")? then maybe you could keep the period = 0x01?
src/PProf.jl
Outdated
| @@ -163,7 +163,7 @@ function pprof(data::Union{Nothing, Vector{UInt}} = nothing, | |||
| # End of sample | |||
| value = [ | |||
| 1, # events | |||
| length(location_id), # stack_depth | |||
| 60000000 # CPU ns | |||
There was a problem hiding this comment.
Hm yeah, I think I found I needed to put some number here to get it to show up in DD, but this is just a bogus value. Not sure what it's supposed to be… Maybe it's supposed to correspond to the sample period? i.e. if you're taking a stack sample every 100ms, this is supposed to be 100ms (in ns)?
There was a problem hiding this comment.
ah, yes, that's right. i think that is exactly what it's supposed to be. it's supposed to be the value matching the value type:
ValueType!("cpu", "nanoseconds")
👍
🤔 i think you should be able to ask Profile what it's current sample rate is, so that this can be an accurate number.
help?> Profile.init()
init(; n::Integer, delay::Real)
Configure the delay between backtraces (measured in seconds), and the number n of instruction pointers that may be stored per thread. Each instruction
pointer corresponds to a single line of code; backtraces generally consist of a long list of instruction pointers. Note that 6 spaces for instruction
pointers per backtrace are used to store metadata and two NULL end markers. Current settings can be obtained by calling this function with no arguments,
and each can be set independently using keywords or in the order (n, delay).
│ Julia 1.8
│
│ As of Julia 1.8, this function allocates space for n instruction pointers per thread being profiled. Previously this was n total.
julia> Profile.init()
(10000000, 0.001)
julia> Profile.init()[2]
0.001
| ValueType!("events", "count"), # Mandatory | ||
| ValueType!("stack_depth", "count") | ||
| ValueType!("events", "count"), # Mandatory | ||
| ValueType!("cpu", "nanoseconds") |
There was a problem hiding this comment.
can we still emit stack-depth values, or does even their presence break the datadog viewer?
I'm fine to drop them though; i don't think they really added much, and they're just there because Valentin and i were trying to understand how the sample_types worked, i think
There was a problem hiding this comment.
I'm not sure if they break the DD viewer… I'm not sure why you'd need them though, since each sample itself is a full stack, so the depth information is already there
| full_name_with_args = _escape_name_for_pprof(String(take!(io))) | ||
| call_str = String(take!(io)) | ||
| # add module name as well | ||
| full_name_with_args = _escape_name_for_pprof("$(meth.module).$call_str") |
There was a problem hiding this comment.
adding the module name here should be the only user-visible change in this PR — it's pretty essential to get frames to show up right in DD since it essentially does
package, func = split(name, "."; limit=2)
e.g. for this one:
Without the module name it would parse as
- package:
DatadogProfileUploader.profile_and_upload(DatadogProfileUploader - func:
DDConfig, typeof(myfunc))
If people don't want the behavior to change though, I could put this behind an option.
Without these, the flame graph shows up empty in their viewer.
https://www.datadoghq.com/product/code-profiling/
(Accidentally closed #73; reopening)