Skip to content

Commit

Permalink
Replace "Missing Allocations" with "Skipped Samples" for placeholder …
Browse files Browse the repository at this point in the history
…name

This was a great suggestion from the PR review.
  • Loading branch information
ivoanjo committed Jul 8, 2024
1 parent 67b9633 commit 1446109
Show file tree
Hide file tree
Showing 5 changed files with 26 additions and 26 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -1145,9 +1145,9 @@ static VALUE rescued_sample_allocation(VALUE tracepoint_data) {
// assigning a very large number to a sample, if for instance the dynamic sampling mechanism chose a really big interval.
unsigned int weight = allocations_since_last_sample > MAX_ALLOC_WEIGHT ? MAX_ALLOC_WEIGHT : (unsigned int) allocations_since_last_sample;
thread_context_collector_sample_allocation(state->thread_context_collector_instance, weight, new_object);
// ...but we still represent the missing allocations in the profile, thus the data will account for all allocations.
// ...but we still represent the skipped samples in the profile, thus the data will account for all allocations.
if (weight < allocations_since_last_sample) {
thread_context_collector_sample_missed_allocations(state->thread_context_collector_instance, allocations_since_last_sample - weight);
thread_context_collector_sample_skipped_allocation_samples(state->thread_context_collector_instance, allocations_since_last_sample - weight);
}

// Return a dummy VALUE because we're called from rb_rescue2 which requires it
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,7 @@ static void ddtrace_otel_trace_identifiers_for(
VALUE active_span,
VALUE otel_values
);
static VALUE _native_sample_missed_allocations(DDTRACE_UNUSED VALUE self, VALUE collector_instance, VALUE missed_allocations);
static VALUE _native_sample_skipped_allocation_samples(DDTRACE_UNUSED VALUE self, VALUE collector_instance, VALUE missed_allocations);

void collectors_thread_context_init(VALUE profiling_module) {
VALUE collectors_module = rb_define_module_under(profiling_module, "Collectors");
Expand Down Expand Up @@ -262,7 +262,7 @@ void collectors_thread_context_init(VALUE profiling_module) {
rb_define_singleton_method(testing_module, "_native_stats", _native_stats, 1);
rb_define_singleton_method(testing_module, "_native_gc_tracking", _native_gc_tracking, 1);
rb_define_singleton_method(testing_module, "_native_new_empty_thread", _native_new_empty_thread, 0);
rb_define_singleton_method(testing_module, "_native_sample_missed_allocations", _native_sample_missed_allocations, 2);
rb_define_singleton_method(testing_module, "_native_sample_skipped_allocation_samples", _native_sample_skipped_allocation_samples, 2);

at_active_span_id = rb_intern_const("@active_span");
at_active_trace_id = rb_intern_const("@active_trace");
Expand Down Expand Up @@ -1403,14 +1403,14 @@ static void ddtrace_otel_trace_identifiers_for(
*numeric_span_id = resolved_numeric_span_id;
}

void thread_context_collector_sample_missed_allocations(VALUE self_instance, unsigned int missed_allocations) {
void thread_context_collector_sample_skipped_allocation_samples(VALUE self_instance, unsigned int missed_allocations) {
struct thread_context_collector_state *state;
TypedData_Get_Struct(self_instance, struct thread_context_collector_state, &thread_context_collector_typed_data, state);

ddog_prof_Label labels[] = {
{.key = DDOG_CHARSLICE_C("thread id"), .str = DDOG_CHARSLICE_C("MA")},
{.key = DDOG_CHARSLICE_C("thread name"), .str = DDOG_CHARSLICE_C("Missing Allocations")},
{.key = DDOG_CHARSLICE_C("allocation class"), .str = DDOG_CHARSLICE_C("(Missing Allocations)")},
{.key = DDOG_CHARSLICE_C("thread id"), .str = DDOG_CHARSLICE_C("SA")},
{.key = DDOG_CHARSLICE_C("thread name"), .str = DDOG_CHARSLICE_C("Skipped Samples")},
{.key = DDOG_CHARSLICE_C("allocation class"), .str = DDOG_CHARSLICE_C("(Skipped Samples)")},
};
ddog_prof_Slice_Label slice_labels = {.ptr = labels, .len = sizeof(labels) / sizeof(labels[0])};

Expand All @@ -1423,11 +1423,11 @@ void thread_context_collector_sample_missed_allocations(VALUE self_instance, uns
.state_label = NULL,
.end_timestamp_ns = 0, // For now we're not collecting timestamps for allocation events
},
DDOG_CHARSLICE_C("Missing Allocations")
DDOG_CHARSLICE_C("Skipped Samples")
);
}

static VALUE _native_sample_missed_allocations(DDTRACE_UNUSED VALUE self, VALUE collector_instance, VALUE missed_allocations) {
thread_context_collector_sample_missed_allocations(collector_instance, NUM2UINT(missed_allocations));
static VALUE _native_sample_skipped_allocation_samples(DDTRACE_UNUSED VALUE self, VALUE collector_instance, VALUE missed_allocations) {
thread_context_collector_sample_skipped_allocation_samples(collector_instance, NUM2UINT(missed_allocations));
return Qtrue;
}
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ void thread_context_collector_sample(
VALUE profiler_overhead_stack_thread
);
void thread_context_collector_sample_allocation(VALUE self_instance, unsigned int sample_weight, VALUE new_object);
void thread_context_collector_sample_missed_allocations(VALUE self_instance, unsigned int missed_allocations);
void thread_context_collector_sample_skipped_allocation_samples(VALUE self_instance, unsigned int missed_allocations);
VALUE thread_context_collector_sample_after_gc(VALUE self_instance);
void thread_context_collector_on_gc_start(VALUE self_instance);
bool thread_context_collector_on_gc_finish(VALUE self_instance);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -543,15 +543,15 @@
# We then assign a weight to every sample to compensate for this; to avoid bias, we have a limit on this weight,
# and we clamp it if it goes over the limit.
# But the total amount of allocations recorded should match the number we observed, and thus we record the
# remainder above the clamped value as a separate "Missing Allocations" step.
it 'records missed allocations when weights are clamped' do
# remainder above the clamped value as a separate "Skipped Samples" step.
it 'records skipped allocation samples when weights are clamped' do
start

thread_that_allocates_as_fast_as_possible = Thread.new { loop { BasicObject.new } }

allocation_samples = try_wait_until do
samples = samples_from_pprof(recorder.serialize!).select { |it| it.values[:'alloc-samples'] > 0 }
samples if samples.any? { |it| it.labels[:'thread name'] == 'Missing Allocations' }
samples if samples.any? { |it| it.labels[:'thread name'] == 'Skipped Samples' }
end

thread_that_allocates_as_fast_as_possible.kill
Expand Down
22 changes: 11 additions & 11 deletions spec/datadog/profiling/collectors/thread_context_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -79,8 +79,8 @@ def sample_allocation(weight:, new_object: Object.new)
described_class::Testing._native_sample_allocation(cpu_and_wall_time_collector, weight, new_object)
end

def sample_missed_allocations(missed_allocations)
described_class::Testing._native_sample_missed_allocations(cpu_and_wall_time_collector, missed_allocations)
def sample_skipped_allocation_samples(missed_allocations)
described_class::Testing._native_sample_skipped_allocation_samples(cpu_and_wall_time_collector, missed_allocations)
end

def thread_list
Expand Down Expand Up @@ -1214,28 +1214,28 @@ def self.otel_sdk_available?
end
end

describe '#sample_missed_allocations' do
describe '#sample_skipped_allocation_samples' do
let(:single_sample) do
expect(samples.size).to be 1
samples.first
end
before { sample_missed_allocations(123) }
before { sample_skipped_allocation_samples(123) }

it 'records the number of missed allocations' do
it 'records the number of skipped allocations' do
expect(single_sample.values).to include('alloc-samples': 123)
end

it 'attributes the missed allocations to a "Missing Allocations" thread' do
expect(single_sample.labels).to include('thread id': 'MA', 'thread name': 'Missing Allocations')
it 'attributes the missed allocations to a "Skipped Samples" thread' do
expect(single_sample.labels).to include('thread id': 'SA', 'thread name': 'Skipped Samples')
end

it 'attributes the missed allocations to a "(Missing Allocations)" allocation class' do
expect(single_sample.labels).to include('allocation class': '(Missing Allocations)')
it 'attributes the missed allocations to a "(Skipped Samples)" allocation class' do
expect(single_sample.labels).to include('allocation class': '(Skipped Samples)')
end

it 'includes a placeholder stack attributed to "Missing Allocations"' do
it 'includes a placeholder stack attributed to "Skipped Samples"' do
expect(single_sample.locations.size).to be 1
expect(single_sample.locations.first.path).to eq 'Missing Allocations'
expect(single_sample.locations.first.path).to eq 'Skipped Samples'
end
end

Expand Down

0 comments on commit 1446109

Please sign in to comment.