feat(profiling): add experimental outside-in stack sampling method (s…

…tack v2) (#8471) This allows users to optionally use a new stack sampling technique. Briefly, * Avoids issues in Python 3.11 and later where the C struct backing Python frame objects may contain invalid pointers (causing segfaults within standard API calls) * Addresses a bias in the "normal" stack sampler where we had a tendency to sample threads only at the points where they transitioned off-GIL * Hopefully removes some serialization overhead, although in the first few versions the overall consumption of this technique may be higher. There are a few tests around the new code, those will be added to CI in a later update. ## Checklist - [x] Change(s) are motivated and described in the PR description - [x] Testing strategy is described if automated tests are not included in the PR - [x] Risks are described (performance impact, potential for breakage, maintainability) - [x] Change is maintainable (easy to change, telemetry, documentation) - [x] [Library release note guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html) are followed or label `changelog/no-changelog` is set - [x] Documentation is included (in-code, generated user docs, [public corp docs](https://github.com/DataDog/documentation/)) - [x] Backport labels are set (if [applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)) - [x] If this PR changes the public interface, I've notified `@DataDog/apm-tees`. - [x] If change touches code that signs or publishes builds or packages, or handles credentials of any kind, I've requested a review from `@DataDog/security-design-and-guidance`. ## Reviewer Checklist - [x] Title is accurate - [x] All changes are related to the pull request's stated goal - [x] Description motivates each change - [x] Avoids breaking [API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces) changes - [x] Testing strategy adequately addresses listed risks - [x] Change is maintainable (easy to change, telemetry, documentation) - [x] Release note makes sense to a user of the library - [x] Author has acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment - [x] 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) --------- Co-authored-by: sanchda <[email protected]> Co-authored-by: Gabriele N. Tornetta <[email protected]> Co-authored-by: Tahir H. Butt <[email protected]>
DataDog · Mar 5, 2024 · 33d0af1 · 33d0af1
1 parent d5445a7
commit 33d0af1
Show file tree

Hide file tree

Showing 44 changed files with 1,333 additions and 582 deletions.
diff --git a/.gitignore b/.gitignore
@@ -13,8 +13,7 @@ ddtrace/profiling/collector/_traceback.c
 ddtrace/profiling/collector/stack.c
 ddtrace/profiling/exporter/pprof.c
 ddtrace/profiling/_build.c
-ddtrace/internal/datadog/profiling/ddup.cpp
-ddtrace/internal/datadog/profiling/_ddup.cpp
+ddtrace/internal/datadog/profiling/ddup/_ddup.cpp
 ddtrace/internal/_encoding.c
 ddtrace/internal/_rand.c
 ddtrace/internal/_tagset.c

diff --git a/ddtrace/internal/datadog/profiling/cmake/AnalysisFunc.cmake b/ddtrace/internal/datadog/profiling/cmake/AnalysisFunc.cmake
@@ -0,0 +1,31 @@
+include(CheckIPOSupported)
+
+function(add_ddup_config target)
+    target_compile_options(${target} PRIVATE
+      "$<$<CONFIG:Debug>:-Og;-ggdb3>"
+      "$<$<CONFIG:Release>:-Os>"
+      -ffunction-sections -fno-semantic-interposition -Wall -Werror -Wextra -Wshadow -Wnon-virtual-dtor -Wold-style-cast
+    )
+    target_link_options(${target} PRIVATE
+      "$<$<CONFIG:Release>:-s>"
+      -Wl,--as-needed -Wl,-Bsymbolic-functions -Wl,--gc-sections
+    )
+    set_property(TARGET ${target} PROPERTY INTERPROCEDURAL_OPTIMIZATION TRUE)
+    check_ipo_supported(RESULT result)
+    if (result)
+      set_property(TARGET ${target} PROPERTY INTERPROCEDURAL_OPTIMIZATION TRUE)
+    endif()
+
+    # Propagate sanitizers
+    if (SANITIZE_OPTIONS)
+        # Some sanitizers (or the analysis--such as symbolization--tooling thereof) work better with frame
+        # pointers, so we include it here.
+        target_compile_options(${target} PRIVATE -fsanitize=${SANITIZE_OPTIONS} -fno-omit-frame-pointer)
+        target_link_options(${target} PRIVATE -fsanitize=${SANITIZE_OPTIONS})
+    endif()
+
+    # If DO_FANALYZER is specified and we're using gcc, then we can use -fanalyzer
+    if (DO_FANALYZER AND CMAKE_CXX_COMPILER_ID MATCHES "GNU")
+      target_compile_options(${target} PRIVATE -fanalyzer)
+    endif()
+endfunction()
diff --git a/ddtrace/internal/datadog/profiling/cmake/FindCppcheck.cmake b/ddtrace/internal/datadog/profiling/cmake/FindCppcheck.cmake
@@ -1,3 +1,8 @@
+# Only add this project if it doesn't already exist
+if (TARGET cppcheck_project)
+    return()
+endif()
+
 include(ExternalProject)
 
 # Build cppcheck from sources
@@ -17,10 +22,20 @@ endif()
 
 # This function will add a cppcheck target for a given directory
 # unless DO_CPPCHECK is set to false, then it will add a target that does nothing
-function(add_cppcheck_target NAME DIRECTORY)
+function(add_cppcheck_target)
+  # Parse additional arguments as lists
+  set(options)
+  set(oneValueArgs TARGET)
+  set(multiValueArgs INCLUDE SRC)
+  cmake_parse_arguments(PARSE_ARGV 0 ARG "${options}" "${oneValueArgs}" "${multiValueArgs}")
+
+  # Automatically generate the cppcheck target name
+  set(NAME "cppcheck_dd_${ARGV0}")
+
   if (DO_CPPCHECK)
-    add_custom_target("${NAME}"
-      COMMAND ${CPPCHECK_EXECUTABLE}
+    # Initialize command variable
+    set(cppcheck_cmd
+      ${CPPCHECK_EXECUTABLE}
       --enable=all
       --addon=threadsafety.py
       --addon=misc
@@ -31,18 +46,31 @@ function(add_cppcheck_target NAME DIRECTORY)
       --suppress=missingIncludeSystem
       --inline-suppr
       --error-exitcode=1
-      -I ${CMAKE_SOURCE_DIR}/include
-      -I ${Datadog_INCLUDE_DIRS}
-      ${CMAKE_SOURCE_DIR}/src
-      WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}
-      COMMENT "Running cppcheck"
     )
-    add_dependencies(cppcheck_runner "${NAME}")
+
+    # Append include directories to the command
+    foreach(INCLUDE_DIR ${ARG_INCLUDE})
+      list(APPEND cppcheck_cmd -I ${INCLUDE_DIR})
+    endforeach()
+
+    # Append source directories/files to the command
+    foreach(SRC_FILE ${ARG_SRC})
+      list(APPEND cppcheck_cmd ${SRC_FILE})
+    endforeach()
+
+    # Define the custom target with the constructed command
+    add_custom_target(${NAME}
+      COMMAND ${cppcheck_cmd}
+      COMMENT "Running cppcheck on ${ARGV0}"
+    )
+
+    # Make the cppcheck target a dependent of the specified target
+    add_dependencies(${ARGV0} ${NAME})
   else()
-    # Need to make this function work, but kinda do nothing
-    add_custom_target("${NAME}"
-      COMMAND echo "building ${NAME} without cppcheck"
-      COMMENT "cppcheck is disabled"
+    # Define a do-nothing target if cppcheck is disabled
+    add_custom_target(${NAME}
+      COMMAND echo "Cppcheck target ${NAME} is disabled."
+      COMMENT "cppcheck is disabled for ${ARGV0}"
     )
   endif()
 endfunction()
diff --git a/ddtrace/internal/datadog/profiling/cmake/FindLibdatadog.cmake b/ddtrace/internal/datadog/profiling/cmake/FindLibdatadog.cmake
@@ -1,7 +1,14 @@
+# Only add this project if Datadog::Profiling is not already defined
+if(TARGET Datadog::Profiling)
+    return()
+endif()
+
+include(ExternalProject)
 set(TAG_LIBDATADOG
     "v5.0.0"
     CACHE STRING "libdatadog github tag")
 
+set(Datadog_BUILD_DIR ${CMAKE_BINARY_DIR}/libdatadog)
 set(Datadog_ROOT ${Datadog_BUILD_DIR}/libdatadog-${TAG_LIBDATADOG})
 
 message(STATUS "${CMAKE_CURRENT_LIST_DIR}/tools/fetch_libdatadog.sh ${TAG_LIBDATADOG} ${Datadog_ROOT}")
@@ -15,7 +22,7 @@ set(Datadog_DIR "${Datadog_ROOT}/cmake")
 
 # Prefer static library to shared library
 set(CMAKE_FIND_LIBRARY_SUFFIXES_BACKUP ${CMAKE_FIND_LIBRARY_SUFFIXES})
-set(CMAKE_FIND_LIBRARY_SUFFIXES .a .so)
+set(CMAKE_FIND_LIBRARY_SUFFIXES .a)
 
 find_package(Datadog REQUIRED)
 

diff --git a/ddtrace/internal/datadog/profiling/dd_wrapper/CMakeLists.txt b/ddtrace/internal/datadog/profiling/dd_wrapper/CMakeLists.txt
@@ -4,37 +4,19 @@ project(dd_wrapper
     LANGUAGES CXX
 )
 
-# Infer some basic properties about the build.  This is necessary because multiple
-# extensions reuse this cmake file, so we need its artifacts to go in a consistent
-# place
-get_filename_component(dd_wrapper_BUILD_PARENT "${CMAKE_BINARY_DIR}/../.." ABSOLUTE)
-set(dd_wrapper_BUILD_DIR "${dd_wrapper_BUILD_PARENT}/ddtrace.internal.datadog.profiling")
+# Build in a predictable location.  This is needed for setup.py
+get_filename_component(dd_wrapper_BUILD_DIR "${CMAKE_CURRENT_SOURCE_DIR}/../ddtrace.internal.datadog.profiling" ABSOLUTE)
 
 # Custom modules are in the parent directory
 list(APPEND CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/../cmake")
 
-# Setup state for FindLibdatadog
-set(Datadog_BUILD_DIR "${dd_wrapper_BUILD_PARENT}/libdatadog")
-
 # Includes
-include(CMakePackageConfigHelpers) # For generating the config file
 include(FetchContent)
 include(ExternalProject)
 include(FindLibdatadog)
+include(AnalysisFunc)
 include(FindCppcheck)
 
-# Send some of these vars back up to the parent
-if (DD_WRAPPER_IS_SUBPROJECT)
-  set(Datadog_INCLUDE_DIRS "${Datadog_INCLUDE_DIRS}" PARENT_SCOPE)
-  set(dd_wrapper_BUILD_DIR "${dd_wrapper_BUILD_PARENT}/ddtrace.internal.datadog.profiling" PARENT_SCOPE)
-endif()
-
-add_compile_options(
-  "$<$<CONFIG:Debug>:-Og;-ggdb3>"
-  "$<$<CONFIG:Release>:-Os -s>"
-  -fno-semantic-interposition -Wall -Werror -Wextra -Wshadow -Wnon-virtual-dtor -Wold-style-cast
-)
-
 # Library sources
 add_library(dd_wrapper SHARED
     src/uploader_builder.cpp
@@ -45,19 +27,17 @@ add_library(dd_wrapper SHARED
     src/interface.cpp
 )
 
+# Add common configuration flags
+add_ddup_config(dd_wrapper)
+
 # At present, C++17 is chosen as the minimum standard. This conflicts with the
 # manylinux2014 standard upon which we must rely. We'll have to statically link
 # libstdc++ to avoid this conflict, but need to remain mindful of symbol visibility
 # and overall binary size.
 target_compile_features(dd_wrapper PUBLIC cxx_std_17)
 
-# Set some linker options
-target_compile_options(dd_wrapper PRIVATE -fno-semantic-interposition)
-target_compile_options(dd_wrapper PRIVATE -ffunction-sections -fdata-sections)
+# Statically link libstdc++ to avoid manylinux2014 conflicts
 target_link_options(dd_wrapper PRIVATE -static-libstdc++)
-target_link_options(dd_wrapper PRIVATE -Wl,-Bsymbolic-functions)
-target_link_options(dd_wrapper PRIVATE -Wl,--gc-sections)
-set_property(TARGET dd_wrapper PROPERTY INTERPROCEDURAL_OPTIMIZATION TRUE)
 
 target_include_directories(dd_wrapper PRIVATE
     include
@@ -68,35 +48,22 @@ target_link_libraries(dd_wrapper PRIVATE
 )
 set_target_properties(dd_wrapper PROPERTIES POSITION_INDEPENDENT_CODE ON)
 
-set_target_properties(dd_wrapper
-    PROPERTIES
-    LIBRARY_OUTPUT_DIRECTORY "${dd_wrapper_BUILD_DIR}"
-)
-
-# If DO_FANALYZER is specified and we're using gcc, then we can use -fanalyzer
-if (DO_FANALYZER AND CMAKE_CXX_COMPILER_ID MATCHES "GNU")
-  target_compile_options(dd_wrapper PRIVATE -fanalyzer)
-endif()
-
-# If DO_CPPCHECK is specified, then we can use cppcheck
-add_cppcheck_target(cppcheck_dd_wrapper ${CMAKE_CURRENT_SOURCE_DIR})
-
-# Propagate sanitizers
-if (SANITIZE_OPTIONS)
-    # Some sanitizers (or the analysis--such as symbolization--tooling thereof) work better with frame
-    # pointers, so we include it here.
-    target_compile_options(dd_wrapper PRIVATE -fsanitize=${SANITIZE_OPTIONS} -fno-omit-frame-pointer)
-    target_link_options(dd_wrapper PRIVATE -fsanitize=${SANITIZE_OPTIONS})
+# If LIB_INSTALL_DIR is set, install the library.
+# Install one directory up--both ddup and stackv2 are set to the same relative level.
+if (LIB_INSTALL_DIR)
+    install(TARGETS dd_wrapper
+        LIBRARY DESTINATION ${LIB_INSTALL_DIR}/..
+        ARCHIVE DESTINATION ${LIB_INSTALL_DIR}/..
+        RUNTIME DESTINATION ${LIB_INSTALL_DIR}/..
+    )
 endif()
 
-# If dd_wrapper_INSTALL_DIR is specified by the parent, use it
-if (NOT dd_wrapper_INSTALL_DIR)
-  # If not, then just keep it in the build directory
-  set(dd_wrapper_INSTALL_DIR "${CMAKE_BINARY_DIR}/lib")
-endif()
-message(STATUS "dd_wrapper_INSTALL_DIR: ${dd_wrapper_INSTALL_DIR}")
-install(TARGETS dd_wrapper
-    DESTINATION ${dd_wrapper_INSTALL_DIR}
+# Configure cppcheck
+add_cppcheck_target(dd_wrapper
+    DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}
+    INCLUDE ${CMAKE_CURRENT_SOURCE_DIR}/include
+            ${Datadog_INCLUDE_DIRS}
+    SRC ${CMAKE_CURRENT_SOURCE_DIR}/src
 )
 
 # Add the tests

diff --git a/ddtrace/internal/datadog/profiling/dd_wrapper/include/interface.hpp b/ddtrace/internal/datadog/profiling/dd_wrapper/include/interface.hpp
@@ -2,6 +2,7 @@
 
 #include <stddef.h>
 #include <stdint.h>
+#include <string_view>
 
 // Forward decl of the return pointer
 namespace Datadog {
@@ -12,21 +13,21 @@ class Sample;
 extern "C"
 {
 #endif
-    void ddup_config_env(const char* dd_env);
-    void ddup_config_service(const char* service);
-    void ddup_config_version(const char* version);
-    void ddup_config_runtime(const char* runtime);
-    void ddup_config_runtime_version(const char* runtime_version);
-    void ddup_config_profiler_version(const char* profiler_version);
-    void ddup_config_url(const char* url);
+    void ddup_config_env(std::string_view dd_env);
+    void ddup_config_service(std::string_view service);
+    void ddup_config_version(std::string_view version);
+    void ddup_config_runtime(std::string_view runtime);
+    void ddup_config_runtime_version(std::string_view runtime_version);
+    void ddup_config_profiler_version(std::string_view profiler_version);
+    void ddup_config_url(std::string_view url);
     void ddup_config_max_nframes(int max_nframes);
 
-    void ddup_config_user_tag(const char* key, const char* val);
+    void ddup_config_user_tag(std::string_view key, std::string_view val);
     void ddup_config_sample_type(unsigned int type);
 
     bool ddup_is_initialized();
     void ddup_init();
-    void ddup_set_runtime_id(const char* id, size_t sz);
+    void ddup_set_runtime_id(std::string_view id);
     bool ddup_upload();
 
     // Proxy functions to the underlying sample
@@ -37,22 +38,22 @@ extern "C"
     void ddup_push_release(Datadog::Sample* sample, int64_t release_time, int64_t count);
     void ddup_push_alloc(Datadog::Sample* sample, uint64_t size, uint64_t count);
     void ddup_push_heap(Datadog::Sample* sample, uint64_t size);
-    void ddup_push_lock_name(Datadog::Sample* sample, const char* lock_name);
+    void ddup_push_lock_name(Datadog::Sample* sample, std::string_view lock_name);
     void ddup_push_threadinfo(Datadog::Sample* sample,
                               int64_t thread_id,
                               int64_t thread_native_id,
-                              const char* thread_name);
+                              std::string_view thread_name);
     void ddup_push_task_id(Datadog::Sample* sample, int64_t task_id);
-    void ddup_push_task_name(Datadog::Sample* sample, const char* task_name);
+    void ddup_push_task_name(Datadog::Sample* sample, std::string_view task_name);
     void ddup_push_span_id(Datadog::Sample* sample, int64_t span_id);
     void ddup_push_local_root_span_id(Datadog::Sample* sample, int64_t local_root_span_id);
-    void ddup_push_trace_type(Datadog::Sample* sample, const char* trace_type);
-    void ddup_push_trace_resource_container(Datadog::Sample* sample, const char* trace_resource_container);
-    void ddup_push_exceptioninfo(Datadog::Sample* sample, const char* exception_type, int64_t count);
-    void ddup_push_class_name(Datadog::Sample* sample, const char* class_name);
+    void ddup_push_trace_type(Datadog::Sample* sample, std::string_view trace_type);
+    void ddup_push_trace_resource_container(Datadog::Sample* sample, std::string_view trace_resource_container);
+    void ddup_push_exceptioninfo(Datadog::Sample* sample, std::string_view exception_type, int64_t count);
+    void ddup_push_class_name(Datadog::Sample* sample, std::string_view class_name);
     void ddup_push_frame(Datadog::Sample* sample,
-                         const char* _name,
-                         const char* _filename,
+                         std::string_view _name,
+                         std::string_view _filename,
                          uint64_t address,
                          int64_t line);
     void ddup_flush_sample(Datadog::Sample* sample);

diff --git a/ddtrace/internal/datadog/profiling/dd_wrapper/include/sample.hpp b/ddtrace/internal/datadog/profiling/dd_wrapper/include/sample.hpp
@@ -39,9 +39,6 @@ class Sample
     std::vector<int64_t> values = {};
 
   public:
-    // Initialization and stuff
-    void start_sample();
-
     // Helpers
     bool push_label(ExportLabelKey key, std::string_view val);
     bool push_label(ExportLabelKey key, int64_t val);