-
Notifications
You must be signed in to change notification settings - Fork 5
Associate label sets with wall profiles #105
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
Conversation
Overall package sizeSelf size: 2.08 MB Dependency sizes
🤖 This report was automatically generated by heaviest-objects-in-the-universe |
e38f05e
to
6009fcf
Compare
…le with labels was taken
a11552d
to
522d443
Compare
#endif | ||
|
||
if (includeLines && withLabels) { | ||
// Currently custom labels are not compatible with caller line |
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.
great comment, should we add a ticket to v8 for this ?
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.
LGTM
A very well thought approach
48fe3e5
to
dfbf67b
Compare
Make line number incompatible with custom labels
dfbf67b
to
516a9d6
Compare
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.
LGTM
// skip first sample because it's the one taken on profiler start, outside of | ||
// signal handler | ||
for (int i = 1; i < sampleCount; i++) { | ||
// Handle out-of-order samples, hypothesis is that at most 2 consecutive |
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.
@nsavoire Hi, I want to know why this happened and why do you need to deal with this situation ? I read the V8 source code and found that the timestamp of samples is incremental (test/cctest/test-cpu-profiler.cc).
uint64_t end_time = profile->GetEndTime();
uint64_t current_time = profile->GetStartTime();
CHECK_LE(current_time, end_time);
for (int i = 0; i < profile->GetSamplesCount(); i++) {
CHECK(profile->GetSample(i));
uint64_t timestamp = profile->GetSampleTimestamp(i);
// <=
CHECK_LE(current_time, timestamp);
CHECK_LE(timestamp, end_time);
current_time = timestamp;
}
Based on #98
Adds ability to associate custom label sets to wall samples. This can then be used by dd-trace-js to associate span ids with said samples.
Implementation notes
The implementation adds JS API to set and clear labels. In practice, the
WallProfiler
JS class will gain getters and setters for propertieslabels
.Internally, the implementation uses a PPROF signal handler executing before V8's signal handler used for profiling. It will associate the current label set with a timestamp, and put it in a preallocated buffer (so allocations are avoided in the signal handler.) This happens once every 10ms while profiling. The buffer is sized generously, for 2x the number of samples for a single profile.
When the profile is post-processed once every minute, the labels are matched to samples based on the timestamp, and emitted into the serialized profile.
Timestamp matching
Two timestamps are collected in profiler signal handler and are associated with the current label: one before v8 handler is invoked and one after.
These two timestamps allow easily matching v8 profiling samples to labels, each sample is matched with the label whose timestamp interval contains the sample timestamp.
v8 does not expose publicly its clock API (
v8::base::TimeTicks::Now()
), matching samples / labels without a common clock source proved to be difficult, hence profiler uses private v8 API by redeclaring it.