From 51eb0e215084145b255e0a69fc3c85c13a495cc8 Mon Sep 17 00:00:00 2001 From: Taegyun Kim Date: Mon, 30 Sep 2024 14:10:12 -0400 Subject: [PATCH] fix(profiling): reverse locations for stack v2 [backport-2.14] (#10871) Manual backport of https://github.com/DataDog/dd-trace-py/pull/10851 to 2.14 Fixes an issue where flamegraph was upside down for stack v2, when DD_PROFILING_STACK_V2_ENABLED ## Checklist - [x] PR author has checked that all the criteria below are met - The PR description includes an overview of the change - The PR description articulates the motivation for the change - The change includes tests OR the PR description describes a testing strategy - The PR description notes risks associated with the change, if any - Newly-added code is easy to change - The change follows the [library release note guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html) - The change includes or references documentation updates if necessary - Backport labels are set (if [applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)) ## Reviewer Checklist - [x] Reviewer has checked that all the criteria below are met - Title is accurate - All changes are related to the pull request's stated goal - Avoids breaking [API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces) changes - Testing strategy adequately addresses listed risks - Newly-added code is easy to change - Release note makes sense to a user of the library - If necessary, author has acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment - Backport labels are set in a manner that is consistent with the [release branch maintenance policy](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting) --- .../dd_wrapper/include/ddup_interface.hpp | 2 + .../profiling/dd_wrapper/include/sample.hpp | 2 +- .../dd_wrapper/src/ddup_interface.cpp | 6 ++ .../profiling/dd_wrapper/src/sample.cpp | 7 ++- .../profiling/stack_v2/src/stack_renderer.cpp | 2 +- ...stack-v2-upside-down-82ebd3073b5e8e43.yaml | 5 ++ tests/profiling/collector/pprof_utils.py | 27 ++++++++- tests/profiling_v2/collector/test_stack.py | 55 +++++++++++++++++++ 8 files changed, 102 insertions(+), 4 deletions(-) create mode 100644 releasenotes/notes/profiling-stack-v2-upside-down-82ebd3073b5e8e43.yaml diff --git a/ddtrace/internal/datadog/profiling/dd_wrapper/include/ddup_interface.hpp b/ddtrace/internal/datadog/profiling/dd_wrapper/include/ddup_interface.hpp index b09a727fa84..cd18ead1966 100644 --- a/ddtrace/internal/datadog/profiling/dd_wrapper/include/ddup_interface.hpp +++ b/ddtrace/internal/datadog/profiling/dd_wrapper/include/ddup_interface.hpp @@ -63,6 +63,8 @@ extern "C" int64_t line); void ddup_push_monotonic_ns(Datadog::Sample* sample, int64_t monotonic_ns); void ddup_flush_sample(Datadog::Sample* sample); + // Stack v2 specific flush, which reverses the locations + void ddup_flush_sample_v2(Datadog::Sample* sample); void ddup_drop_sample(Datadog::Sample* sample); #ifdef __cplusplus } // extern "C" diff --git a/ddtrace/internal/datadog/profiling/dd_wrapper/include/sample.hpp b/ddtrace/internal/datadog/profiling/dd_wrapper/include/sample.hpp index 1f755ea167a..56d356fa9b6 100644 --- a/ddtrace/internal/datadog/profiling/dd_wrapper/include/sample.hpp +++ b/ddtrace/internal/datadog/profiling/dd_wrapper/include/sample.hpp @@ -84,7 +84,7 @@ class Sample ); // Flushes the current buffer, clearing it - bool flush_sample(); + bool flush_sample(bool reverse_locations = false); static ddog_prof_Profile& profile_borrow(); static void profile_release(); diff --git a/ddtrace/internal/datadog/profiling/dd_wrapper/src/ddup_interface.cpp b/ddtrace/internal/datadog/profiling/dd_wrapper/src/ddup_interface.cpp index 684f4278bf7..b49d4605873 100644 --- a/ddtrace/internal/datadog/profiling/dd_wrapper/src/ddup_interface.cpp +++ b/ddtrace/internal/datadog/profiling/dd_wrapper/src/ddup_interface.cpp @@ -271,6 +271,12 @@ ddup_flush_sample(Datadog::Sample* sample) // cppcheck-suppress unusedFunction sample->flush_sample(); } +void +ddup_flush_sample_v2(Datadog::Sample* sample) // cppcheck-suppress unusedFunction +{ + sample->flush_sample(/*reverse_locations*/ true); +} + void ddup_drop_sample(Datadog::Sample* sample) // cppcheck-suppress unusedFunction { diff --git a/ddtrace/internal/datadog/profiling/dd_wrapper/src/sample.cpp b/ddtrace/internal/datadog/profiling/dd_wrapper/src/sample.cpp index 7bb6fadb942..9f939055f9e 100644 --- a/ddtrace/internal/datadog/profiling/dd_wrapper/src/sample.cpp +++ b/ddtrace/internal/datadog/profiling/dd_wrapper/src/sample.cpp @@ -1,5 +1,6 @@ #include "sample.hpp" +#include #include #include @@ -104,7 +105,7 @@ Datadog::Sample::clear_buffers() } bool -Datadog::Sample::flush_sample() +Datadog::Sample::flush_sample(bool reverse_locations) { if (dropped_frames > 0) { const std::string name = @@ -112,6 +113,10 @@ Datadog::Sample::flush_sample() Sample::push_frame_impl(name, "", 0, 0); } + if (reverse_locations) { + std::reverse(locations.begin(), locations.end()); + } + const ddog_prof_Sample sample = { .locations = { locations.data(), locations.size() }, .values = { values.data(), values.size() }, diff --git a/ddtrace/internal/datadog/profiling/stack_v2/src/stack_renderer.cpp b/ddtrace/internal/datadog/profiling/stack_v2/src/stack_renderer.cpp index d013e13735e..9ee7a3813d0 100644 --- a/ddtrace/internal/datadog/profiling/stack_v2/src/stack_renderer.cpp +++ b/ddtrace/internal/datadog/profiling/stack_v2/src/stack_renderer.cpp @@ -144,7 +144,7 @@ StackRenderer::render_stack_end() return; } - ddup_flush_sample(sample); + ddup_flush_sample_v2(sample); ddup_drop_sample(sample); sample = nullptr; } diff --git a/releasenotes/notes/profiling-stack-v2-upside-down-82ebd3073b5e8e43.yaml b/releasenotes/notes/profiling-stack-v2-upside-down-82ebd3073b5e8e43.yaml new file mode 100644 index 00000000000..55775a1550d --- /dev/null +++ b/releasenotes/notes/profiling-stack-v2-upside-down-82ebd3073b5e8e43.yaml @@ -0,0 +1,5 @@ +--- +fixes: + - | + profiling: fixes an issue where flamegraph was upside down for stack v2, + ``DD_PROFILING_STACK_V2_ENABLED``. diff --git a/tests/profiling/collector/pprof_utils.py b/tests/profiling/collector/pprof_utils.py index ddf4dff74b2..a9a2425dca6 100644 --- a/tests/profiling/collector/pprof_utils.py +++ b/tests/profiling/collector/pprof_utils.py @@ -28,6 +28,12 @@ def reinterpret_int_as_int64(value: int) -> int: return ctypes.c_int64(value).value +class StackLocation: + def __init__(self, function_name: str, filename: str): + self.function_name = function_name + self.filename = filename + + class LockEventType(Enum): ACQUIRE = 1 RELEASE = 2 @@ -50,7 +56,8 @@ def __init__( class StackEvent(EventBaseClass): - def __init__(self, *args, **kwargs): + def __init__(self, locations: typing.Optional[typing.Any] = None, *args, **kwargs): + self.locations = locations super().__init__(*args, **kwargs) @@ -265,5 +272,23 @@ def assert_lock_event(profile, sample: pprof_pb2.Sample, expected_event: LockEve assert_base_event(profile, sample, expected_event) +# helper function to check whether the expected stack event is present in the samples +def has_sample_with_locations( + profile, samples: typing.List[pprof_pb2.Sample], expected_locations: typing.List[StackLocation] +) -> bool: + for sample in samples: + for location_id, expected_location in zip(sample.location_id, expected_locations): + location = get_location_with_id(profile, location_id) + function = get_function_with_id(profile, location.line[0].function_id) + if profile.string_table[function.name] != expected_location.function_name: + continue + if not profile.string_table[function.filename].endswith(expected_location.filename): + continue + + return True + + return False + + def assert_stack_event(profile, sample: pprof_pb2.Sample, expected_event: StackEvent): assert_base_event(profile, sample, expected_event) diff --git a/tests/profiling_v2/collector/test_stack.py b/tests/profiling_v2/collector/test_stack.py index cbd7c7c386e..da5a9078642 100644 --- a/tests/profiling_v2/collector/test_stack.py +++ b/tests/profiling_v2/collector/test_stack.py @@ -1,6 +1,61 @@ import pytest +@pytest.mark.subprocess(env=dict(DD_PROFILING_STACK_V2_ENABLED="true")) +def test_stack_v2_locations(): + import os + import time + + from ddtrace.internal.datadog.profiling import ddup + from ddtrace.profiling.collector import stack + from tests.profiling.collector import pprof_utils + + test_name = "test_locations" + pprof_prefix = "/tmp/" + test_name + output_filename = pprof_prefix + "." + str(os.getpid()) + + assert ddup.is_available + ddup.config(env="test", service=test_name, version="my_version", output_filename=pprof_prefix) + ddup.start() + + def baz(): + time.sleep(0.01) + + def bar(): + baz() + + def foo(): + bar() + + with stack.StackCollector(None): + for _ in range(5): + foo() + ddup.upload() + + profile = pprof_utils.parse_profile(output_filename) + samples = pprof_utils.get_samples_with_value_type(profile, "wall-time") + assert len(samples) > 0 + + expected_locations = [ + pprof_utils.StackLocation( + function_name="baz", + filename="test_stack.py", + ), + pprof_utils.StackLocation( + function_name="bar", + filename="test_stack.py", + ), + pprof_utils.StackLocation( + function_name="foo", + filename="test_stack.py", + ), + ] + + assert pprof_utils.has_sample_with_locations( + profile, samples, expected_locations + ), "Sample with expected locations not found" + + # Tests here are marked as subprocess as they are flaky when not marked as such, # similar to test_user_threads_have_native_id in test_threading.py. For some # reason, when the Span is created, it's not linked to the MainThread, and the