From 31d079bd2ec9561d863b1953b91fc52910774560 Mon Sep 17 00:00:00 2001 From: sanchda Date: Wed, 21 Feb 2024 15:55:48 +0000 Subject: [PATCH 01/76] Refactor ddup into a peer directory --- ddtrace/internal/datadog/profiling/{ => ddup}/CMakeLists.txt | 0 ddtrace/internal/datadog/profiling/{ => ddup}/__init__.py | 0 ddtrace/internal/datadog/profiling/{ => ddup}/_ddup.pyi | 0 ddtrace/internal/datadog/profiling/{ => ddup}/_ddup.pyx | 0 ddtrace/internal/datadog/profiling/{ => ddup}/ddup.py | 0 ddtrace/internal/datadog/profiling/{ => ddup}/utils.py | 0 6 files changed, 0 insertions(+), 0 deletions(-) rename ddtrace/internal/datadog/profiling/{ => ddup}/CMakeLists.txt (100%) rename ddtrace/internal/datadog/profiling/{ => ddup}/__init__.py (100%) rename ddtrace/internal/datadog/profiling/{ => ddup}/_ddup.pyi (100%) rename ddtrace/internal/datadog/profiling/{ => ddup}/_ddup.pyx (100%) rename ddtrace/internal/datadog/profiling/{ => ddup}/ddup.py (100%) rename ddtrace/internal/datadog/profiling/{ => ddup}/utils.py (100%) diff --git a/ddtrace/internal/datadog/profiling/CMakeLists.txt b/ddtrace/internal/datadog/profiling/ddup/CMakeLists.txt similarity index 100% rename from ddtrace/internal/datadog/profiling/CMakeLists.txt rename to ddtrace/internal/datadog/profiling/ddup/CMakeLists.txt diff --git a/ddtrace/internal/datadog/profiling/__init__.py b/ddtrace/internal/datadog/profiling/ddup/__init__.py similarity index 100% rename from ddtrace/internal/datadog/profiling/__init__.py rename to ddtrace/internal/datadog/profiling/ddup/__init__.py diff --git a/ddtrace/internal/datadog/profiling/_ddup.pyi b/ddtrace/internal/datadog/profiling/ddup/_ddup.pyi similarity index 100% rename from ddtrace/internal/datadog/profiling/_ddup.pyi rename to ddtrace/internal/datadog/profiling/ddup/_ddup.pyi diff --git a/ddtrace/internal/datadog/profiling/_ddup.pyx b/ddtrace/internal/datadog/profiling/ddup/_ddup.pyx similarity index 100% rename from ddtrace/internal/datadog/profiling/_ddup.pyx rename to ddtrace/internal/datadog/profiling/ddup/_ddup.pyx diff --git a/ddtrace/internal/datadog/profiling/ddup.py b/ddtrace/internal/datadog/profiling/ddup/ddup.py similarity index 100% rename from ddtrace/internal/datadog/profiling/ddup.py rename to ddtrace/internal/datadog/profiling/ddup/ddup.py diff --git a/ddtrace/internal/datadog/profiling/utils.py b/ddtrace/internal/datadog/profiling/ddup/utils.py similarity index 100% rename from ddtrace/internal/datadog/profiling/utils.py rename to ddtrace/internal/datadog/profiling/ddup/utils.py From 6d91204ba9a29c6b4239af294766627ddda10d1a Mon Sep 17 00:00:00 2001 From: sanchda Date: Wed, 21 Feb 2024 15:56:46 +0000 Subject: [PATCH 02/76] Factor module more cleanly --- .../datadog/profiling/ddup/__init__.py | 87 +++++++++++++++++++ .../internal/datadog/profiling/ddup/ddup.py | 87 ------------------- setup.py | 2 +- 3 files changed, 88 insertions(+), 88 deletions(-) delete mode 100644 ddtrace/internal/datadog/profiling/ddup/ddup.py diff --git a/ddtrace/internal/datadog/profiling/ddup/__init__.py b/ddtrace/internal/datadog/profiling/ddup/__init__.py index e69de29bb2d..8f5ad4fe0b0 100644 --- a/ddtrace/internal/datadog/profiling/ddup/__init__.py +++ b/ddtrace/internal/datadog/profiling/ddup/__init__.py @@ -0,0 +1,87 @@ +from .utils import sanitize_string # noqa: F401 + + +try: + from ._ddup import * # noqa: F403, F401 +except Exception: + from typing import Dict # noqa:F401 + from typing import Optional # noqa:F401 + + from ddtrace._trace.span import Span # noqa:F401 + + # Decorator for not-implemented + def not_implemented(func): + def wrapper(*args, **kwargs): + raise NotImplementedError("{} is not implemented on this platform".format(func.__name__)) + + @not_implemented + def init( + env, # type: Optional[str] + service, # type: Optional[str] + version, # type: Optional[str] + tags, # type: Optional[Dict[str, str]] + max_nframes, # type: Optional[int] + url, # type: Optional[str] + ): + pass + + @not_implemented + def upload(): # type: () -> None + pass + + class SampleHandle: + @not_implemented + def push_cputime(self, value, count): # type: (int, int) -> None + pass + + @not_implemented + def push_walltime(self, value, count): # type: (int, int) -> None + pass + + @not_implemented + def push_acquire(self, value, count): # type: (int, int) -> None + pass + + @not_implemented + def push_release(self, value, count): # type: (int, int) -> None + pass + + @not_implemented + def push_alloc(self, value, count): # type: (int, int) -> None + pass + + @not_implemented + def push_heap(self, value): # type: (int) -> None + pass + + @not_implemented + def push_lock_name(self, lock_name): # type: (str) -> None + pass + + @not_implemented + def push_frame(self, name, filename, address, line): # type: (str, str, int, int) -> None + pass + + @not_implemented + def push_threadinfo(self, thread_id, thread_native_id, thread_name): # type: (int, int, Optional[str]) -> None + pass + + @not_implemented + def push_taskinfo(self, task_id, task_name): # type: (int, str) -> None + pass + + @not_implemented + def push_exceptioninfo(self, exc_type, count): # type: (type, int) -> None + pass + + @not_implemented + def push_class_name(self, class_name): # type: (str) -> None + pass + + @not_implemented + def push_span(self, span, endpoint_collection_enabled): # type: (Optional[Span], bool) -> None + pass + + @not_implemented + def flush_sample(self): # type: () -> None + pass diff --git a/ddtrace/internal/datadog/profiling/ddup/ddup.py b/ddtrace/internal/datadog/profiling/ddup/ddup.py deleted file mode 100644 index 8f5ad4fe0b0..00000000000 --- a/ddtrace/internal/datadog/profiling/ddup/ddup.py +++ /dev/null @@ -1,87 +0,0 @@ -from .utils import sanitize_string # noqa: F401 - - -try: - from ._ddup import * # noqa: F403, F401 -except Exception: - from typing import Dict # noqa:F401 - from typing import Optional # noqa:F401 - - from ddtrace._trace.span import Span # noqa:F401 - - # Decorator for not-implemented - def not_implemented(func): - def wrapper(*args, **kwargs): - raise NotImplementedError("{} is not implemented on this platform".format(func.__name__)) - - @not_implemented - def init( - env, # type: Optional[str] - service, # type: Optional[str] - version, # type: Optional[str] - tags, # type: Optional[Dict[str, str]] - max_nframes, # type: Optional[int] - url, # type: Optional[str] - ): - pass - - @not_implemented - def upload(): # type: () -> None - pass - - class SampleHandle: - @not_implemented - def push_cputime(self, value, count): # type: (int, int) -> None - pass - - @not_implemented - def push_walltime(self, value, count): # type: (int, int) -> None - pass - - @not_implemented - def push_acquire(self, value, count): # type: (int, int) -> None - pass - - @not_implemented - def push_release(self, value, count): # type: (int, int) -> None - pass - - @not_implemented - def push_alloc(self, value, count): # type: (int, int) -> None - pass - - @not_implemented - def push_heap(self, value): # type: (int) -> None - pass - - @not_implemented - def push_lock_name(self, lock_name): # type: (str) -> None - pass - - @not_implemented - def push_frame(self, name, filename, address, line): # type: (str, str, int, int) -> None - pass - - @not_implemented - def push_threadinfo(self, thread_id, thread_native_id, thread_name): # type: (int, int, Optional[str]) -> None - pass - - @not_implemented - def push_taskinfo(self, task_id, task_name): # type: (int, str) -> None - pass - - @not_implemented - def push_exceptioninfo(self, exc_type, count): # type: (type, int) -> None - pass - - @not_implemented - def push_class_name(self, class_name): # type: (str) -> None - pass - - @not_implemented - def push_span(self, span, endpoint_collection_enabled): # type: (Optional[Span], bool) -> None - pass - - @not_implemented - def flush_sample(self): # type: () -> None - pass diff --git a/setup.py b/setup.py index 4aad92ad702..88f3cb0490b 100644 --- a/setup.py +++ b/setup.py @@ -461,7 +461,7 @@ def get_exts_for(name): if platform.system() == "Linux" and is_64_bit_python(): ext_modules.append( CMakeExtension( - "ddtrace.internal.datadog.profiling._ddup", + "ddtrace.internal.datadog.profiling.ddup._ddup", source_dir=DDUP_DIR, optional=True, cmake_args=[ From c7f1cc424556d0015ba0ccb47cca58e07fd6d653 Mon Sep 17 00:00:00 2001 From: sanchda Date: Wed, 21 Feb 2024 15:58:50 +0000 Subject: [PATCH 03/76] Add V2 sampling code, some other minor refactors --- .../profiling/cmake/AnalysisFunc.cmake | 30 +++ .../profiling/cmake/FindLibdatadog.cmake | 7 + .../profiling/dd_wrapper/CMakeLists.txt | 64 +---- .../profiling/dd_wrapper/setup_custom.sh | 234 +++++++++++++----- .../profiling/dd_wrapper/test/CMakeLists.txt | 15 +- .../profiling/dd_wrapper/test/forking.cpp | 4 +- .../datadog/profiling/ddup/CMakeLists.txt | 15 +- .../datadog/profiling/ddup/__init__.py | 2 +- .../datadog/profiling/stack_v2/CMakeLists.txt | 95 +++++++ .../profiling/stack_v2/include/constants.hpp | 8 + .../stack_v2/include/python_headers.hpp | 7 + .../profiling/stack_v2/include/sampler.hpp | 47 ++++ .../stack_v2/include/stack_renderer.hpp | 39 +++ .../stack_v2/include/util/cast_to_pyfunc.hpp | 11 + .../include/vendored/utf8_validate.hpp | 34 +++ .../profiling/stack_v2/src/sampler.cpp | 112 +++++++++ .../profiling/stack_v2/src/stack_renderer.cpp | 89 +++++++ .../profiling/stack_v2/src/stack_v2.cpp | 73 ++++++ .../profiling/stack_v2/src/stackv2.cpp | 62 +++++ setup.py | 23 +- 20 files changed, 827 insertions(+), 144 deletions(-) create mode 100644 ddtrace/internal/datadog/profiling/cmake/AnalysisFunc.cmake create mode 100644 ddtrace/internal/datadog/profiling/stack_v2/CMakeLists.txt create mode 100644 ddtrace/internal/datadog/profiling/stack_v2/include/constants.hpp create mode 100644 ddtrace/internal/datadog/profiling/stack_v2/include/python_headers.hpp create mode 100644 ddtrace/internal/datadog/profiling/stack_v2/include/sampler.hpp create mode 100644 ddtrace/internal/datadog/profiling/stack_v2/include/stack_renderer.hpp create mode 100644 ddtrace/internal/datadog/profiling/stack_v2/include/util/cast_to_pyfunc.hpp create mode 100644 ddtrace/internal/datadog/profiling/stack_v2/include/vendored/utf8_validate.hpp create mode 100644 ddtrace/internal/datadog/profiling/stack_v2/src/sampler.cpp create mode 100644 ddtrace/internal/datadog/profiling/stack_v2/src/stack_renderer.cpp create mode 100644 ddtrace/internal/datadog/profiling/stack_v2/src/stack_v2.cpp create mode 100644 ddtrace/internal/datadog/profiling/stack_v2/src/stackv2.cpp diff --git a/ddtrace/internal/datadog/profiling/cmake/AnalysisFunc.cmake b/ddtrace/internal/datadog/profiling/cmake/AnalysisFunc.cmake new file mode 100644 index 00000000000..804538d218b --- /dev/null +++ b/ddtrace/internal/datadog/profiling/cmake/AnalysisFunc.cmake @@ -0,0 +1,30 @@ +include(FindCppcheck) + +function(add_ddup_config target) + target_compile_options(${target} PRIVATE + "$<$:-Og -ggdb3>" + "$<$:-Os>" + -ffunction-sections -fno-semantic-interposition -Wall -Werror -Wextra -Wshadow -Wnon-virtual-dtor -Wold-style-cast + ) + target_link_options(${target} PRIVATE + "$<$:-s>" + -Wl,--as-needed -Wl,-Bsymbolic-functions -Wl,--gc-sections + ) + set_property(TARGET ${target} PROPERTY INTERPROCEDURAL_OPTIMIZATION TRUE) + + # 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() + + # If DO_CPPCHECK is specified, then we can use cppcheck + add_cppcheck_target(cppcheck_dd_${target} ${CMAKE_CURRENT_SOURCE_DIR}) +endfunction() diff --git a/ddtrace/internal/datadog/profiling/cmake/FindLibdatadog.cmake b/ddtrace/internal/datadog/profiling/cmake/FindLibdatadog.cmake index cb327c187cc..cad6b66d021 100644 --- 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_CURRENT_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}") diff --git a/ddtrace/internal/datadog/profiling/dd_wrapper/CMakeLists.txt b/ddtrace/internal/datadog/profiling/dd_wrapper/CMakeLists.txt index 90ad5cccb39..1075ab25104 100644 --- a/ddtrace/internal/datadog/profiling/dd_wrapper/CMakeLists.txt +++ b/ddtrace/internal/datadog/profiling/dd_wrapper/CMakeLists.txt @@ -4,36 +4,17 @@ 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(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( - "$<$:-Og;-ggdb3>" - "$<$:-Os -s>" - -fno-semantic-interposition -Wall -Werror -Wextra -Wshadow -Wnon-virtual-dtor -Wold-style-cast -) +include(AnalysisFunc) # Library sources add_library(dd_wrapper SHARED @@ -45,19 +26,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 @@ -73,32 +52,13 @@ set_target_properties(dd_wrapper 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) +# If LIB_INSTALL_DIR is set, install the library +if (LIB_INSTALL_DIR) + install(TARGETS dd_wrapper + DESTINATION "${LIB_INSTALL_DIR}" + ) 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}) -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} -) - # Add the tests if (BUILD_TESTING) enable_testing() diff --git a/ddtrace/internal/datadog/profiling/dd_wrapper/setup_custom.sh b/ddtrace/internal/datadog/profiling/dd_wrapper/setup_custom.sh index 255747651e3..34452736b9f 100755 --- a/ddtrace/internal/datadog/profiling/dd_wrapper/setup_custom.sh +++ b/ddtrace/internal/datadog/profiling/dd_wrapper/setup_custom.sh @@ -1,9 +1,9 @@ #!/bin/bash -# This script is used to build the library independently of the setup.py -# build system. This is mostly used to integrate different analysis tools -# which I haven't promoted into our normal builds yet. - +### Useful globals +MY_DIR=$(dirname $(realpath $0)) +MY_NAME="$0" +BUILD_DIR="build" ### Compiler discovery # Initialize variables to store the highest versions @@ -40,22 +40,33 @@ find_highest_compiler_version clang highest_clang find_highest_compiler_version clang++ highest_clangxx ### Build setup -SANITIZE_OPTIONS="" # passed directly to cmake -SAFETY_OPTIONS="address,leak,undefined" -THREAD_OPTIONS="thread" -NUMERICAL_OPTIONS="integer,nullability,signed-integer-overflow,bounds,float-divide-by-zero" -DATAFLOW_OPTIONS="dataflow" -MEMORY_OPTIONS="memory" -ANALYZE_OPTIONS="-fanalyzer" - -# helper function for setting the compiler -# Final cmake args +# Targets to target dirs +declare -A target_dirs +target_dirs["ddup"]="ddup" +target_dirs["stack_v2"]="stack_v2" +target_dirs["dd_wrapper"]="dd_wrapper" + +# Compiler options +declare -A compiler_args +compiler_args["safety"]="-DSANITIZE_OPTIONS=address,leak,undefined" +compiler_args["thread"]="-DSANITIZE_OPTIONS=thread" +compiler_args["numerical"]="-DSANITIZE_OPTIONS=integer,nullability,signed-integer-overflow,bounds,float-divide-by-zero" +compiler_args["dataflow"]="-DSANITIZE_OPTIONS=dataflow" +compiler_args["memory"]="-DSANITIZE_OPTIONS=memory" +compiler_args["fanalyzer"]="-DDO_FANALYZE=ON" +compiler_args["cppcheck"]="-DDO_CPPCHECK=ON" + +# Initial cmake args cmake_args=( -DCMAKE_EXPORT_COMPILE_COMMANDS=ON -DCMAKE_VERBOSE_MAKEFILE=ON - -DBUILD_TESTING=ON + -DLIB_INSTALL_DIR=$(realpath $MY_DIR)/lib ) +# Initial build targets; no matter what, dd_wrapper is the base dependency, so it's always built +targets=("dd_wrapper") + +# Helper functions for finding the compiler(s) set_clang() { export CC=$highest_clang export CXX=$highest_clangxx @@ -74,68 +85,110 @@ set_gcc() { ) } +### Build runners +run_cmake() { + target=$1 + dir=${target_dirs[$target]} + if [ -z "$dir" ]; then + echo "No directory specified for cmake" + exit 1 + fi + # Enter the directory and create the build directory + pushd $dir || { echo "Failed to change to $dir"; exit 1; } + mkdir -p $BUILD_DIR && cd $BUILD_DIR || { echo "Failed to create build directory for $dir"; exit 1; } + + # Run cmake + cmake "${cmake_args[@]}" .. || { echo "cmake failed"; exit 1; } + cmake --build . || { echo "build failed"; exit 1; } + if [[ " ${cmake_args[*]} " =~ " -DDO_CPPCHECK=ON " ]]; then + echo "--------------------------------------------------------------------- Running CPPCHECK" + make cppcheck || { echo "cppcheck failed"; exit 1; } + fi + if [[ " ${cmake_args[*]} " =~ " -DBUILD_TESTING=ON " ]]; then + echo "--------------------------------------------------------------------- Running Tests" +# make test || { echo "tests failed"; exit 1; } + ctest --output-on-failure || { echo "tests failed!"; exit 1; } + fi + + # OK, the build or whatever went fine I guess. + popd +} -# Check input -if [ -n "$1" ]; then +### Print help +print_help() { + echo "Usage: ${MY_NAME} [options] [build_mode] [target]" + echo "Options (one of)" + echo " -h, --help Show this help message and exit" + echo " -s, --safety Clang + " compile_args["safety"] + echo " -t, --thread Clang + " compile_args["thread"] + echo " -n, --numerical Clang + " compile_args["numerical"] + echo " -d, --dataflow Clang + " compile_args["dataflow"] + echo " -m --memory Clang + " compile_args["memory"] + echo " -C --cppcheck Clang + " compile_args["cppcheck"] + echo " -f, --fanalyze GCC + " compile_args["fanalyzer"] + echo " -c, --clang Clang (alone)" + echo " -g, --gcc GCC (alone)" + echo " -- Don't do anything special" + echo "" + echo "Build Modes:" + echo " Debug (default)" + echo " Release" + echo " RelWithDebInfo" + echo "" + echo "(any possible others, depending on what cmake supports for" + echo "BUILD_TYPE out of the box)" + echo "" + echo "Targets:" + echo " all" + echo " all_test (default)" + echo " dd_wrapper" + echo " dd_wrapper_test" + echo " stack_v2 (also builds dd_wrapper)" + echo " stack_v2_test (also builds dd_wrapper_test)" + echo " ddup (also builds dd_wrapper)" + echo " ddup_test (also builds dd_wrapper_test)" +} + +print_cmake_args() { + echo "CMake Args: ${cmake_args[*]}" + echo "Targets: ${targets[*]}" +} + +### Check input +# Check the first slot, options +add_compiler_args() { case "$1" in -h|--help) - echo "Usage: $0 [options] [build_mode]" - echo "Options (one of)" - echo " -h, --help Show this help message and exit" - echo " -s, --safety Clang + fsanitize=$SAFETY_OPTIONS" - echo " -t, --thread Clang + fsanitize=$THREAD_OPTIONS" - echo " -n, --numerical Clang + fsanitize=$NUMERICAL_OPTIONS" - echo " -d, --dataflow Clang + fsanitize=$DATAFLOW_OPTIONS" - echo " -m --memory Clang + fsanitize=$MEMORY_OPTIONS" - echo " -C --cppcheck Clang + cppcheck" - echo " -f, --fanalyze GCC + -fanalyzer" - echo " -c, --clang Clang (alone)" - echo " -g, --gcc GCC (alone)" - echo " -- Don't do anything special" - echo "" - echo "Build Modes:" - echo " Debug (default)" - echo " Release" - echo " RelWithDebInfo" - echo "" - echo "(any possible others, depending on what cmake supports for" - echo "BUILD_TYPE out of the box)" - echo "" - echo "-------------------------------------------------" - echo "Diagnostic parameters" - echo "Highest gcc: $highest_gcc" - echo "Highest g++: $highest_gxx" - echo "Highest Clang: $highest_clang" - echo "Highest Clang++: $highest_clangxx" + print_help exit 0 ;; -s|--safety) - cmake_args+=(-DSANITIZE_OPTIONS=$SAFETY_OPTIONS) + cmake_args+=compiler_args["safety"] set_clang ;; -t|--thread) - cmake_args+=(-DSANITIZE_OPTIONS=$THREAD_OPTIONS) + cmake_args+=compiler_args["thread"] set_clang ;; -n|--numerical) - cmake_args+=(-DSANITIZE_OPTIONS=$NUMERICAL_OPTIONS) + cmake_args+=compiler_args["numerical"] set_clang ;; -d|--dataflow) - cmake_args+=(-DSANITIZE_OPTIONS=$DATAFLOW_OPTIONS) + cmake_args+=compiler_args["dataflow"] set_clang ;; -m|--memory) - cmake_args+=(-DSANITIZE_OPTIONS=$MEMORY_OPTIONS) + cmake_args+=compiler_args["memory"] set_clang ;; -C|--cppcheck) - cmake_args+=(-DDO_CPPCHECK=ON) + cmake_args+=compiler_args["cppcheck"] set_clang ;; -f|--fanalyze) - cmake_args+=(-DDO_FANALYZE=ON) + cmake_args+=compiler_args["fanalyzer"] set_gcc ;; -c|--clang) @@ -145,28 +198,77 @@ if [ -n "$1" ]; then set_gcc ;; --) + set_gcc # Default to GCC, since this is what will happen in the official build ;; *) echo "Unknown option: $1" exit 1 ;; esac -else - set_gcc +} + +# Check the second slot, build mode +add_build_mode() { + case "$1" in + Debug|Release|RelWithDebInfo) + cmake_args+=(-DCMAKE_BUILD_TYPE=$1) + ;; + ""|--) + cmake_args+=(-DCMAKE_BUILD_TYPE=Debug) + ;; + *) + echo "Unknown build mode: $1" + exit 1 + ;; + esac +} + +# Check the third slot, target +add_target() { + arg=${1:-"all_test"} + if [[ "${arg}" =~ _test$ ]]; then + cmake_args+=(-DBUILD_TESTING=ON) + fi + target=${arg%_test} + + case "${target}" in + all|--) + targets+=("stack_v2") + targets+=("ddup") + ;; + dd_wrapper) + # We always build dd_wrapper, so no need to add it to the list + ;; + stack_v2) + targets+=("stack_v2") + ;; + ddup) + targets+=("ddup") + ;; + *) + echo "Unknown target: $1" + exit 1 + ;; + esac +} + + +### ENTRYPOINT +# Check for basic input validity +if [ $# -eq 0 ]; then + echo "No arguments given. At least one is needed, otherwise I'd (a m b i g u o u s l y) do a lot of work!" + print_help + exit 1 fi -# If there are two arguments, override build mode -BUILD_MODE=${2:-Debug} -cmake_args+=(-DCMAKE_BUILD_TYPE=$BUILD_MODE) +add_compiler_args "$1" +add_build_mode "$2" +add_target "$3" -# Setup cmake stuff -BUILD_DIR="build" -mkdir -p $BUILD_DIR && cd $BUILD_DIR || { echo "Failed to create build directory"; exit 1; } +# Print cmake args +print_cmake_args # Run cmake -cmake "${cmake_args[@]}" .. || { echo "cmake failed"; exit 1; } -cmake --build . || { echo "build failed"; exit 1; } -if [[ "$SANITIZE_OPTIONS" == "cppcheck" ]]; then - make cppcheck_run || { echo "cppcheck failed"; exit 1; } -fi -ctest --output-on-failure || { echo "tests failed!"; exit 1; } +for target in "${targets[@]}"; do + run_cmake $target +done diff --git a/ddtrace/internal/datadog/profiling/dd_wrapper/test/CMakeLists.txt b/ddtrace/internal/datadog/profiling/dd_wrapper/test/CMakeLists.txt index a2690c28c53..6163229a5df 100644 --- a/ddtrace/internal/datadog/profiling/dd_wrapper/test/CMakeLists.txt +++ b/ddtrace/internal/datadog/profiling/dd_wrapper/test/CMakeLists.txt @@ -8,6 +8,7 @@ set(gtest_force_shared_crt ON CACHE BOOL "" FORCE) set(INSTALL_GTEST OFF CACHE BOOL "" FORCE) FetchContent_MakeAvailable(googletest) include(GoogleTest) +include(AnalysisFunc) function(dd_wrapper_add_test name) add_executable(${name} ${ARGN}) @@ -18,23 +19,11 @@ function(dd_wrapper_add_test name) gtest_main dd_wrapper ) + add_ddup_config(dd_wrapper) - if (SANITIZE_OPTIONS) - if(NOT (SANITIZE_OPTIONS MATCHES "fanalyzer" OR SANITIZE_OPTIONS MATCHES "cppcheck")) - target_compile_options(${name} PRIVATE -fsanitize=${SANITIZE_OPTIONS}) - target_link_options(${name} PRIVATE -fsanitize=${SANITIZE_OPTIONS}) - endif() - endif() gtest_discover_tests(${name}) endfunction() -# Shouldn't be necessary, but whatever -add_compile_options( - "$<$:-Og;-ggdb3;-fno-omit-frame-pointer>" - "$<$:-O3>" - -Wall -Werror -Wextra -Wshadow -Wnon-virtual-dtor -Wold-style-cast -) - # Add the tests dd_wrapper_add_test(initialization initialization.cpp diff --git a/ddtrace/internal/datadog/profiling/dd_wrapper/test/forking.cpp b/ddtrace/internal/datadog/profiling/dd_wrapper/test/forking.cpp index 909ddb9422c..82a4f1c41e8 100644 --- a/ddtrace/internal/datadog/profiling/dd_wrapper/test/forking.cpp +++ b/ddtrace/internal/datadog/profiling/dd_wrapper/test/forking.cpp @@ -14,8 +14,8 @@ upload_in_thread() [[noreturn]] void profile_in_child(unsigned int num_threads, unsigned int run_time_ns, std::atomic& done) { - // Assumes setup has been called. Launch some samplers, wait, upload, exit. - // Managing interleaved execution is tricky, so the fork exits--rather than returns--when it is done. + // Assumes setup has been called. Launch some samplers, wait, upload, return. + // Exit because we don't want this process to exit std::vector ids; for (unsigned int i = 0; i < num_threads; i++) { ids.push_back(i); diff --git a/ddtrace/internal/datadog/profiling/ddup/CMakeLists.txt b/ddtrace/internal/datadog/profiling/ddup/CMakeLists.txt index 0570b9ab471..b5e963444bc 100644 --- a/ddtrace/internal/datadog/profiling/ddup/CMakeLists.txt +++ b/ddtrace/internal/datadog/profiling/ddup/CMakeLists.txt @@ -8,16 +8,17 @@ cmake_minimum_required(VERSION 3.19) set(EXTENSION_NAME "ddup" CACHE STRING "Name of the extension") project(${EXTENSION_NAME}) +# Get the cmake modules for this project +list(APPEND CMAKE_MODULE_PATH "${CMAKE_CURRENT_LIST_DIR}/../cmake") + # Includes include(FetchContent) include(ExternalProject) include(FindPython3) +include(FindLibdatadog) +include(AnalysisFunc) -# Build the dd_wrapper -# Need to specify that we're calling it via add_subdirectory, so that it knows to -# propagate the right variables to its parent scope -set(DD_WRAPPER_IS_SUBPROJECT ON) -add_subdirectory(dd_wrapper "${LIB_INSTALL_DIR}") +add_subdirectory(../dd_wrapper "${LIB_INSTALL_DIR}") # Find the Python interpreter find_package(Python3 COMPONENTS Interpreter REQUIRED) @@ -42,7 +43,7 @@ add_library(${EXTENSION_NAME} SHARED _ddup.cpp ) -target_compile_features(dd_wrapper PUBLIC cxx_std_17) +target_compile_features(${EXTENSION_NAME} PUBLIC cxx_std_17) # cmake may mutate the name of the library (e.g., lib- and -.so for dynamic libraries). # This suppresses that behavior, which is required to ensure all paths can be inferred @@ -56,7 +57,7 @@ set_target_properties(${EXTENSION_NAME} PROPERTIES INSTALL_RPATH "$ORIGIN") target_include_directories(${EXTENSION_NAME} PRIVATE include - dd_wrapper/include + ../dd_wrapper/include ${Datadog_INCLUDE_DIRS} ${Python3_INCLUDE_DIRS} ) diff --git a/ddtrace/internal/datadog/profiling/ddup/__init__.py b/ddtrace/internal/datadog/profiling/ddup/__init__.py index 8f5ad4fe0b0..133432e93ae 100644 --- a/ddtrace/internal/datadog/profiling/ddup/__init__.py +++ b/ddtrace/internal/datadog/profiling/ddup/__init__.py @@ -3,7 +3,7 @@ try: from ._ddup import * # noqa: F403, F401 -except Exception: +except Exception as e: from typing import Dict # noqa:F401 from typing import Optional # noqa:F401 diff --git a/ddtrace/internal/datadog/profiling/stack_v2/CMakeLists.txt b/ddtrace/internal/datadog/profiling/stack_v2/CMakeLists.txt new file mode 100644 index 00000000000..990dea179e7 --- /dev/null +++ b/ddtrace/internal/datadog/profiling/stack_v2/CMakeLists.txt @@ -0,0 +1,95 @@ +# The exact name of this extension determines aspects of the installation and build paths, +# which need to be kept in sync with setup.py. Accordingly, take the name passed in by the +# caller, defaulting to "stack_v2" if needed. +cmake_minimum_required(VERSION 3.19) +set(EXTENSION_NAME "stack_v2" CACHE STRING "Name of the extension") +project(${EXTENSION_NAME}) + +# Custom cmake modules are in the parent directory +list(APPEND CMAKE_MODULE_PATH "${CMAKE_CURRENT_LIST_DIR}/../cmake") + +# Includes +include(FetchContent) +include(ExternalProject) +include(FindPython3) +include(AnalysisFunc) +add_subdirectory(../dd_wrapper "${LIB_INSTALL_DIR}") + +# Find the Python interpreter +find_package(Python3 COMPONENTS Interpreter REQUIRED) +if (NOT Python3_INCLUDE_DIRS) + message(FATAL_ERROR "Python3_INCLUDE_DIRS not found") +endif() + +# Add echion +FetchContent_Declare( + echion + GIT_REPOSITORY "https://github.com/sanchda/echion.git" + GIT_TAG "sanchda/more_rendering" +) +FetchContent_GetProperties(echion) +if(NOT echion_POPULATED) + FetchContent_Populate(echion) +endif() + +# Specify the target C-extension that we want to build +add_library(${EXTENSION_NAME} SHARED + src/sampler.cpp + src/stack_renderer.cpp + src/stack_v2.cpp +) + +# Add common config +add_ddup_config(${EXTENSION_NAME}) + +# This project is build with C++17, even though the underlying repo adheres to the manylinux 2014 standard. +# This isn't currently a problem, but if it becomes one, we may have to structure the library differently. +target_compile_features(${EXTENSION_NAME} PUBLIC cxx_std_17) + +# Never build with native unwinding, since this is not currently used +target_compile_definitions(${EXTENSION_NAME} PRIVATE UNWIND_NATIVE_DISABLE) + +# Includes; echion and python are marked "system" to suppress warnings, +# but note in MSVC we'll have to #pragma warning(push, 0 then pop for the same effect. +message(ERROR "echion_SOURCE_DIR: ${echion_SOURCE_DIR}") +target_include_directories(${EXTENSION_NAME} PRIVATE + .. + include +) +target_include_directories(${EXTENSION_NAME} SYSTEM PRIVATE + ${echion_SOURCE_DIR} + ${Python3_INCLUDE_DIRS} + include/vendored + include/util +) + +# Echion sources need to be given the current platform +if(WIN32) + target_compile_definitions(${EXTENSION_NAME} PRIVATE PL_WINDOWS) +elseif(APPLE) + target_compile_definitions(${EXTENSION_NAME} PRIVATE PL_DARWIN) +elseif(UNIX) + target_compile_definitions(${EXTENSION_NAME} PRIVATE PL_LINUX) +endif() + +# cmake may mutate the name of the library (e.g., lib- and -.so for dynamic libraries). +# This suppresses that behavior, which is required to ensure all paths can be inferred +# correctly by setup.py. +set_target_properties(${EXTENSION_NAME} PROPERTIES PREFIX "") +set_target_properties(${EXTENSION_NAME} PROPERTIES SUFFIX "") +set_target_properties(${EXTENSION_NAME} PROPERTIES INSTALL_RPATH "$ORIGIN") # NB., gets installed to parent! + +target_link_libraries(${EXTENSION_NAME} PRIVATE + dd_wrapper +) + +# We want the extension to be built with position-independent code +set_target_properties(${EXTENSION_NAME} PROPERTIES POSITION_INDEPENDENT_CODE ON) + +# Set the output directory for the built library +set_target_properties(${EXTENSION_NAME} + PROPERTIES + LIBRARY_OUTPUT_DIRECTORY "${CMAKE_CURRENT_LIST_DIR}" +) + +install(TARGETS ${EXTENSION_NAME} DESTINATION ${LIB_INSTALL_DIR}) diff --git a/ddtrace/internal/datadog/profiling/stack_v2/include/constants.hpp b/ddtrace/internal/datadog/profiling/stack_v2/include/constants.hpp new file mode 100644 index 00000000000..1dd51a8e87d --- /dev/null +++ b/ddtrace/internal/datadog/profiling/stack_v2/include/constants.hpp @@ -0,0 +1,8 @@ +#include "dd_wrapper/include/constants.hpp" + +// Default sampling frequency in microseconds. This will almost certainly be overridden by dynamic sampling. +constexpr unsigned int g_default_sampling_period_us = 10000; // 100 Hz +constexpr double g_default_sampling_period_s = g_default_sampling_period_us / 1e6; + +// Echion maintains a cache of frames--the size of this cache is specified up-front. +constexpr unsigned int g_default_echion_frame_cache_size = 1024; diff --git a/ddtrace/internal/datadog/profiling/stack_v2/include/python_headers.hpp b/ddtrace/internal/datadog/profiling/stack_v2/include/python_headers.hpp new file mode 100644 index 00000000000..d7d6398c3b4 --- /dev/null +++ b/ddtrace/internal/datadog/profiling/stack_v2/include/python_headers.hpp @@ -0,0 +1,7 @@ +#define PY_SSIZE_T_CLEAN +#include + +#if PY_VERSION_HEX >= 0x030c0000 +// https://github.com/python/cpython/issues/108216#issuecomment-1696565797 +#undef _PyGC_FINALIZED +#endif diff --git a/ddtrace/internal/datadog/profiling/stack_v2/include/sampler.hpp b/ddtrace/internal/datadog/profiling/stack_v2/include/sampler.hpp new file mode 100644 index 00000000000..9f54f5dcd1c --- /dev/null +++ b/ddtrace/internal/datadog/profiling/stack_v2/include/sampler.hpp @@ -0,0 +1,47 @@ +#pragma once +#include "constants.hpp" +#include "stack_renderer.hpp" + +#include +#include + +namespace Datadog { + +class Sampler +{ + // This class is a singleton purely because the underlying Echion state is global. + private: + std::shared_ptr renderer_ptr; + + // The sampling interval is atomic because it needs to be safely propagated to the sampling thread + std::atomic sample_interval_us{ g_default_sampling_period_us }; + + std::atomic is_profiling{ false }; + std::mutex profiling_mutex; + + // One-time configuration + unsigned int echion_frame_cache_size{ g_default_echion_frame_cache_size }; + unsigned int max_nframes = g_default_max_nframes; + + // Helper function; implementation of the echion sampling thread + void sampling_thread(); + + // This is a singleton, so no public constructor + Sampler(); + + public: + // Singleton instance + static Sampler& get(); + void start(); + void stop(); + + // The Python side dynamically adjusts the sampling rate based on overhead, so we need to be able to update our own + // intervals accordingly. Rather than a preemptive measure, we assume the rate is ~fairly stable and just update + // the next rate with the latest interval. This is not perfect because the adjustment is based on self-time, and + // we're not currently accounting for the echion self-time. + void set_interval(double new_interval); + + void set_max_nframes(unsigned int _max_nfames); +}; + +} // namespace Datadog diff --git a/ddtrace/internal/datadog/profiling/stack_v2/include/stack_renderer.hpp b/ddtrace/internal/datadog/profiling/stack_v2/include/stack_renderer.hpp new file mode 100644 index 00000000000..6e860fb480b --- /dev/null +++ b/ddtrace/internal/datadog/profiling/stack_v2/include/stack_renderer.hpp @@ -0,0 +1,39 @@ +#pragma once + +// Makes up for shortcomings in my echion PR +#include +#include +#include +#include +#include +#include +#include +#include + +#include "python_headers.hpp" + +#include "dd_wrapper/include/interface.hpp" +#include "echion/render.h" + +namespace Datadog { + +class StackRenderer : public RendererInterface +{ + Sample* sample = nullptr; + + void render_message(std::string_view msg) override; + void render_thread_begin(PyThreadState* tstate, + std::string_view name, + microsecond_t wall_time_us, + uintptr_t thread_id, + unsigned long native_id) override; + + void render_stack_begin() override; + void render_python_frame(std::string_view name, std::string_view file, uint64_t line) override; + void render_native_frame(std::string_view name, std::string_view file, uint64_t line) override; + void render_cpu_time(microsecond_t cpu_time_us) override; + void render_stack_end() override; + bool is_valid() override; +}; + +} // namespace Datadog diff --git a/ddtrace/internal/datadog/profiling/stack_v2/include/util/cast_to_pyfunc.hpp b/ddtrace/internal/datadog/profiling/stack_v2/include/util/cast_to_pyfunc.hpp new file mode 100644 index 00000000000..a370b50a0b2 --- /dev/null +++ b/ddtrace/internal/datadog/profiling/stack_v2/include/util/cast_to_pyfunc.hpp @@ -0,0 +1,11 @@ +#pragma once +#include + +// Abiding by the contract required in the Python interface requires abandoning certain checks in one's main project. +// Rather than abandoning discipline, we can include this header in a way that bypasses these checks. +inline PyCFunction +cast_to_pycfunction(PyObject* (*func)(PyObject*, PyObject*, PyObject*)) +{ + // Direct C-style cast + return (PyCFunction)(func); +} diff --git a/ddtrace/internal/datadog/profiling/stack_v2/include/vendored/utf8_validate.hpp b/ddtrace/internal/datadog/profiling/stack_v2/include/vendored/utf8_validate.hpp new file mode 100644 index 00000000000..1715b7ec5ff --- /dev/null +++ b/ddtrace/internal/datadog/profiling/stack_v2/include/vendored/utf8_validate.hpp @@ -0,0 +1,34 @@ +#pragma once + +// From https://gist.github.com/ichramm/3ffeaf7ba4f24853e9ecaf176da84566 +inline static bool +utf8_check_is_valid(const char* str, int len) +{ + int n; + for (int i = 0; i < len; ++i) { + unsigned char c = (unsigned char)str[i]; + // if (c==0x09 || c==0x0a || c==0x0d || (0x20 <= c && c <= 0x7e) ) n = 0; // is_printable_ascii + if (0x00 <= c && c <= 0x7f) { + n = 0; // 0bbbbbbb + } else if ((c & 0xE0) == 0xC0) { + n = 1; // 110bbbbb + } else if (c == 0xed && i < (len - 1) && ((unsigned char)str[i + 1] & 0xa0) == 0xa0) { + return false; // U+d800 to U+dfff + } else if ((c & 0xF0) == 0xE0) { + n = 2; // 1110bbbb + } else if ((c & 0xF8) == 0xF0) { + n = 3; // 11110bbb + //} else if (($c & 0xFC) == 0xF8) { n=4; // 111110bb //byte 5, unnecessary in 4 byte UTF-8 + //} else if (($c & 0xFE) == 0xFC) { n=5; // 1111110b //byte 6, unnecessary in 4 byte UTF-8 + } else { + return false; + } + + for (int j = 0; j < n && i < len; ++j) { // n bytes matching 10bbbbbb follow ? + if ((++i == len) || (((unsigned char)str[i] & 0xC0) != 0x80)) { + return false; + } + } + } + return true; +} diff --git a/ddtrace/internal/datadog/profiling/stack_v2/src/sampler.cpp b/ddtrace/internal/datadog/profiling/stack_v2/src/sampler.cpp new file mode 100644 index 00000000000..e0653c7c165 --- /dev/null +++ b/ddtrace/internal/datadog/profiling/stack_v2/src/sampler.cpp @@ -0,0 +1,112 @@ +#include "sampler.hpp" + +#include "echion/interp.h" +#include "echion/tasks.h" +#include "echion/threads.h" + +using namespace Datadog; + +void +Sampler::sampling_thread() +{ + using namespace std::chrono; + auto sample_time_prev = steady_clock::now(); + + while (true) { + auto sample_time_now = steady_clock::now(); + auto wall_time_us = duration_cast(sample_time_now - sample_time_prev).count(); + sample_time_prev = sample_time_now; + + // Perform the sample + for_each_interp([&](PyInterpreterState* interp) -> void { + for_each_thread(interp, [&](PyThreadState* tstate, ThreadInfo& thread) { + thread.sample(interp->id, tstate, wall_time_us); + }); + }); + + // If we've been asked to stop, then stop + if (!is_profiling.load()) { + break; + } + + // Sleep for the remainder of the interval, get it atomically + // Generally speaking system "sleep" times will wait _at least_ as long as the specified time, so + // in actual fact the duration may be more than we indicated. This tends to be more true on busy + // systems. + std::this_thread::sleep_until(sample_time_now + microseconds(sample_interval_us.load())); + } + + // Release the profiling mutex + profiling_mutex.unlock(); +} + +void +Sampler::set_interval(double new_interval_s) +{ + unsigned int new_interval_us = static_cast(new_interval_s * 1e6); + sample_interval_us.store(new_interval_us); +} + +void +Sampler::set_max_nframes(unsigned int _max_nframes) +{ + max_nframes = _max_nframes; +} + +Sampler::Sampler() +{ + renderer_ptr = std::make_shared(); +} + +Sampler& +Sampler::get() +{ + static Sampler instance; + return instance; +} + +void +Sampler::start() +{ + // Do some one-time setup. There's no real reason to leave this to the caller + static bool initialized = false; + if (!initialized) { + _set_cpu(true); + init_frame_cache(echion_frame_cache_size); + _set_pid(getpid()); + + // Register our rendering callbacks with echion's Renderer singleton + Renderer::get().set_renderer(renderer_ptr); + + // OK, never initialize again + initialized = true; + } + + // For simplicity, we just try to stop the sampling thread. Stopping/starting the profiler in + // a tight loop isn't something we really support. + stop(); + + // OK, now we can start the profiler. + is_profiling.store(true); + std::thread t(&Sampler::sampling_thread, this); + t.detach(); +} + +void +Sampler::stop() +{ + // Try to take the profiling mutex. If we can take it, then we release and we're done. + // If we can't take it, then we're already profiling and we need to stop it. + if (profiling_mutex.try_lock()) { + profiling_mutex.unlock(); + return; + } + + // If we're here, then we need to tell the sampling thread to stop + // We send the signal and wait. + is_profiling.store(false); + profiling_mutex.lock(); + + // When we get the lock, we know the thread is dead, so we can release the mutex + profiling_mutex.unlock(); +} diff --git a/ddtrace/internal/datadog/profiling/stack_v2/src/stack_renderer.cpp b/ddtrace/internal/datadog/profiling/stack_v2/src/stack_renderer.cpp new file mode 100644 index 00000000000..6d7dbf52fe4 --- /dev/null +++ b/ddtrace/internal/datadog/profiling/stack_v2/src/stack_renderer.cpp @@ -0,0 +1,89 @@ +#include "stack_renderer.hpp" +#include "utf8_validate.hpp" + +using namespace Datadog; + +void +StackRenderer::render_message(std::string_view msg) +{ + // This is unused for now, since the dd-trace-py profiler doesn't support emitting arbitrary messages. + (void)msg; +} + +void +StackRenderer::render_thread_begin(PyThreadState* tstate, + std::string_view name, + microsecond_t wall_time_us, + uintptr_t thread_id, + unsigned long native_id) +{ + (void)tstate; + sample = ddup_start_sample(); + if (sample == nullptr) { + return; + } + ddup_push_threadinfo(sample, static_cast(thread_id), static_cast(native_id), name.data()); + ddup_push_walltime(sample, 1000 * wall_time_us, 1); +} + +void +StackRenderer::render_stack_begin() +{ + // This is currently unused, the profiler determines state by keeping track of a sample start +} + +void +StackRenderer::render_python_frame(std::string_view name, std::string_view file, uint64_t line) +{ + if (sample == nullptr) { + return; + } + static const std::string_view invalid = ""; + if (!utf8_check_is_valid(name.data(), name.size())) { + name = invalid; + } + if (!utf8_check_is_valid(file.data(), file.size())) { + file = invalid; + } + ddup_push_frame(sample, name.data(), file.data(), 0, line); + ddup_drop_sample(sample); + sample = nullptr; +} + +void +StackRenderer::render_native_frame(std::string_view name, std::string_view file, uint64_t line) +{ + if (sample == nullptr) { + return; + } + ddup_push_frame(sample, name.data(), file.data(), 0, line); +} + +void +StackRenderer::render_cpu_time(microsecond_t cpu_time_us) +{ + // ddup is configured to expect nanoseconds + if (sample == nullptr) { + return; + } + ddup_push_cputime(sample, 1000 * cpu_time_us, 1); +} + +void +StackRenderer::render_stack_end() +{ + if (sample == nullptr) { + return; + } + ddup_flush_sample(sample); + ddup_drop_sample(sample); + sample = nullptr; +} + +bool +StackRenderer::is_valid() +{ + // In general, echion may need to check whether the extension has invalid state before calling into it, + // but in this case it doesn't matter + return true; +} diff --git a/ddtrace/internal/datadog/profiling/stack_v2/src/stack_v2.cpp b/ddtrace/internal/datadog/profiling/stack_v2/src/stack_v2.cpp new file mode 100644 index 00000000000..60234f53a48 --- /dev/null +++ b/ddtrace/internal/datadog/profiling/stack_v2/src/stack_v2.cpp @@ -0,0 +1,73 @@ +#include "cast_to_pyfunc.hpp" +#include "python_headers.hpp" +#include "sampler.hpp" + +using namespace Datadog; + +static PyObject* +_stack_v2_start(PyObject* self, PyObject* args, PyObject* kwargs) +{ + (void)self; + static const char* const_kwlist[] = { "min_interval", "max_frames", NULL }; + static char** kwlist = const_cast(const_kwlist); + double min_interval_s = g_default_sampling_period_s; + double max_nframes = g_default_max_nframes; + + if (!PyArg_ParseTupleAndKeywords(args, kwargs, "|dd", kwlist, &min_interval_s, &max_nframes)) { + return NULL; // If an error occurs during argument parsing + } + + Sampler::get().set_interval(min_interval_s); + Sampler::get().set_max_nframes(max_nframes); + + return PyLong_FromLong(1); +} + +// Bypasses the old-style cast warning with an unchecked helper function +PyCFunction stack_v2_start = cast_to_pycfunction(_stack_v2_start); + +static PyObject* +stack_v2_stop(PyObject* self, PyObject* args) +{ + (void)self; + (void)args; + Sampler::get().stop(); + Py_INCREF(Py_None); + return Py_None; +} + +static PyObject* +stack_v2_set_interval(PyObject* self, PyObject* args) +{ + // Assumes the interval is given in fractional seconds + (void)self; + double new_interval; + if (!PyArg_ParseTuple(args, "d", &new_interval)) { + return NULL; // If an error occurs during argument parsing + } + Sampler::get().set_interval(new_interval); + Py_INCREF(Py_None); + return Py_None; +} + +static PyMethodDef _stack_v2_methods[] = { + { "start", reinterpret_cast(stack_v2_start), METH_VARARGS | METH_KEYWORDS, "Start the sampler" }, + { "stop", stack_v2_stop, METH_VARARGS, "Stop the sampler" }, + { "set_interval", stack_v2_set_interval, METH_VARARGS, "Set the sampling interval" }, + { NULL, NULL, 0, NULL } +}; + +PyMODINIT_FUNC +PyInit_stack_v2(void) +{ + PyObject* m; + static struct PyModuleDef moduledef = { + PyModuleDef_HEAD_INIT, "_stack_v2", NULL, -1, _stack_v2_methods, NULL, NULL, NULL, NULL + }; + + m = PyModule_Create(&moduledef); + if (!m) + return NULL; + + return m; +} diff --git a/ddtrace/internal/datadog/profiling/stack_v2/src/stackv2.cpp b/ddtrace/internal/datadog/profiling/stack_v2/src/stackv2.cpp new file mode 100644 index 00000000000..aec56b8ef94 --- /dev/null +++ b/ddtrace/internal/datadog/profiling/stack_v2/src/stackv2.cpp @@ -0,0 +1,62 @@ +define PY_SSIZE_T_CLEAN +#include + +#if PY_VERSION_HEX >= 0x030c0000 +// https://github.com/python/cpython/issues/108216#issuecomment-1696565797 +#undef _PyGC_FINALIZED +#endif + +#include "sampler.hpp" + + static PyObject* + stack_v2_start(PyObject* self, PyObject* args) +{ + static const char* const_kwlist[] = { "min_interval", "max_frames", NULL }; + static char** kwlist = const_cast(const_kwlist); + double min_interval_s = g_default_sampling_period_s; + double max_nframes = g_default_max_nframes; + + if (!PyArg_ParseTupleAndKeywords(args, kwargs, "|dd", kwlist, &min_interval_f, &max_frames)) { + return NULL; // If an error occurs during argument parsing + } + + Sampler::get().set_interval(min_interval_s); + Sampler::get().set_max_nframes(max_nframes); + Sampler::init(min_interval_s, max_frames); + + return PyLong_FromLong(1); +} + +static PyObject* +set_v2_interval(PyObject* self, PyObject* args) +{ + double new_interval; + if (!PyArg_ParseTuple(args, "d", &new_interval)) { + return NULL; // If an error occurs during argument parsing + } + _set_v2_interval(new_interval); + Py_INCREF(Py_None); + return Py_None; +} + +static PyMethodDef _stack_v2_methods[] = { + { "start", (PyCFunction)start_stack_v2, METH_VARARGS | METH_KEYWORDS, "Start the sampler" }, + { "stop", (PyCFunction)stop_stack_v2, METH_VARARGS, "Stop the sampler" }, + { "set_interval", (PyCFunction)set_v2_interval, METH_VARARGS, "Set the sampling interval" }, + { NULL, NULL, 0, NULL } +}; + +PyMODINIT_FUNC +PyInit_stack_v2(void) +{ + PyObject* m; + static struct PyModuleDef moduledef = { + PyModuleDef_HEAD_INIT, "_stack_v2", NULL, -1, _stack_v2_methods, NULL, NULL, NULL, NULL + }; + + m = PyModule_Create(&moduledef); + if (!m) + return NULL; + + return m; +} diff --git a/setup.py b/setup.py index 88f3cb0490b..766393420c0 100644 --- a/setup.py +++ b/setup.py @@ -42,7 +42,8 @@ LIBDDWAF_DOWNLOAD_DIR = HERE / "ddtrace" / "appsec" / "_ddwaf" / "libddwaf" IAST_DIR = HERE / "ddtrace" / "appsec" / "_iast" / "_taint_tracking" -DDUP_DIR = HERE / "ddtrace" / "internal" / "datadog" / "profiling" +DDUP_DIR = HERE / "ddtrace" / "internal" / "datadog" / "profiling" / "ddup" +STACK_V2_DIR = HERE / "ddtrace" / "internal" / "datadog" / "profiling" / "stack_v2" CURRENT_OS = platform.system() @@ -463,12 +464,28 @@ def get_exts_for(name): CMakeExtension( "ddtrace.internal.datadog.profiling.ddup._ddup", source_dir=DDUP_DIR, - optional=True, + optional=False, # Change to True for release cmake_args=[ "-DPY_MAJOR_VERSION={}".format(sys.version_info.major), "-DPY_MINOR_VERSION={}".format(sys.version_info.minor), "-DPY_MICRO_VERSION={}".format(sys.version_info.micro), - "-Ddd_wrapper_INSTALL_DIR={}".format(DDUP_DIR), + ], + ) + ) + + ext_modules.append( + CMakeExtension( + "ddtrace.internal.datadog.profiling.stack_v2", + source_dir=STACK_V2_DIR, + optional=False, # Change to True for release + cmake_args=[ + "-DPLATFORM={}".format(CURRENT_OS), + "-DPY_MAJOR_VERSION={}".format(sys.version_info.major), + "-DPY_MINOR_VERSION={}".format(sys.version_info.minor), + "-DPY_MICRO_VERSION={}".format(sys.version_info.micro), + ], + build_args=[ + "--verbose" ], ) ) From 11104837e46f1b44e37f7bbd332397d74ad57a69 Mon Sep 17 00:00:00 2001 From: sanchda Date: Wed, 21 Feb 2024 16:12:03 +0000 Subject: [PATCH 04/76] Add v2 python code --- ddtrace/profiling/collector/stack.pyx | 9 +++++++++ ddtrace/settings/profiling.py | 11 +++++++++++ 2 files changed, 20 insertions(+) diff --git a/ddtrace/profiling/collector/stack.pyx b/ddtrace/profiling/collector/stack.pyx index 31e0deb4375..dd96e23bf97 100644 --- a/ddtrace/profiling/collector/stack.pyx +++ b/ddtrace/profiling/collector/stack.pyx @@ -13,6 +13,7 @@ from ddtrace import context from ddtrace import span as ddspan from ddtrace.internal import compat from ddtrace.internal.datadog.profiling import ddup +from ddtrace.internal.datadog.profiling import stack_v2 from ddtrace.internal.utils import formats from ddtrace.profiling import _threading from ddtrace.profiling import collector @@ -474,6 +475,7 @@ class StackCollector(collector.PeriodicCollector): _thread_time = attr.ib(init=False, repr=False, eq=False) _last_wall_time = attr.ib(init=False, repr=False, eq=False, type=int) _thread_span_links = attr.ib(default=None, init=False, repr=False, eq=False) + _stack_collector_v2_enabled = attr.ib(type=bool, default=config.stack.v2.enabled) @max_time_usage_pct.validator def _check_max_time_usage(self, attribute, value): @@ -490,6 +492,9 @@ class StackCollector(collector.PeriodicCollector): set_use_libdd(config.export.libdd_enabled) set_use_py(config.export.py_enabled) + if self._stack_collector_v2_enabled and use_libdd: + stack_v2.start(max_frames=self.nframes, min_interval=self.min_interval_time) + def _start_service(self): # type: (...) -> None # This is split in its own function to ease testing @@ -506,6 +511,10 @@ class StackCollector(collector.PeriodicCollector): self.tracer.context_provider._deregister_on_activate(self._thread_span_links.link_span) LOG.debug("Profiling StackCollector stopped") + # Also tell the native thread running the v2 sampler to stop, if needed + if self._stack_collector_v2_enabled and use_libdd: + stack_v2.stop() + def _compute_new_interval(self, used_wall_time_ns): interval = (used_wall_time_ns / (self.max_time_usage_pct / 100.0)) - used_wall_time_ns return max(interval / 1e9, self.min_interval_time) diff --git a/ddtrace/settings/profiling.py b/ddtrace/settings/profiling.py index f9829d8389d..3a07e4f406e 100644 --- a/ddtrace/settings/profiling.py +++ b/ddtrace/settings/profiling.py @@ -165,6 +165,17 @@ class Stack(En): help="Whether to enable the stack profiler", ) + class V2(En): + __item__ = __prefix__ = "v2" + + enabled = En.v( + bool, + "enabled", + default=False, + help_type="Boolean", + help="Whether to enable the v2 stack profiler", + ) + class Lock(En): __item__ = __prefix__ = "lock" From 43ec6838939d89311b017def3fe1c54872eb96c3 Mon Sep 17 00:00:00 2001 From: sanchda Date: Wed, 21 Feb 2024 16:42:06 +0000 Subject: [PATCH 05/76] Formatting --- .../datadog/profiling/ddup/__init__.py | 2 +- ddtrace/settings/profiling.py | 18 +++++++++--------- pyproject.toml | 2 +- setup.py | 8 +++----- 4 files changed, 14 insertions(+), 16 deletions(-) diff --git a/ddtrace/internal/datadog/profiling/ddup/__init__.py b/ddtrace/internal/datadog/profiling/ddup/__init__.py index 133432e93ae..8f5ad4fe0b0 100644 --- a/ddtrace/internal/datadog/profiling/ddup/__init__.py +++ b/ddtrace/internal/datadog/profiling/ddup/__init__.py @@ -3,7 +3,7 @@ try: from ._ddup import * # noqa: F403, F401 -except Exception as e: +except Exception: from typing import Dict # noqa:F401 from typing import Optional # noqa:F401 diff --git a/ddtrace/settings/profiling.py b/ddtrace/settings/profiling.py index 3a07e4f406e..2f5e0e2077e 100644 --- a/ddtrace/settings/profiling.py +++ b/ddtrace/settings/profiling.py @@ -166,15 +166,15 @@ class Stack(En): ) class V2(En): - __item__ = __prefix__ = "v2" - - enabled = En.v( - bool, - "enabled", - default=False, - help_type="Boolean", - help="Whether to enable the v2 stack profiler", - ) + __item__ = __prefix__ = "v2" + + enabled = En.v( + bool, + "enabled", + default=False, + help_type="Boolean", + help="Whether to enable the v2 stack profiler", + ) class Lock(En): __item__ = __prefix__ = "lock" diff --git a/pyproject.toml b/pyproject.toml index d753729da75..1c0688a8c5d 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -93,7 +93,7 @@ exclude = ''' | ddtrace/profiling/collector/stack.pyx$ | ddtrace/profiling/exporter/pprof_.*_pb2.py$ | ddtrace/profiling/exporter/pprof.pyx$ - | ddtrace/internal/datadog/profiling/_ddup.pyx$ + | ddtrace/internal/datadog/profiling/ddup/_ddup.pyx$ | ddtrace/vendor/ | ddtrace/appsec/_iast/_taint_tracking/_vendor/ | ddtrace/_version.py diff --git a/setup.py b/setup.py index 766393420c0..f35f063a004 100644 --- a/setup.py +++ b/setup.py @@ -464,7 +464,7 @@ def get_exts_for(name): CMakeExtension( "ddtrace.internal.datadog.profiling.ddup._ddup", source_dir=DDUP_DIR, - optional=False, # Change to True for release + optional=False, # Change to True for release cmake_args=[ "-DPY_MAJOR_VERSION={}".format(sys.version_info.major), "-DPY_MINOR_VERSION={}".format(sys.version_info.minor), @@ -477,16 +477,14 @@ def get_exts_for(name): CMakeExtension( "ddtrace.internal.datadog.profiling.stack_v2", source_dir=STACK_V2_DIR, - optional=False, # Change to True for release + optional=False, # Change to True for release cmake_args=[ "-DPLATFORM={}".format(CURRENT_OS), "-DPY_MAJOR_VERSION={}".format(sys.version_info.major), "-DPY_MINOR_VERSION={}".format(sys.version_info.minor), "-DPY_MICRO_VERSION={}".format(sys.version_info.micro), ], - build_args=[ - "--verbose" - ], + build_args=["--verbose"], ) ) From f667466239748d87113611cf9a1aae2829ba12b8 Mon Sep 17 00:00:00 2001 From: sanchda Date: Wed, 21 Feb 2024 16:51:21 +0000 Subject: [PATCH 06/76] Fixup gitignore --- .gitignore | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/.gitignore b/.gitignore index 9e66a75cb8a..f93c439edba 100644 --- 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 From c6317e8c52f0fb0ad71e61638d4ae9af68a84201 Mon Sep 17 00:00:00 2001 From: sanchda Date: Wed, 21 Feb 2024 19:45:13 +0000 Subject: [PATCH 07/76] Fixups --- .../datadog/profiling/dd_wrapper/src/uploader_builder.cpp | 8 +++++--- ddtrace/profiling/exporter/pprof.pyx | 2 +- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/ddtrace/internal/datadog/profiling/dd_wrapper/src/uploader_builder.cpp b/ddtrace/internal/datadog/profiling/dd_wrapper/src/uploader_builder.cpp index d5affa66df4..3dfde517f44 100644 --- a/ddtrace/internal/datadog/profiling/dd_wrapper/src/uploader_builder.cpp +++ b/ddtrace/internal/datadog/profiling/dd_wrapper/src/uploader_builder.cpp @@ -115,8 +115,10 @@ UploaderBuilder::build() for (const auto& [tag, data] : tag_data) { std::string errmsg; - if (!add_tag(tags, tag, data, errmsg)) { - reasons.push_back(std::string(to_string(tag)) + ": " + errmsg); + if (!data.empty()) { + if (!add_tag(tags, tag, data, errmsg)) { + reasons.push_back(std::string(to_string(tag)) + ": " + errmsg); + } } } @@ -131,7 +133,7 @@ UploaderBuilder::build() // If any mistakes were made, report on them now and throw if (!reasons.empty()) { ddog_Vec_Tag_drop(tags); - throw std::runtime_error("Error initializing exporter: missing or bad configuration: " + join(reasons, ", ")); + throw std::runtime_error("Error initializing exporter, missing or bad configuration: " + join(reasons, ", ")); } // If we're here, the tags are good, so we can initialize the exporter diff --git a/ddtrace/profiling/exporter/pprof.pyx b/ddtrace/profiling/exporter/pprof.pyx index 68061a38608..79f40685fef 100644 --- a/ddtrace/profiling/exporter/pprof.pyx +++ b/ddtrace/profiling/exporter/pprof.pyx @@ -12,7 +12,7 @@ from ddtrace import ext from ddtrace.internal import packages from ddtrace.internal._encoding import ListStringTable as _StringTable from ddtrace.internal.compat import ensure_text -from ddtrace.internal.datadog.profiling.utils import sanitize_string +from ddtrace.internal.datadog.profiling.ddup.utils import sanitize_string from ddtrace.internal.utils import config from ddtrace.profiling import event from ddtrace.profiling import exporter From c207279a5b248b3f4bd827e2867f59d02c272b37 Mon Sep 17 00:00:00 2001 From: sanchda Date: Fri, 23 Feb 2024 14:03:48 +0000 Subject: [PATCH 08/76] Stricter type conformance in ddup --- ddtrace/internal/datadog/profiling/ddup/_ddup.pyx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ddtrace/internal/datadog/profiling/ddup/_ddup.pyx b/ddtrace/internal/datadog/profiling/ddup/_ddup.pyx index 20ad71d91e7..b3054f2f3b5 100644 --- a/ddtrace/internal/datadog/profiling/ddup/_ddup.pyx +++ b/ddtrace/internal/datadog/profiling/ddup/_ddup.pyx @@ -190,9 +190,9 @@ IF UNAME_SYSNAME == "Linux": if span._local_root.span_id: ddup_push_local_root_span_id(self.ptr, span._local_root.span_id) if span._local_root.span_type: - ddup_push_trace_type(self.ptr, span._local_root.span_type) + ddup_push_trace_type(self.ptr, ensure_binary(span._local_root.span_type)) if endpoint_collection_enabled: - ddup_push_trace_resource_container(self.ptr, span._local_root._resource) + ddup_push_trace_resource_container(self.ptr, ensure_binary(span._local_root.service)) def flush_sample(self) -> None: # Flushing the sample consumes it. The user will no longer be able to use From 3ca20fb0dfdc4c331de2e6dcc4ed4bd736e31d7e Mon Sep 17 00:00:00 2001 From: sanchda Date: Fri, 23 Feb 2024 14:19:51 +0000 Subject: [PATCH 09/76] Fix incorrect counting of dropped frames --- ddtrace/internal/datadog/profiling/dd_wrapper/src/sample.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/ddtrace/internal/datadog/profiling/dd_wrapper/src/sample.cpp b/ddtrace/internal/datadog/profiling/dd_wrapper/src/sample.cpp index f2051eb1516..617007f26b4 100644 --- a/ddtrace/internal/datadog/profiling/dd_wrapper/src/sample.cpp +++ b/ddtrace/internal/datadog/profiling/dd_wrapper/src/sample.cpp @@ -54,8 +54,9 @@ Sample::push_frame(std::string_view name, std::string_view filename, uint64_t ad if (locations.size() <= max_nframes) { push_frame_impl(name, filename, address, line); + } else { + ++dropped_frames; } - ++dropped_frames; } bool From 06d05b53babaed35ecce97bad2c81f09a96854aa Mon Sep 17 00:00:00 2001 From: sanchda Date: Fri, 23 Feb 2024 15:14:11 +0000 Subject: [PATCH 10/76] Make extensions optional --- setup.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/setup.py b/setup.py index f35f063a004..8a608d17784 100644 --- a/setup.py +++ b/setup.py @@ -354,7 +354,7 @@ def __init__( build_args=[], install_args=[], build_type=None, - optional=False, + optional=True, # By default, extensions are optional for now ): super().__init__(name, sources=[]) self.source_dir = source_dir @@ -456,7 +456,7 @@ def get_exts_for(name): ) ext_modules.append( - CMakeExtension("ddtrace.appsec._iast._taint_tracking._native", source_dir=IAST_DIR, optional=True) + CMakeExtension("ddtrace.appsec._iast._taint_tracking._native", source_dir=IAST_DIR) ) if platform.system() == "Linux" and is_64_bit_python(): @@ -464,7 +464,6 @@ def get_exts_for(name): CMakeExtension( "ddtrace.internal.datadog.profiling.ddup._ddup", source_dir=DDUP_DIR, - optional=False, # Change to True for release cmake_args=[ "-DPY_MAJOR_VERSION={}".format(sys.version_info.major), "-DPY_MINOR_VERSION={}".format(sys.version_info.minor), @@ -477,7 +476,6 @@ def get_exts_for(name): CMakeExtension( "ddtrace.internal.datadog.profiling.stack_v2", source_dir=STACK_V2_DIR, - optional=False, # Change to True for release cmake_args=[ "-DPLATFORM={}".format(CURRENT_OS), "-DPY_MAJOR_VERSION={}".format(sys.version_info.major), From 01a026bc9369b52864d65a19a6cd8fdbf53d75aa Mon Sep 17 00:00:00 2001 From: sanchda Date: Fri, 23 Feb 2024 21:07:19 +0000 Subject: [PATCH 11/76] Various cleanups to builds Moved things around to streamline the build process a bit. Also discovered that recent changes were leaking intermediate artifacts into the final .whl, which was causing auditwheel issues when the .so.debug from libdatadog was being inspected. --- .../profiling/cmake/FindLibdatadog.cmake | 4 +-- .../datadog/profiling/ddup/CMakeLists.txt | 7 +++-- .../profiling/{dd_wrapper => }/runner.sh | 2 ++ .../{dd_wrapper => }/setup_custom.sh | 1 + .../datadog/profiling/stack_v2/CMakeLists.txt | 5 ++-- .../profiling/stack_v2/src/stack_renderer.cpp | 2 ++ setup.py | 29 +++++++++---------- 7 files changed, 29 insertions(+), 21 deletions(-) rename ddtrace/internal/datadog/profiling/{dd_wrapper => }/runner.sh (96%) rename ddtrace/internal/datadog/profiling/{dd_wrapper => }/setup_custom.sh (99%) diff --git a/ddtrace/internal/datadog/profiling/cmake/FindLibdatadog.cmake b/ddtrace/internal/datadog/profiling/cmake/FindLibdatadog.cmake index cad6b66d021..b632f1a84fd 100644 --- a/ddtrace/internal/datadog/profiling/cmake/FindLibdatadog.cmake +++ b/ddtrace/internal/datadog/profiling/cmake/FindLibdatadog.cmake @@ -8,7 +8,7 @@ set(TAG_LIBDATADOG "v5.0.0" CACHE STRING "libdatadog github tag") -set(Datadog_BUILD_DIR ${CMAKE_CURRENT_BINARY_DIR}/libdatadog) +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}") @@ -22,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/ddup/CMakeLists.txt b/ddtrace/internal/datadog/profiling/ddup/CMakeLists.txt index b5e963444bc..79b19e61c36 100644 --- a/ddtrace/internal/datadog/profiling/ddup/CMakeLists.txt +++ b/ddtrace/internal/datadog/profiling/ddup/CMakeLists.txt @@ -5,7 +5,7 @@ cmake_minimum_required(VERSION 3.19) # setup.py to build the wheel. Otherwise, we have to propagate a lot of state all around. # Thus, we use the same name as the Python package (i.e., the caller sets EXTENSION_NAME) # ddup is used by a default for standalone/test builds. -set(EXTENSION_NAME "ddup" CACHE STRING "Name of the extension") +set(EXTENSION_NAME "ddtrace.internal.datadog.profiling.ddup._ddup" CACHE STRING "Name of the extension") project(${EXTENSION_NAME}) # Get the cmake modules for this project @@ -75,4 +75,7 @@ set_target_properties(${EXTENSION_NAME} LIBRARY_OUTPUT_DIRECTORY "${CMAKE_CURRENT_LIST_DIR}" ) -install(TARGETS ${EXTENSION_NAME} DESTINATION ${LIB_INSTALL_DIR}) +# This installs the .so back into the source tree +if (LIB_INSTALL_DIR) + install(TARGETS ${EXTENSION_NAME} DESTINATION ${LIB_INSTALL_DIR}) +endif() diff --git a/ddtrace/internal/datadog/profiling/dd_wrapper/runner.sh b/ddtrace/internal/datadog/profiling/runner.sh similarity index 96% rename from ddtrace/internal/datadog/profiling/dd_wrapper/runner.sh rename to ddtrace/internal/datadog/profiling/runner.sh index b5636a05acd..e2afdec2443 100755 --- a/ddtrace/internal/datadog/profiling/dd_wrapper/runner.sh +++ b/ddtrace/internal/datadog/profiling/runner.sh @@ -1,4 +1,6 @@ #!/bin/bash +set -euox pipefail + ./setup_custom.sh -C || { echo "Failed cppcheck"; exit 1; } ./setup_custom.sh -s || { echo "Failed safety tests"; exit 1; } ./setup_custom.sh -f || { echo "Failed -fanalyzer"; exit 1; } diff --git a/ddtrace/internal/datadog/profiling/dd_wrapper/setup_custom.sh b/ddtrace/internal/datadog/profiling/setup_custom.sh similarity index 99% rename from ddtrace/internal/datadog/profiling/dd_wrapper/setup_custom.sh rename to ddtrace/internal/datadog/profiling/setup_custom.sh index 34452736b9f..4b64acdb123 100755 --- a/ddtrace/internal/datadog/profiling/dd_wrapper/setup_custom.sh +++ b/ddtrace/internal/datadog/profiling/setup_custom.sh @@ -1,4 +1,5 @@ #!/bin/bash +set -euox pipefail ### Useful globals MY_DIR=$(dirname $(realpath $0)) diff --git a/ddtrace/internal/datadog/profiling/stack_v2/CMakeLists.txt b/ddtrace/internal/datadog/profiling/stack_v2/CMakeLists.txt index 990dea179e7..56fd5f24e2e 100644 --- a/ddtrace/internal/datadog/profiling/stack_v2/CMakeLists.txt +++ b/ddtrace/internal/datadog/profiling/stack_v2/CMakeLists.txt @@ -51,7 +51,6 @@ target_compile_definitions(${EXTENSION_NAME} PRIVATE UNWIND_NATIVE_DISABLE) # Includes; echion and python are marked "system" to suppress warnings, # but note in MSVC we'll have to #pragma warning(push, 0 then pop for the same effect. -message(ERROR "echion_SOURCE_DIR: ${echion_SOURCE_DIR}") target_include_directories(${EXTENSION_NAME} PRIVATE .. include @@ -92,4 +91,6 @@ set_target_properties(${EXTENSION_NAME} LIBRARY_OUTPUT_DIRECTORY "${CMAKE_CURRENT_LIST_DIR}" ) -install(TARGETS ${EXTENSION_NAME} DESTINATION ${LIB_INSTALL_DIR}) +if (LIB_INSTALL_DIR) + install(TARGETS ${EXTENSION_NAME} DESTINATION ${LIB_INSTALL_DIR}) +endif() 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 6d7dbf52fe4..b2fceb2d41a 100644 --- a/ddtrace/internal/datadog/profiling/stack_v2/src/stack_renderer.cpp +++ b/ddtrace/internal/datadog/profiling/stack_v2/src/stack_renderer.cpp @@ -22,6 +22,8 @@ StackRenderer::render_thread_begin(PyThreadState* tstate, if (sample == nullptr) { return; } + +//#warning stack_v2 should use a C++ interface instead of re-converting intermediates ddup_push_threadinfo(sample, static_cast(thread_id), static_cast(native_id), name.data()); ddup_push_walltime(sample, 1000 * wall_time_us, 1); } diff --git a/setup.py b/setup.py index 8a608d17784..78ddea860e4 100644 --- a/setup.py +++ b/setup.py @@ -283,29 +283,20 @@ def build_extension(self, ext): def build_extension_cmake(self, ext): # Define the build and output directories output_dir = Path(self.get_ext_fullpath(ext.name)).parent.resolve() - extension_basename = Path(self.get_ext_fullpath(ext.name)).name # We derive the cmake build directory from the output directory, but put it in # a sibling directory to avoid polluting the final package cmake_build_dir = Path(self.build_lib.replace("lib.", "cmake."), ext.name).resolve() cmake_build_dir.mkdir(parents=True, exist_ok=True) - # Get development paths - python_include = sysconfig.get_paths()["include"] - python_lib = sysconfig.get_config_var("LIBDIR") - # Which commands are passed to _every_ cmake invocation cmake_args = ext.cmake_args or [] cmake_args += [ "-S{}".format(ext.source_dir), # cmake>=3.13 "-B{}".format(cmake_build_dir), # cmake>=3.13 - "-DPython3_INCLUDE_DIRS={}".format(python_include), - "-DPython3_LIBRARIES={}".format(python_lib), - "-DPYTHON_EXECUTABLE={}".format(sys.executable), "-DCMAKE_BUILD_TYPE={}".format(ext.build_type), "-DCMAKE_LIBRARY_OUTPUT_DIRECTORY={}".format(output_dir), "-DLIB_INSTALL_DIR={}".format(output_dir), - "-DEXTENSION_NAME={}".format(extension_basename), ] # Arguments to the cmake --build command @@ -460,27 +451,35 @@ def get_exts_for(name): ) if platform.system() == "Linux" and is_64_bit_python(): + # Get Python library paths development paths + python_include = sysconfig.get_paths()["include"] + python_lib = sysconfig.get_config_var("LIBDIR") + + ddup_extension_name = "ddtrace.internal.datadog.profiling.ddup._ddup" ext_modules.append( CMakeExtension( - "ddtrace.internal.datadog.profiling.ddup._ddup", + ddup_extension_name, source_dir=DDUP_DIR, cmake_args=[ "-DPY_MAJOR_VERSION={}".format(sys.version_info.major), "-DPY_MINOR_VERSION={}".format(sys.version_info.minor), "-DPY_MICRO_VERSION={}".format(sys.version_info.micro), + "-DEXTENSION_NAME={}".format(ddup_extension_name), + "-DPython3_INCLUDE_DIRS={}".format(python_include), + "-DPYTHON_EXECUTABLE={}".format(sys.executable), ], ) ) + stack_v2_extension_name = "ddtrace.internal.datadog.profiling.stack_v2" ext_modules.append( CMakeExtension( - "ddtrace.internal.datadog.profiling.stack_v2", + stack_v2_extension_name, source_dir=STACK_V2_DIR, cmake_args=[ - "-DPLATFORM={}".format(CURRENT_OS), - "-DPY_MAJOR_VERSION={}".format(sys.version_info.major), - "-DPY_MINOR_VERSION={}".format(sys.version_info.minor), - "-DPY_MICRO_VERSION={}".format(sys.version_info.micro), + "-DEXTENSION_NAME={}".format(stack_v2_extension_name), + "-DPython3_INCLUDE_DIRS={}".format(python_include), + "-DPYTHON_EXECUTABLE={}".format(sys.executable), ], build_args=["--verbose"], ) From 8a708d55ec23ab636d0efcff7d13c391ef430b95 Mon Sep 17 00:00:00 2001 From: sanchda Date: Mon, 26 Feb 2024 18:56:40 +0000 Subject: [PATCH 12/76] First cut at string_viewifying --- .../dd_wrapper/include/interface.hpp | 37 +++---- .../profiling/dd_wrapper/src/interface.cpp | 102 ++++++------------ .../internal/datadog/profiling/ddup/_ddup.pyx | 85 +++++++++------ setup.py | 1 + 4 files changed, 104 insertions(+), 121 deletions(-) diff --git a/ddtrace/internal/datadog/profiling/dd_wrapper/include/interface.hpp b/ddtrace/internal/datadog/profiling/dd_wrapper/include/interface.hpp index 10baa7e93ed..0018a4b5e15 100644 --- 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 #include +#include // 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/src/interface.cpp b/ddtrace/internal/datadog/profiling/dd_wrapper/src/interface.cpp index 254ca8a4124..a730686a8bc 100644 --- a/ddtrace/internal/datadog/profiling/dd_wrapper/src/interface.cpp +++ b/ddtrace/internal/datadog/profiling/dd_wrapper/src/interface.cpp @@ -40,73 +40,57 @@ ddup_prefork() // Configuration void -ddup_config_env(const char* dd_env) +ddup_config_env(std::string_view dd_env) { - if (dd_env) { - Datadog::UploaderBuilder::set_env(dd_env); - } + Datadog::UploaderBuilder::set_env(dd_env); } void -ddup_config_service(const char* service) +ddup_config_service(std::string_view service) { - if (service) { - Datadog::UploaderBuilder::set_service(service); - } + Datadog::UploaderBuilder::set_service(service); } void -ddup_config_version(const char* version) +ddup_config_version(std::string_view version) { - if (version) { - Datadog::UploaderBuilder::set_version(version); - } + Datadog::UploaderBuilder::set_version(version); } void -ddup_config_runtime(const char* runtime) +ddup_config_runtime(std::string_view runtime) { - if (runtime) { - Datadog::UploaderBuilder::set_runtime(runtime); - } + Datadog::UploaderBuilder::set_runtime(runtime); } void -ddup_config_runtime_version(const char* runtime_version) +ddup_config_runtime_version(std::string_view runtime_version) { - if (runtime_version) { - Datadog::UploaderBuilder::set_runtime_version(runtime_version); - } + Datadog::UploaderBuilder::set_runtime_version(runtime_version); } void -ddup_config_profiler_version(const char* profiler_version) +ddup_config_profiler_version(std::string_view profiler_version) { - if (profiler_version) { - Datadog::UploaderBuilder::set_profiler_version(profiler_version); - } + Datadog::UploaderBuilder::set_profiler_version(profiler_version); } void -ddup_config_url(const char* url) +ddup_config_url(std::string_view url) { - if (url) { - Datadog::UploaderBuilder::set_url(url); - } + Datadog::UploaderBuilder::set_url(url); } void -ddup_config_user_tag(const char* key, const char* val) +ddup_config_user_tag(std::string_view key, std::string_view val) { - if (key && val) { - Datadog::UploaderBuilder::set_tag(key, val); - } + Datadog::UploaderBuilder::set_tag(key, val); } void -ddup_set_runtime_id(const char* id, size_t sz) +ddup_set_runtime_id(std::string_view id) { - Datadog::UploaderBuilder::set_runtime_id(std::string_view(id, sz)); + Datadog::UploaderBuilder::set_runtime_id(id); } void @@ -193,21 +177,15 @@ ddup_push_heap(Datadog::Sample* sample, uint64_t size) } void -ddup_push_lock_name(Datadog::Sample* sample, const char* lock_name) +ddup_push_lock_name(Datadog::Sample* sample, std::string_view lock_name) { - if (lock_name) { - sample->push_lock_name(lock_name); - } + sample->push_lock_name(lock_name); } void -ddup_push_threadinfo(Datadog::Sample* sample, int64_t thread_id, int64_t thread_native_id, const char* thread_name) +ddup_push_threadinfo(Datadog::Sample* sample, int64_t thread_id, int64_t thread_native_id, std::string_view thread_name) { - if (thread_name) { - sample->push_threadinfo(thread_id, thread_native_id, thread_name); - } else { - sample->push_threadinfo(thread_id, thread_native_id, ""); - } + sample->push_threadinfo(thread_id, thread_native_id, thread_name); } void @@ -217,11 +195,9 @@ ddup_push_task_id(Datadog::Sample* sample, int64_t task_id) } void -ddup_push_task_name(Datadog::Sample* sample, const char* task_name) +ddup_push_task_name(Datadog::Sample* sample, std::string_view task_name) { - if (task_name) { - sample->push_task_name(task_name); - } + sample->push_task_name(task_name); } void @@ -237,43 +213,33 @@ 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) +ddup_push_trace_type(Datadog::Sample* sample, std::string_view trace_type) { - if (trace_type) { - sample->push_trace_type(trace_type); - } + sample->push_trace_type(trace_type); } void -ddup_push_trace_resource_container(Datadog::Sample* sample, const char* trace_resource_container) +ddup_push_trace_resource_container(Datadog::Sample* sample, std::string_view trace_resource_container) { - if (trace_resource_container) { - sample->push_trace_resource_container(trace_resource_container); - } + sample->push_trace_resource_container(trace_resource_container); } void -ddup_push_exceptioninfo(Datadog::Sample* sample, const char* exception_type, int64_t count) +ddup_push_exceptioninfo(Datadog::Sample* sample, std::string_view exception_type, int64_t count) { - if (exception_type) { - sample->push_exceptioninfo(exception_type, count); - } + sample->push_exceptioninfo(exception_type, count); } void -ddup_push_class_name(Datadog::Sample* sample, const char* class_name) +ddup_push_class_name(Datadog::Sample* sample, std::string_view class_name) { - if (class_name) { - sample->push_class_name(class_name); - } + sample->push_class_name(class_name); } void -ddup_push_frame(Datadog::Sample* sample, const char* _name, const char* _filename, uint64_t address, int64_t line) +ddup_push_frame(Datadog::Sample* sample, std::string_view _name, std::string_view _filename, uint64_t address, int64_t line) { - if (_name && _filename) { - sample->push_frame(_name, _filename, address, line); - } + sample->push_frame(_name, _filename, address, line); } void diff --git a/ddtrace/internal/datadog/profiling/ddup/_ddup.pyx b/ddtrace/internal/datadog/profiling/ddup/_ddup.pyx index b3054f2f3b5..bbdd1ae3096 100644 --- a/ddtrace/internal/datadog/profiling/ddup/_ddup.pyx +++ b/ddtrace/internal/datadog/profiling/ddup/_ddup.pyx @@ -1,3 +1,6 @@ +# distutils: language = c++ +# cython: language_level=3 + import platform import typing from typing import Optional @@ -13,6 +16,18 @@ from .utils import sanitize_string IF UNAME_SYSNAME == "Linux": + cdef extern from "" namespace "std" nogil: + cdef cppclass string_view: + string_view(const char* s, size_t count) + + cdef pystr_to_sv(object s): + cdef bytes s_bytes = b"" + try: + s_bytes = ensure_binary(s) + except Exception: + pass + return string_view(s_bytes, len(s_bytes)) + cdef extern from "types.hpp": ctypedef enum ProfileType "ProfileType": CPU "ProfileType::CPU" @@ -31,16 +46,16 @@ IF UNAME_SYSNAME == "Linux": cdef extern from "interface.hpp": ctypedef signed int int64_t ctypedef unsigned int uint64_t - void ddup_config_env(const char *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(string_view env) + void ddup_config_service(string_view service) + void ddup_config_version(string_view version) + void ddup_config_runtime(string_view runtime) + void ddup_config_runtime_version(string_view runtime_version) + void ddup_config_profiler_version(string_view profiler_version) + void ddup_config_url(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(string_view key, string_view val) void ddup_config_sample_type(unsigned int type) void ddup_init() @@ -52,20 +67,20 @@ IF UNAME_SYSNAME == "Linux": void ddup_push_release(Sample *sample, int64_t release_time, int64_t count) void ddup_push_alloc(Sample *sample, uint64_t size, uint64_t count) void ddup_push_heap(Sample *sample, uint64_t size) - void ddup_push_lock_name(Sample *sample, const char *lock_name) - void ddup_push_threadinfo(Sample *sample, int64_t thread_id, int64_t thread_native_id, const char *thread_name) + void ddup_push_lock_name(Sample *sample, string_view lock_name) + void ddup_push_threadinfo(Sample *sample, int64_t thread_id, int64_t thread_native_id, string_view thread_name) void ddup_push_task_id(Sample *sample, int64_t task_id) - void ddup_push_task_name(Sample *sample, const char *task_name) + void ddup_push_task_name(Sample *sample, string_view task_name) void ddup_push_span_id(Sample *sample, uint64_t span_id) void ddup_push_local_root_span_id(Sample *sample, uint64_t local_root_span_id) - void ddup_push_trace_type(Sample *sample, const char *trace_type) - void ddup_push_trace_resource_container(Sample *sample, const char *trace_resource_container) - void ddup_push_exceptioninfo(Sample *sample, const char *exception_type, int64_t count) - void ddup_push_class_name(Sample *sample, const char *class_name) - void ddup_push_frame(Sample *sample, const char *_name, const char *_filename, uint64_t address, int64_t line) + void ddup_push_trace_type(Sample *sample, string_view trace_type) + void ddup_push_trace_resource_container(Sample *sample, string_view trace_resource_container) + void ddup_push_exceptioninfo(Sample *sample, string_view exception_type, int64_t count) + void ddup_push_class_name(Sample *sample, string_view class_name) + void ddup_push_frame(Sample *sample, string_view _name, string_view _filename, uint64_t address, int64_t line) void ddup_flush_sample(Sample *sample) void ddup_drop_sample(Sample *sample) - void ddup_set_runtime_id(const char *_id, size_t sz) + void ddup_set_runtime_id(string_view _id) bint ddup_upload() nogil def init( @@ -78,31 +93,31 @@ IF UNAME_SYSNAME == "Linux": # Try to provide a ddtrace-specific default service if one is not given service = service or DEFAULT_SERVICE_NAME - ddup_config_service(ensure_binary(service)) + ddup_config_service(pystr_to_sv(service)) # If otherwise no values are provided, the uploader will omit the fields # and they will be auto-populated in the backend if env: - ddup_config_env(ensure_binary(env)) + ddup_config_env(pystr_to_sv(env)) if version: - ddup_config_version(ensure_binary(version)) + ddup_config_version(pystr_to_sv(version)) if url: - ddup_config_url(ensure_binary(url)) + ddup_config_url(pystr_to_sv(url)) # Inherited - ddup_config_runtime(ensure_binary(platform.python_implementation())) - ddup_config_runtime_version(ensure_binary(platform.python_version())) - ddup_config_profiler_version(ensure_binary(ddtrace.__version__)) + ddup_config_runtime(pystr_to_sv(platform.python_implementation())) + ddup_config_runtime_version(pystr_to_sv(platform.python_version())) + ddup_config_profiler_version(pystr_to_sv(ddtrace.__version__)) ddup_config_max_nframes(max_nframes) if tags is not None: for key, val in tags.items(): if key and val: - ddup_config_user_tag(ensure_binary(key), ensure_binary(val)) + ddup_config_user_tag(pystr_to_sv(key), pystr_to_sv(val)) ddup_init() def upload() -> None: - runtime_id = ensure_binary(runtime.get_runtime_id()) - ddup_set_runtime_id(runtime_id, len(runtime_id)) + runtime_id = pystr_to_sv(runtime.get_runtime_id()) + ddup_set_runtime_id(runtime_id) with nogil: ddup_upload() @@ -143,20 +158,20 @@ IF UNAME_SYSNAME == "Linux": def push_lock_name(self, lock_name: str) -> None: if self.ptr is not NULL: - ddup_push_lock_name(self.ptr, ensure_binary(lock_name)) + ddup_push_lock_name(self.ptr, pystr_to_sv(lock_name)) def push_frame(self, name: str, filename: str, int address, int line) -> None: if self.ptr is not NULL: name = sanitize_string(name) filename = sanitize_string(filename) - ddup_push_frame(self.ptr, ensure_binary(name), ensure_binary(filename), address, line) + ddup_push_frame(self.ptr, pystr_to_sv(name), pystr_to_sv(filename), address, line) def push_threadinfo(self, thread_id: int, thread_native_id: int, thread_name: str) -> None: if self.ptr is not NULL: thread_id = thread_id if thread_id is not None else 0 thread_native_id = thread_native_id if thread_native_id is not None else 0 thread_name = thread_name if thread_name is not None else "" - ddup_push_threadinfo(self.ptr, thread_id, thread_native_id, ensure_binary(thread_name)) + ddup_push_threadinfo(self.ptr, thread_id, thread_native_id, pystr_to_sv(thread_name)) def push_task_id(self, task_id: int) -> None: if self.ptr is not NULL: @@ -165,18 +180,18 @@ IF UNAME_SYSNAME == "Linux": def push_task_name(self, task_name: str) -> None: if self.ptr is not NULL: if task_name: - ddup_push_task_name(self.ptr, ensure_binary(task_name)) + ddup_push_task_name(self.ptr, pystr_to_sv(task_name)) def push_exceptioninfo(self, exc_type: type, count: int) -> None: if self.ptr is not NULL: if exc_type is not None: exc_name = exc_type.__module__ + "." + exc_type.__name__ - ddup_push_exceptioninfo(self.ptr, ensure_binary(exc_name), count) + ddup_push_exceptioninfo(self.ptr, pystr_to_sv(exc_name), count) def push_class_name(self, class_name: str) -> None: if self.ptr is not NULL: class_name = class_name if class_name is not None else "" - ddup_push_class_name(self.ptr, ensure_binary(class_name)) + ddup_push_class_name(self.ptr, pystr_to_sv(class_name)) def push_span(self, span: typing.Optional[Span], endpoint_collection_enabled: bool) -> None: if self.ptr is NULL: @@ -190,9 +205,9 @@ IF UNAME_SYSNAME == "Linux": if span._local_root.span_id: ddup_push_local_root_span_id(self.ptr, span._local_root.span_id) if span._local_root.span_type: - ddup_push_trace_type(self.ptr, ensure_binary(span._local_root.span_type)) + ddup_push_trace_type(self.ptr, pystr_to_sv(span._local_root.span_type)) if endpoint_collection_enabled: - ddup_push_trace_resource_container(self.ptr, ensure_binary(span._local_root.service)) + ddup_push_trace_resource_container(self.ptr, pystr_to_sv(span._local_root.service)) def flush_sample(self) -> None: # Flushing the sample consumes it. The user will no longer be able to use diff --git a/setup.py b/setup.py index 78ddea860e4..550a1d97d39 100644 --- a/setup.py +++ b/setup.py @@ -460,6 +460,7 @@ def get_exts_for(name): CMakeExtension( ddup_extension_name, source_dir=DDUP_DIR, + optional=False, cmake_args=[ "-DPY_MAJOR_VERSION={}".format(sys.version_info.major), "-DPY_MINOR_VERSION={}".format(sys.version_info.minor), From e76e8d02846b0011854a835a9ae79eb7c51461ea Mon Sep 17 00:00:00 2001 From: sanchda Date: Mon, 26 Feb 2024 19:50:11 +0000 Subject: [PATCH 13/76] Add more string_view stuff --- .../internal/datadog/profiling/ddup/_ddup.pyx | 105 +++++++++++------- 1 file changed, 62 insertions(+), 43 deletions(-) diff --git a/ddtrace/internal/datadog/profiling/ddup/_ddup.pyx b/ddtrace/internal/datadog/profiling/ddup/_ddup.pyx index bbdd1ae3096..5a8567a4447 100644 --- a/ddtrace/internal/datadog/profiling/ddup/_ddup.pyx +++ b/ddtrace/internal/datadog/profiling/ddup/_ddup.pyx @@ -16,28 +16,16 @@ from .utils import sanitize_string IF UNAME_SYSNAME == "Linux": - cdef extern from "" namespace "std" nogil: - cdef cppclass string_view: - string_view(const char* s, size_t count) - - cdef pystr_to_sv(object s): - cdef bytes s_bytes = b"" + def ensure_binary_or_empty(s) -> bytes: try: - s_bytes = ensure_binary(s) + return ensure_binary(s) except Exception: pass - return string_view(s_bytes, len(s_bytes)) - - cdef extern from "types.hpp": - ctypedef enum ProfileType "ProfileType": - CPU "ProfileType::CPU" - Wall "ProfileType::Wall" - Exception "ProfileType::Exception" - LockAcquire "ProfileType::LockAcquire" - LockRelease "ProfileType::LockRelease" - Allocation "ProfileType::Allocation" - Heap "ProfileType::Heap" - All "ProfileType::All" + return b"" + + cdef extern from "" namespace "std" nogil: + cdef cppclass string_view: + string_view(const char* s, size_t count) cdef extern from "sample.hpp" namespace "Datadog": ctypedef struct Sample: @@ -83,6 +71,31 @@ IF UNAME_SYSNAME == "Linux": void ddup_set_runtime_id(string_view _id) bint ddup_upload() nogil + # Create wrappers for cython + cdef call_ddup_config_service(bytes service): + ddup_config_service(string_view(service, len(service))) + + cdef call_ddup_config_env(bytes env): + ddup_config_env(string_view(env, len(env))) + + cdef call_ddup_config_version(bytes version): + ddup_config_version(string_view(version, len(version))) + + cdef call_ddup_config_url(bytes url): + ddup_config_url(string_view(url, len(url))) + + cdef call_ddup_config_runtime(bytes runtime): + ddup_config_runtime(string_view(runtime, len(runtime))) + + cdef call_ddup_config_runtime_version(bytes runtime_version): + ddup_config_runtime_version(string_view(runtime_version, len(runtime_version))) + + cdef call_ddup_config_profiler_version(bytes profiler_version): + ddup_config_profiler_version(string_view(profiler_version, len(profiler_version))) + + cdef call_ddup_config_user_tag(bytes key, bytes val): + ddup_config_user_tag(string_view(key, len(key)), string_view(val, len(val))) + def init( service: Optional[str], env: Optional[str], @@ -93,31 +106,31 @@ IF UNAME_SYSNAME == "Linux": # Try to provide a ddtrace-specific default service if one is not given service = service or DEFAULT_SERVICE_NAME - ddup_config_service(pystr_to_sv(service)) + call_ddup_config_service(ensure_binary_or_empty(service)) # If otherwise no values are provided, the uploader will omit the fields # and they will be auto-populated in the backend if env: - ddup_config_env(pystr_to_sv(env)) + call_ddup_config_env(ensure_binary_or_empty(env)) if version: - ddup_config_version(pystr_to_sv(version)) + call_ddup_config_version(ensure_binary_or_empty(version)) if url: - ddup_config_url(pystr_to_sv(url)) + call_ddup_config_url(ensure_binary_or_empty(url)) # Inherited - ddup_config_runtime(pystr_to_sv(platform.python_implementation())) - ddup_config_runtime_version(pystr_to_sv(platform.python_version())) - ddup_config_profiler_version(pystr_to_sv(ddtrace.__version__)) - ddup_config_max_nframes(max_nframes) + call_ddup_config_runtime(ensure_binary_or_empty(platform.python_implementation())) + call_ddup_config_runtime_version(ensure_binary_or_empty(platform.python_version())) + call_ddup_config_profiler_version(ensure_binary_or_empty(ddtrace.__version__)) + ddup_config_max_nframes(max_nframes) # call_* only needed for string-type args if tags is not None: for key, val in tags.items(): if key and val: - ddup_config_user_tag(pystr_to_sv(key), pystr_to_sv(val)) + call_ddup_config_user_tag(ensure_binary_or_empty(key), ensure_binary_or_empty(val)) ddup_init() def upload() -> None: - runtime_id = pystr_to_sv(runtime.get_runtime_id()) - ddup_set_runtime_id(runtime_id) + runtime_id = ensure_binary_or_empty(runtime.get_runtime_id()) + ddup_set_runtime_id(string_view(runtime_id, len(runtime_id))) with nogil: ddup_upload() @@ -158,20 +171,23 @@ IF UNAME_SYSNAME == "Linux": def push_lock_name(self, lock_name: str) -> None: if self.ptr is not NULL: - ddup_push_lock_name(self.ptr, pystr_to_sv(lock_name)) + lock_name_bytes = ensure_binary_or_empty(lock_name) + ddup_push_lock_name(self.ptr, string_view(lock_name_bytes, len(lock_name_bytes))) def push_frame(self, name: str, filename: str, int address, int line) -> None: if self.ptr is not NULL: - name = sanitize_string(name) - filename = sanitize_string(filename) - ddup_push_frame(self.ptr, pystr_to_sv(name), pystr_to_sv(filename), address, line) + # We've ogtten rreports that `name` and `filename` may be unexpected objects, so we go through a sanitization procedure. + # this is almost certainly wasteful. + name_bytes = ensure_binary_or_empty(sanitize_string(name)) + filename_bytes = ensure_binary_or_empty(sanitize_string(filename)) + ddup_push_frame(self.ptr, string_view(name_bytes, len(name_bytes)), string_view(filename_bytes, len(filename_bytes)), address, line) def push_threadinfo(self, thread_id: int, thread_native_id: int, thread_name: str) -> None: if self.ptr is not NULL: thread_id = thread_id if thread_id is not None else 0 thread_native_id = thread_native_id if thread_native_id is not None else 0 - thread_name = thread_name if thread_name is not None else "" - ddup_push_threadinfo(self.ptr, thread_id, thread_native_id, pystr_to_sv(thread_name)) + thread_name_bytes = ensure_binary_or_empty(thread_name) + ddup_push_threadinfo(self.ptr, thread_id, thread_native_id, string_view(thread_name_bytes, len(thread_name_bytes))) def push_task_id(self, task_id: int) -> None: if self.ptr is not NULL: @@ -180,18 +196,19 @@ IF UNAME_SYSNAME == "Linux": def push_task_name(self, task_name: str) -> None: if self.ptr is not NULL: if task_name: - ddup_push_task_name(self.ptr, pystr_to_sv(task_name)) + task_name_bytes = ensure_binary_or_empty(task_name) + ddup_push_task_name(self.ptr, string_view(task_name_bytes, len(task_name_bytes))) def push_exceptioninfo(self, exc_type: type, count: int) -> None: if self.ptr is not NULL: if exc_type is not None: - exc_name = exc_type.__module__ + "." + exc_type.__name__ - ddup_push_exceptioninfo(self.ptr, pystr_to_sv(exc_name), count) + exc_name = ensure_binary_or_empty(exc_type.__module__ + "." + exc_type.__name__) + ddup_push_exceptioninfo(self.ptr, string_view(exc_name, len(exc_name)), count) def push_class_name(self, class_name: str) -> None: if self.ptr is not NULL: - class_name = class_name if class_name is not None else "" - ddup_push_class_name(self.ptr, pystr_to_sv(class_name)) + class_name_bytes = ensure_binary_or_empty(class_name) + ddup_push_class_name(self.ptr, string_view(class_name_bytes, len(class_name_bytes))) def push_span(self, span: typing.Optional[Span], endpoint_collection_enabled: bool) -> None: if self.ptr is NULL: @@ -205,9 +222,11 @@ IF UNAME_SYSNAME == "Linux": if span._local_root.span_id: ddup_push_local_root_span_id(self.ptr, span._local_root.span_id) if span._local_root.span_type: - ddup_push_trace_type(self.ptr, pystr_to_sv(span._local_root.span_type)) + span_type_bytes = ensure_binary_or_empty(span._local_root.span_type) + ddup_push_trace_type(self.ptr, string_view(span_type_bytes, len(span_type_bytes))) if endpoint_collection_enabled: - ddup_push_trace_resource_container(self.ptr, pystr_to_sv(span._local_root.service)) + root_service_bytes = ensure_binary_or_empty(span._local_root.service) + ddup_push_trace_resource_container(self.ptr, string_view(root_service_bytes, len(root_service_bytes))) def flush_sample(self) -> None: # Flushing the sample consumes it. The user will no longer be able to use From 6539f4911683d62d56648a6be410bb4133811f21 Mon Sep 17 00:00:00 2001 From: sanchda Date: Mon, 26 Feb 2024 19:51:30 +0000 Subject: [PATCH 14/76] Update stackv2 to use new string_view interface --- .../datadog/profiling/stack_v2/src/stack_renderer.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) 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 b2fceb2d41a..0485efd2b2a 100644 --- a/ddtrace/internal/datadog/profiling/stack_v2/src/stack_renderer.cpp +++ b/ddtrace/internal/datadog/profiling/stack_v2/src/stack_renderer.cpp @@ -24,7 +24,7 @@ StackRenderer::render_thread_begin(PyThreadState* tstate, } //#warning stack_v2 should use a C++ interface instead of re-converting intermediates - ddup_push_threadinfo(sample, static_cast(thread_id), static_cast(native_id), name.data()); + ddup_push_threadinfo(sample, static_cast(thread_id), static_cast(native_id), name); ddup_push_walltime(sample, 1000 * wall_time_us, 1); } @@ -47,7 +47,7 @@ StackRenderer::render_python_frame(std::string_view name, std::string_view file, if (!utf8_check_is_valid(file.data(), file.size())) { file = invalid; } - ddup_push_frame(sample, name.data(), file.data(), 0, line); + ddup_push_frame(sample, name, file, 0, line); ddup_drop_sample(sample); sample = nullptr; } @@ -58,7 +58,7 @@ StackRenderer::render_native_frame(std::string_view name, std::string_view file, if (sample == nullptr) { return; } - ddup_push_frame(sample, name.data(), file.data(), 0, line); + ddup_push_frame(sample, name, file, 0, line); } void From 1dd33e0912aafde7339ea90d5bab7578b6892dcd Mon Sep 17 00:00:00 2001 From: sanchda Date: Mon, 26 Feb 2024 19:54:03 +0000 Subject: [PATCH 15/76] stackv2 required, fix comment --- .../internal/datadog/profiling/dd_wrapper/test/forking.cpp | 4 ++-- setup.py | 1 + 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/ddtrace/internal/datadog/profiling/dd_wrapper/test/forking.cpp b/ddtrace/internal/datadog/profiling/dd_wrapper/test/forking.cpp index 82a4f1c41e8..909ddb9422c 100644 --- a/ddtrace/internal/datadog/profiling/dd_wrapper/test/forking.cpp +++ b/ddtrace/internal/datadog/profiling/dd_wrapper/test/forking.cpp @@ -14,8 +14,8 @@ upload_in_thread() [[noreturn]] void profile_in_child(unsigned int num_threads, unsigned int run_time_ns, std::atomic& done) { - // Assumes setup has been called. Launch some samplers, wait, upload, return. - // Exit because we don't want this process to exit + // Assumes setup has been called. Launch some samplers, wait, upload, exit. + // Managing interleaved execution is tricky, so the fork exits--rather than returns--when it is done. std::vector ids; for (unsigned int i = 0; i < num_threads; i++) { ids.push_back(i); diff --git a/setup.py b/setup.py index 550a1d97d39..2c2c7bbcf42 100644 --- a/setup.py +++ b/setup.py @@ -476,6 +476,7 @@ def get_exts_for(name): ext_modules.append( CMakeExtension( stack_v2_extension_name, + optional=False, source_dir=STACK_V2_DIR, cmake_args=[ "-DEXTENSION_NAME={}".format(stack_v2_extension_name), From b5c4976a853b9b37af19aaf78c8a261340f08644 Mon Sep 17 00:00:00 2001 From: sanchda Date: Mon, 26 Feb 2024 20:02:13 +0000 Subject: [PATCH 16/76] Clarifying comment --- .../internal/datadog/profiling/stack_v2/include/sampler.hpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/ddtrace/internal/datadog/profiling/stack_v2/include/sampler.hpp b/ddtrace/internal/datadog/profiling/stack_v2/include/sampler.hpp index 9f54f5dcd1c..417a4acbc0c 100644 --- a/ddtrace/internal/datadog/profiling/stack_v2/include/sampler.hpp +++ b/ddtrace/internal/datadog/profiling/stack_v2/include/sampler.hpp @@ -9,7 +9,9 @@ namespace Datadog { class Sampler { - // This class is a singleton purely because the underlying Echion state is global. + // This class manages the initialization of echion as well as the sampling thread. + // The underlying echion instance it manages keeps much of its state globally, so this class is a singleton in order + // to keep it aligned with the echion state. private: std::shared_ptr renderer_ptr; From 9e7f1840340fa6b22ff9ffe5bf639c5f8a8758e0 Mon Sep 17 00:00:00 2001 From: sanchda Date: Mon, 26 Feb 2024 20:15:14 +0000 Subject: [PATCH 17/76] Comment clarifying range --- .../datadog/profiling/setup_custom.sh | 3 + .../profiling/stack_v2/src/stackv2.cpp | 62 ------------------- 2 files changed, 3 insertions(+), 62 deletions(-) delete mode 100644 ddtrace/internal/datadog/profiling/stack_v2/src/stackv2.cpp diff --git a/ddtrace/internal/datadog/profiling/setup_custom.sh b/ddtrace/internal/datadog/profiling/setup_custom.sh index 4b64acdb123..504c761502f 100755 --- a/ddtrace/internal/datadog/profiling/setup_custom.sh +++ b/ddtrace/internal/datadog/profiling/setup_custom.sh @@ -19,6 +19,9 @@ find_highest_compiler_version() { local highest_var_name=$2 local highest_version=0 + # Try to find the latest versions of both GCC and Clang + # The range 5-20 is arbitrary (GCC 5 was released in 2015, and 20 is just a + # a high number since Clang is on version 17) for version in {20..5}; do if command -v "${base_name}-${version}" &> /dev/null; then if [ $version -gt $highest_version ]; then diff --git a/ddtrace/internal/datadog/profiling/stack_v2/src/stackv2.cpp b/ddtrace/internal/datadog/profiling/stack_v2/src/stackv2.cpp deleted file mode 100644 index aec56b8ef94..00000000000 --- a/ddtrace/internal/datadog/profiling/stack_v2/src/stackv2.cpp +++ /dev/null @@ -1,62 +0,0 @@ -define PY_SSIZE_T_CLEAN -#include - -#if PY_VERSION_HEX >= 0x030c0000 -// https://github.com/python/cpython/issues/108216#issuecomment-1696565797 -#undef _PyGC_FINALIZED -#endif - -#include "sampler.hpp" - - static PyObject* - stack_v2_start(PyObject* self, PyObject* args) -{ - static const char* const_kwlist[] = { "min_interval", "max_frames", NULL }; - static char** kwlist = const_cast(const_kwlist); - double min_interval_s = g_default_sampling_period_s; - double max_nframes = g_default_max_nframes; - - if (!PyArg_ParseTupleAndKeywords(args, kwargs, "|dd", kwlist, &min_interval_f, &max_frames)) { - return NULL; // If an error occurs during argument parsing - } - - Sampler::get().set_interval(min_interval_s); - Sampler::get().set_max_nframes(max_nframes); - Sampler::init(min_interval_s, max_frames); - - return PyLong_FromLong(1); -} - -static PyObject* -set_v2_interval(PyObject* self, PyObject* args) -{ - double new_interval; - if (!PyArg_ParseTuple(args, "d", &new_interval)) { - return NULL; // If an error occurs during argument parsing - } - _set_v2_interval(new_interval); - Py_INCREF(Py_None); - return Py_None; -} - -static PyMethodDef _stack_v2_methods[] = { - { "start", (PyCFunction)start_stack_v2, METH_VARARGS | METH_KEYWORDS, "Start the sampler" }, - { "stop", (PyCFunction)stop_stack_v2, METH_VARARGS, "Stop the sampler" }, - { "set_interval", (PyCFunction)set_v2_interval, METH_VARARGS, "Set the sampling interval" }, - { NULL, NULL, 0, NULL } -}; - -PyMODINIT_FUNC -PyInit_stack_v2(void) -{ - PyObject* m; - static struct PyModuleDef moduledef = { - PyModuleDef_HEAD_INIT, "_stack_v2", NULL, -1, _stack_v2_methods, NULL, NULL, NULL, NULL - }; - - m = PyModule_Create(&moduledef); - if (!m) - return NULL; - - return m; -} From 73045bbfb1f31705837c33527c7dede41a67a9e7 Mon Sep 17 00:00:00 2001 From: sanchda Date: Mon, 26 Feb 2024 20:17:53 +0000 Subject: [PATCH 18/76] More comments in setup_custom --- ddtrace/internal/datadog/profiling/setup_custom.sh | 3 +++ 1 file changed, 3 insertions(+) diff --git a/ddtrace/internal/datadog/profiling/setup_custom.sh b/ddtrace/internal/datadog/profiling/setup_custom.sh index 504c761502f..b3f5d4e4c32 100755 --- a/ddtrace/internal/datadog/profiling/setup_custom.sh +++ b/ddtrace/internal/datadog/profiling/setup_custom.sh @@ -22,6 +22,9 @@ find_highest_compiler_version() { # Try to find the latest versions of both GCC and Clang # The range 5-20 is arbitrary (GCC 5 was released in 2015, and 20 is just a # a high number since Clang is on version 17) + # TODO provide a passthrough in CI where we want to use a pinned or specified + # version. Not a big deal since this script is currently only used for local + # development. for version in {20..5}; do if command -v "${base_name}-${version}" &> /dev/null; then if [ $version -gt $highest_version ]; then From f5eaa253329b1b18595aeb5450f711d74b4b03c9 Mon Sep 17 00:00:00 2001 From: sanchda Date: Mon, 26 Feb 2024 20:19:47 +0000 Subject: [PATCH 19/76] Remove stale comment --- .../datadog/profiling/stack_v2/include/stack_renderer.hpp | 1 - 1 file changed, 1 deletion(-) diff --git a/ddtrace/internal/datadog/profiling/stack_v2/include/stack_renderer.hpp b/ddtrace/internal/datadog/profiling/stack_v2/include/stack_renderer.hpp index 6e860fb480b..63be0b95d2f 100644 --- a/ddtrace/internal/datadog/profiling/stack_v2/include/stack_renderer.hpp +++ b/ddtrace/internal/datadog/profiling/stack_v2/include/stack_renderer.hpp @@ -1,6 +1,5 @@ #pragma once -// Makes up for shortcomings in my echion PR #include #include #include From c3ff98ff808dc4f1437f97159db20410d4bccf71 Mon Sep 17 00:00:00 2001 From: sanchda Date: Mon, 26 Feb 2024 22:20:34 +0000 Subject: [PATCH 20/76] Minor changes to build system --- .../datadog/profiling/cmake/AnalysisFunc.cmake | 2 +- setup.py | 10 ++++------ 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/ddtrace/internal/datadog/profiling/cmake/AnalysisFunc.cmake b/ddtrace/internal/datadog/profiling/cmake/AnalysisFunc.cmake index 804538d218b..9f0bd65ae29 100644 --- a/ddtrace/internal/datadog/profiling/cmake/AnalysisFunc.cmake +++ b/ddtrace/internal/datadog/profiling/cmake/AnalysisFunc.cmake @@ -2,7 +2,7 @@ include(FindCppcheck) function(add_ddup_config target) target_compile_options(${target} PRIVATE - "$<$:-Og -ggdb3>" + "$<$:-Og;-ggdb3>" "$<$:-Os>" -ffunction-sections -fno-semantic-interposition -Wall -Werror -Wextra -Wshadow -Wnon-virtual-dtor -Wold-style-cast ) diff --git a/setup.py b/setup.py index 2c2c7bbcf42..473461f3696 100644 --- a/setup.py +++ b/setup.py @@ -283,6 +283,7 @@ def build_extension(self, ext): def build_extension_cmake(self, ext): # Define the build and output directories output_dir = Path(self.get_ext_fullpath(ext.name)).parent.resolve() + extension_basename = Path(self.get_ext_fullpath(ext.name)).name # We derive the cmake build directory from the output directory, but put it in # a sibling directory to avoid polluting the final package @@ -297,6 +298,7 @@ def build_extension_cmake(self, ext): "-DCMAKE_BUILD_TYPE={}".format(ext.build_type), "-DCMAKE_LIBRARY_OUTPUT_DIRECTORY={}".format(output_dir), "-DLIB_INSTALL_DIR={}".format(output_dir), + "-DCMAKE_EXTENSION_NAME={}".format(extension_basename), ] # Arguments to the cmake --build command @@ -455,31 +457,27 @@ def get_exts_for(name): python_include = sysconfig.get_paths()["include"] python_lib = sysconfig.get_config_var("LIBDIR") - ddup_extension_name = "ddtrace.internal.datadog.profiling.ddup._ddup" ext_modules.append( CMakeExtension( - ddup_extension_name, + "ddtrace.internal.datadog.profiling.ddup._ddup", source_dir=DDUP_DIR, optional=False, cmake_args=[ "-DPY_MAJOR_VERSION={}".format(sys.version_info.major), "-DPY_MINOR_VERSION={}".format(sys.version_info.minor), "-DPY_MICRO_VERSION={}".format(sys.version_info.micro), - "-DEXTENSION_NAME={}".format(ddup_extension_name), "-DPython3_INCLUDE_DIRS={}".format(python_include), "-DPYTHON_EXECUTABLE={}".format(sys.executable), ], ) ) - stack_v2_extension_name = "ddtrace.internal.datadog.profiling.stack_v2" ext_modules.append( CMakeExtension( - stack_v2_extension_name, + "ddtrace.internal.datadog.profiling.stack_v2", optional=False, source_dir=STACK_V2_DIR, cmake_args=[ - "-DEXTENSION_NAME={}".format(stack_v2_extension_name), "-DPython3_INCLUDE_DIRS={}".format(python_include), "-DPYTHON_EXECUTABLE={}".format(sys.executable), ], From bc161745282d24a0dd27ea1fbbc949987c5391b7 Mon Sep 17 00:00:00 2001 From: sanchda Date: Tue, 27 Feb 2024 04:00:13 +0000 Subject: [PATCH 21/76] Try to fix the broken resource paths --- ddtrace/internal/datadog/profiling/ddup/CMakeLists.txt | 5 +++-- ddtrace/internal/datadog/profiling/stack_v2/CMakeLists.txt | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/ddtrace/internal/datadog/profiling/ddup/CMakeLists.txt b/ddtrace/internal/datadog/profiling/ddup/CMakeLists.txt index 79b19e61c36..37f3b252672 100644 --- a/ddtrace/internal/datadog/profiling/ddup/CMakeLists.txt +++ b/ddtrace/internal/datadog/profiling/ddup/CMakeLists.txt @@ -5,8 +5,9 @@ cmake_minimum_required(VERSION 3.19) # setup.py to build the wheel. Otherwise, we have to propagate a lot of state all around. # Thus, we use the same name as the Python package (i.e., the caller sets EXTENSION_NAME) # ddup is used by a default for standalone/test builds. -set(EXTENSION_NAME "ddtrace.internal.datadog.profiling.ddup._ddup" CACHE STRING "Name of the extension") +set(EXTENSION_NAME "_ddup" CACHE STRING "Name of the extension") project(${EXTENSION_NAME}) +message(STATUS "Building extension: ${EXTENSION_NAME}") # Get the cmake modules for this project list(APPEND CMAKE_MODULE_PATH "${CMAKE_CURRENT_LIST_DIR}/../cmake") @@ -49,7 +50,7 @@ target_compile_features(${EXTENSION_NAME} PUBLIC cxx_std_17) # This suppresses that behavior, which is required to ensure all paths can be inferred # correctly by setup.py. set_target_properties(${EXTENSION_NAME} PROPERTIES PREFIX "") -set_target_properties(${EXTENSION_NAME} PROPERTIES SUFFIX "") +#set_target_properties(${EXTENSION_NAME} PROPERTIES SUFFIX "") # RPATH is needed for sofile discovery at runtime, since Python packages are not # installed in the system path. This is typical. diff --git a/ddtrace/internal/datadog/profiling/stack_v2/CMakeLists.txt b/ddtrace/internal/datadog/profiling/stack_v2/CMakeLists.txt index 56fd5f24e2e..82e532a91bf 100644 --- a/ddtrace/internal/datadog/profiling/stack_v2/CMakeLists.txt +++ b/ddtrace/internal/datadog/profiling/stack_v2/CMakeLists.txt @@ -75,7 +75,7 @@ endif() # This suppresses that behavior, which is required to ensure all paths can be inferred # correctly by setup.py. set_target_properties(${EXTENSION_NAME} PROPERTIES PREFIX "") -set_target_properties(${EXTENSION_NAME} PROPERTIES SUFFIX "") +#set_target_properties(${EXTENSION_NAME} PROPERTIES SUFFIX "") set_target_properties(${EXTENSION_NAME} PROPERTIES INSTALL_RPATH "$ORIGIN") # NB., gets installed to parent! target_link_libraries(${EXTENSION_NAME} PRIVATE From cf56f231d6c33442a70a0b9420c8ea1a0ced07ca Mon Sep 17 00:00:00 2001 From: sanchda Date: Tue, 27 Feb 2024 06:53:06 +0000 Subject: [PATCH 22/76] Fix build, respond to comments --- .../dd_wrapper/src/uploader_builder.cpp | 2 ++ .../datadog/profiling/ddup/CMakeLists.txt | 14 ++++---- .../datadog/profiling/stack_v2/CMakeLists.txt | 20 +++++++---- .../profiling/stack_v2/include/sampler.hpp | 9 ++--- .../profiling/stack_v2/src/sampler.cpp | 34 ++++--------------- setup.py | 2 +- 6 files changed, 36 insertions(+), 45 deletions(-) diff --git a/ddtrace/internal/datadog/profiling/dd_wrapper/src/uploader_builder.cpp b/ddtrace/internal/datadog/profiling/dd_wrapper/src/uploader_builder.cpp index 3dfde517f44..879bb2be3d1 100644 --- a/ddtrace/internal/datadog/profiling/dd_wrapper/src/uploader_builder.cpp +++ b/ddtrace/internal/datadog/profiling/dd_wrapper/src/uploader_builder.cpp @@ -119,6 +119,8 @@ UploaderBuilder::build() if (!add_tag(tags, tag, data, errmsg)) { reasons.push_back(std::string(to_string(tag)) + ": " + errmsg); } + } else { + reasons.push_back(std::string(to_string(tag)) + ": " + "empty"); } } diff --git a/ddtrace/internal/datadog/profiling/ddup/CMakeLists.txt b/ddtrace/internal/datadog/profiling/ddup/CMakeLists.txt index 37f3b252672..a596d6a941e 100644 --- a/ddtrace/internal/datadog/profiling/ddup/CMakeLists.txt +++ b/ddtrace/internal/datadog/profiling/ddup/CMakeLists.txt @@ -50,7 +50,7 @@ target_compile_features(${EXTENSION_NAME} PUBLIC cxx_std_17) # This suppresses that behavior, which is required to ensure all paths can be inferred # correctly by setup.py. set_target_properties(${EXTENSION_NAME} PROPERTIES PREFIX "") -#set_target_properties(${EXTENSION_NAME} PROPERTIES SUFFIX "") +set_target_properties(${EXTENSION_NAME} PROPERTIES SUFFIX "") # RPATH is needed for sofile discovery at runtime, since Python packages are not # installed in the system path. This is typical. @@ -70,13 +70,15 @@ target_link_libraries(${EXTENSION_NAME} PRIVATE # Extensions are built as dynamic libraries, so PIC is required. set_target_properties(${EXTENSION_NAME} PROPERTIES POSITION_INDEPENDENT_CODE ON) -# Set the output directory for the built library -set_target_properties(${EXTENSION_NAME} - PROPERTIES +set_target_properties(${EXTENSION_NAME} PROPERTIES LIBRARY_OUTPUT_DIRECTORY "${CMAKE_CURRENT_LIST_DIR}" ) -# This installs the .so back into the source tree +# Set the output directory for the built library if (LIB_INSTALL_DIR) - install(TARGETS ${EXTENSION_NAME} DESTINATION ${LIB_INSTALL_DIR}) + install(TARGETS ${EXTENSION_NAME} DESTINATION + LIBRARY DESTINATION ${LIB_INSTALL_DIR} + ARCHIVE DESTINATION ${LIB_INSTALL_DIR} + RUNTIME DESTINATION ${LIB_INSTALL_DIR} + ) endif() diff --git a/ddtrace/internal/datadog/profiling/stack_v2/CMakeLists.txt b/ddtrace/internal/datadog/profiling/stack_v2/CMakeLists.txt index 82e532a91bf..4d169d3e96f 100644 --- a/ddtrace/internal/datadog/profiling/stack_v2/CMakeLists.txt +++ b/ddtrace/internal/datadog/profiling/stack_v2/CMakeLists.txt @@ -75,22 +75,28 @@ endif() # This suppresses that behavior, which is required to ensure all paths can be inferred # correctly by setup.py. set_target_properties(${EXTENSION_NAME} PROPERTIES PREFIX "") -#set_target_properties(${EXTENSION_NAME} PROPERTIES SUFFIX "") -set_target_properties(${EXTENSION_NAME} PROPERTIES INSTALL_RPATH "$ORIGIN") # NB., gets installed to parent! +set_target_properties(${EXTENSION_NAME} PROPERTIES SUFFIX "") + +# RPATH is needed for sofile discovery at runtime, since Python packages are not +# installed in the system path. This is typical. +set_target_properties(${EXTENSION_NAME} PROPERTIES INSTALL_RPATH "$ORIGIN") target_link_libraries(${EXTENSION_NAME} PRIVATE dd_wrapper ) -# We want the extension to be built with position-independent code +# Extensions are built as dynamic libraries, so PIC is required. set_target_properties(${EXTENSION_NAME} PROPERTIES POSITION_INDEPENDENT_CODE ON) -# Set the output directory for the built library -set_target_properties(${EXTENSION_NAME} - PROPERTIES +set_target_properties(${EXTENSION_NAME} PROPERTIES LIBRARY_OUTPUT_DIRECTORY "${CMAKE_CURRENT_LIST_DIR}" ) +# Set the output directory for the built library if (LIB_INSTALL_DIR) - install(TARGETS ${EXTENSION_NAME} DESTINATION ${LIB_INSTALL_DIR}) + install(TARGETS ${EXTENSION_NAME} DESTINATION + LIBRARY DESTINATION ${LIB_INSTALL_DIR} + ARCHIVE DESTINATION ${LIB_INSTALL_DIR} + RUNTIME DESTINATION ${LIB_INSTALL_DIR} + ) endif() diff --git a/ddtrace/internal/datadog/profiling/stack_v2/include/sampler.hpp b/ddtrace/internal/datadog/profiling/stack_v2/include/sampler.hpp index 417a4acbc0c..226c60b8c05 100644 --- a/ddtrace/internal/datadog/profiling/stack_v2/include/sampler.hpp +++ b/ddtrace/internal/datadog/profiling/stack_v2/include/sampler.hpp @@ -3,7 +3,6 @@ #include "stack_renderer.hpp" #include -#include namespace Datadog { @@ -18,15 +17,17 @@ class Sampler // The sampling interval is atomic because it needs to be safely propagated to the sampling thread std::atomic sample_interval_us{ g_default_sampling_period_us }; - std::atomic is_profiling{ false }; - std::mutex profiling_mutex; + // This is not a running total of the number of launched threads; it is a sequence for the + // transactions upon the sampling threads (usually starts + stops). This allows threads to be + // stopped or started in a straightforward manner without finer-grained control (locks) + std::atomic thread_seq_num{ 0 }; // One-time configuration unsigned int echion_frame_cache_size{ g_default_echion_frame_cache_size }; unsigned int max_nframes = g_default_max_nframes; // Helper function; implementation of the echion sampling thread - void sampling_thread(); + void sampling_thread(unsigned long seq_num); // This is a singleton, so no public constructor Sampler(); diff --git a/ddtrace/internal/datadog/profiling/stack_v2/src/sampler.cpp b/ddtrace/internal/datadog/profiling/stack_v2/src/sampler.cpp index e0653c7c165..48a4b38ecb8 100644 --- a/ddtrace/internal/datadog/profiling/stack_v2/src/sampler.cpp +++ b/ddtrace/internal/datadog/profiling/stack_v2/src/sampler.cpp @@ -7,12 +7,12 @@ using namespace Datadog; void -Sampler::sampling_thread() +Sampler::sampling_thread(unsigned long seq_num) { using namespace std::chrono; auto sample_time_prev = steady_clock::now(); - while (true) { + while (seq_num == thread_seq_num.load()) { auto sample_time_now = steady_clock::now(); auto wall_time_us = duration_cast(sample_time_now - sample_time_prev).count(); sample_time_prev = sample_time_now; @@ -24,20 +24,12 @@ Sampler::sampling_thread() }); }); - // If we've been asked to stop, then stop - if (!is_profiling.load()) { - break; - } - // Sleep for the remainder of the interval, get it atomically // Generally speaking system "sleep" times will wait _at least_ as long as the specified time, so // in actual fact the duration may be more than we indicated. This tends to be more true on busy // systems. std::this_thread::sleep_until(sample_time_now + microseconds(sample_interval_us.load())); } - - // Release the profiling mutex - profiling_mutex.unlock(); } void @@ -86,27 +78,15 @@ Sampler::start() // a tight loop isn't something we really support. stop(); - // OK, now we can start the profiler. - is_profiling.store(true); - std::thread t(&Sampler::sampling_thread, this); + // OK, now we can start the profiler. When we launch the profiler, we mutate the sequence number and give it to the + // thread. This will (eventually) terminate any outstanding thread. + std::thread t(&Sampler::sampling_thread, this, ++thread_seq_num); t.detach(); } void Sampler::stop() { - // Try to take the profiling mutex. If we can take it, then we release and we're done. - // If we can't take it, then we're already profiling and we need to stop it. - if (profiling_mutex.try_lock()) { - profiling_mutex.unlock(); - return; - } - - // If we're here, then we need to tell the sampling thread to stop - // We send the signal and wait. - is_profiling.store(false); - profiling_mutex.lock(); - - // When we get the lock, we know the thread is dead, so we can release the mutex - profiling_mutex.unlock(); + // Modifying the thread sequence number will cause the sampling thread to exit. + ++thread_seq_num; } diff --git a/setup.py b/setup.py index 473461f3696..93fbcd59650 100644 --- a/setup.py +++ b/setup.py @@ -298,7 +298,7 @@ def build_extension_cmake(self, ext): "-DCMAKE_BUILD_TYPE={}".format(ext.build_type), "-DCMAKE_LIBRARY_OUTPUT_DIRECTORY={}".format(output_dir), "-DLIB_INSTALL_DIR={}".format(output_dir), - "-DCMAKE_EXTENSION_NAME={}".format(extension_basename), + "-DEXTENSION_NAME={}".format(extension_basename), ] # Arguments to the cmake --build command From 904bbff090b5fb2c2637d34273257c4c69ba4933 Mon Sep 17 00:00:00 2001 From: sanchda Date: Tue, 27 Feb 2024 07:20:01 +0000 Subject: [PATCH 23/76] Streamline init --- .../profiling/stack_v2/include/sampler.hpp | 9 +++-- .../profiling/stack_v2/src/sampler.cpp | 39 ++++++++++--------- 2 files changed, 26 insertions(+), 22 deletions(-) diff --git a/ddtrace/internal/datadog/profiling/stack_v2/include/sampler.hpp b/ddtrace/internal/datadog/profiling/stack_v2/include/sampler.hpp index 226c60b8c05..027f7cbd00d 100644 --- a/ddtrace/internal/datadog/profiling/stack_v2/include/sampler.hpp +++ b/ddtrace/internal/datadog/profiling/stack_v2/include/sampler.hpp @@ -15,15 +15,15 @@ class Sampler std::shared_ptr renderer_ptr; // The sampling interval is atomic because it needs to be safely propagated to the sampling thread - std::atomic sample_interval_us{ g_default_sampling_period_us }; + std::atomic sample_interval_us{ g_default_sampling_period_us }; // This is not a running total of the number of launched threads; it is a sequence for the // transactions upon the sampling threads (usually starts + stops). This allows threads to be // stopped or started in a straightforward manner without finer-grained control (locks) std::atomic thread_seq_num{ 0 }; - // One-time configuration - unsigned int echion_frame_cache_size{ g_default_echion_frame_cache_size }; + // Parameters + unsigned int echion_frame_cache_size = g_default_echion_frame_cache_size; unsigned int max_nframes = g_default_max_nframes; // Helper function; implementation of the echion sampling thread @@ -32,6 +32,9 @@ class Sampler // This is a singleton, so no public constructor Sampler(); + // One-time setup of echion + void one_time_setup(); + public: // Singleton instance static Sampler& get(); diff --git a/ddtrace/internal/datadog/profiling/stack_v2/src/sampler.cpp b/ddtrace/internal/datadog/profiling/stack_v2/src/sampler.cpp index 48a4b38ecb8..a7afb7f4baa 100644 --- a/ddtrace/internal/datadog/profiling/stack_v2/src/sampler.cpp +++ b/ddtrace/internal/datadog/profiling/stack_v2/src/sampler.cpp @@ -35,14 +35,18 @@ Sampler::sampling_thread(unsigned long seq_num) void Sampler::set_interval(double new_interval_s) { - unsigned int new_interval_us = static_cast(new_interval_s * 1e6); + microsecond_t new_interval_us = static_cast(new_interval_s * 1e6); sample_interval_us.store(new_interval_us); } void Sampler::set_max_nframes(unsigned int _max_nframes) { - max_nframes = _max_nframes; + if (thread_seq_num.load() == 0) { + // Only change this if the thread isn't running. This is a defensive check since the + // caller, at the moment, might only use this during init anyway. + max_nframes = _max_nframes; + } } Sampler::Sampler() @@ -58,25 +62,21 @@ Sampler::get() } void -Sampler::start() +Sampler::one_time_setup() { - // Do some one-time setup. There's no real reason to leave this to the caller - static bool initialized = false; - if (!initialized) { - _set_cpu(true); - init_frame_cache(echion_frame_cache_size); - _set_pid(getpid()); + _set_cpu(true); + init_frame_cache(echion_frame_cache_size); + _set_pid(getpid()); - // Register our rendering callbacks with echion's Renderer singleton - Renderer::get().set_renderer(renderer_ptr); - - // OK, never initialize again - initialized = true; - } + // Register our rendering callbacks with echion's Renderer singleton + Renderer::get().set_renderer(renderer_ptr); +} - // For simplicity, we just try to stop the sampling thread. Stopping/starting the profiler in - // a tight loop isn't something we really support. - stop(); +void +Sampler::start() +{ + static std::once_flag once; + std::call_once(once, [this]() { this->one_time_setup(); }); // OK, now we can start the profiler. When we launch the profiler, we mutate the sequence number and give it to the // thread. This will (eventually) terminate any outstanding thread. @@ -87,6 +87,7 @@ Sampler::start() void Sampler::stop() { - // Modifying the thread sequence number will cause the sampling thread to exit. + // Modifying the thread sequence number will cause the sampling thread to exit when it completes + // a sampling loop. Currently there is no mechanism to force stuck threads, should they get locked. ++thread_seq_num; } From 749d94c5c93afb070f678c584b76e5e06fef7426 Mon Sep 17 00:00:00 2001 From: sanchda Date: Tue, 27 Feb 2024 13:59:37 +0000 Subject: [PATCH 24/76] Style checks, releasenotes --- .../profiling/dd_wrapper/src/interface.cpp | 30 +++++++++++-------- .../dd_wrapper/src/uploader_builder.cpp | 2 +- .../internal/datadog/profiling/ddup/_ddup.pyx | 23 ++++++++++---- .../profiling/stack_v2/include/sampler.hpp | 6 ++-- .../profiling/stack_v2/src/stack_renderer.cpp | 2 +- ...ofiling-add-stack-v2-975ebe845cdadc8f.yaml | 7 +++++ setup.py | 4 +-- 7 files changed, 48 insertions(+), 26 deletions(-) create mode 100644 releasenotes/notes/profiling-add-stack-v2-975ebe845cdadc8f.yaml diff --git a/ddtrace/internal/datadog/profiling/dd_wrapper/src/interface.cpp b/ddtrace/internal/datadog/profiling/dd_wrapper/src/interface.cpp index a730686a8bc..647a9eb192a 100644 --- a/ddtrace/internal/datadog/profiling/dd_wrapper/src/interface.cpp +++ b/ddtrace/internal/datadog/profiling/dd_wrapper/src/interface.cpp @@ -66,25 +66,25 @@ ddup_config_runtime(std::string_view runtime) void ddup_config_runtime_version(std::string_view runtime_version) { - Datadog::UploaderBuilder::set_runtime_version(runtime_version); + Datadog::UploaderBuilder::set_runtime_version(runtime_version); } void ddup_config_profiler_version(std::string_view profiler_version) { - Datadog::UploaderBuilder::set_profiler_version(profiler_version); + Datadog::UploaderBuilder::set_profiler_version(profiler_version); } void ddup_config_url(std::string_view url) { - Datadog::UploaderBuilder::set_url(url); + Datadog::UploaderBuilder::set_url(url); } void ddup_config_user_tag(std::string_view key, std::string_view val) { - Datadog::UploaderBuilder::set_tag(key, val); + Datadog::UploaderBuilder::set_tag(key, val); } void @@ -179,13 +179,13 @@ ddup_push_heap(Datadog::Sample* sample, uint64_t size) void ddup_push_lock_name(Datadog::Sample* sample, std::string_view lock_name) { - sample->push_lock_name(lock_name); + sample->push_lock_name(lock_name); } void ddup_push_threadinfo(Datadog::Sample* sample, int64_t thread_id, int64_t thread_native_id, std::string_view thread_name) { - sample->push_threadinfo(thread_id, thread_native_id, thread_name); + sample->push_threadinfo(thread_id, thread_native_id, thread_name); } void @@ -197,7 +197,7 @@ ddup_push_task_id(Datadog::Sample* sample, int64_t task_id) void ddup_push_task_name(Datadog::Sample* sample, std::string_view task_name) { - sample->push_task_name(task_name); + sample->push_task_name(task_name); } void @@ -215,31 +215,35 @@ ddup_push_local_root_span_id(Datadog::Sample* sample, int64_t local_root_span_id void ddup_push_trace_type(Datadog::Sample* sample, std::string_view trace_type) { - sample->push_trace_type(trace_type); + sample->push_trace_type(trace_type); } void ddup_push_trace_resource_container(Datadog::Sample* sample, std::string_view trace_resource_container) { - sample->push_trace_resource_container(trace_resource_container); + sample->push_trace_resource_container(trace_resource_container); } void ddup_push_exceptioninfo(Datadog::Sample* sample, std::string_view exception_type, int64_t count) { - sample->push_exceptioninfo(exception_type, count); + sample->push_exceptioninfo(exception_type, count); } void ddup_push_class_name(Datadog::Sample* sample, std::string_view class_name) { - sample->push_class_name(class_name); + sample->push_class_name(class_name); } void -ddup_push_frame(Datadog::Sample* sample, std::string_view _name, std::string_view _filename, uint64_t address, int64_t line) +ddup_push_frame(Datadog::Sample* sample, + std::string_view _name, + std::string_view _filename, + uint64_t address, + int64_t line) { - sample->push_frame(_name, _filename, address, line); + sample->push_frame(_name, _filename, address, line); } void diff --git a/ddtrace/internal/datadog/profiling/dd_wrapper/src/uploader_builder.cpp b/ddtrace/internal/datadog/profiling/dd_wrapper/src/uploader_builder.cpp index 879bb2be3d1..54c69184bad 100644 --- a/ddtrace/internal/datadog/profiling/dd_wrapper/src/uploader_builder.cpp +++ b/ddtrace/internal/datadog/profiling/dd_wrapper/src/uploader_builder.cpp @@ -120,7 +120,7 @@ UploaderBuilder::build() reasons.push_back(std::string(to_string(tag)) + ": " + errmsg); } } else { - reasons.push_back(std::string(to_string(tag)) + ": " + "empty"); + reasons.push_back(std::string(to_string(tag)) + ": " + "empty"); } } diff --git a/ddtrace/internal/datadog/profiling/ddup/_ddup.pyx b/ddtrace/internal/datadog/profiling/ddup/_ddup.pyx index 5a8567a4447..1c0acf39179 100644 --- a/ddtrace/internal/datadog/profiling/ddup/_ddup.pyx +++ b/ddtrace/internal/datadog/profiling/ddup/_ddup.pyx @@ -176,18 +176,28 @@ IF UNAME_SYSNAME == "Linux": def push_frame(self, name: str, filename: str, int address, int line) -> None: if self.ptr is not NULL: - # We've ogtten rreports that `name` and `filename` may be unexpected objects, so we go through a sanitization procedure. - # this is almost certainly wasteful. + # Customers report `name` and `filename` may be unexpected objects, so sanitize. name_bytes = ensure_binary_or_empty(sanitize_string(name)) filename_bytes = ensure_binary_or_empty(sanitize_string(filename)) - ddup_push_frame(self.ptr, string_view(name_bytes, len(name_bytes)), string_view(filename_bytes, len(filename_bytes)), address, line) + ddup_push_frame( + self.ptr, + string_view(name_bytes, len(name_bytes)), + string_view(filename_bytes, len(filename_bytes)), + address, + line + ) def push_threadinfo(self, thread_id: int, thread_native_id: int, thread_name: str) -> None: if self.ptr is not NULL: thread_id = thread_id if thread_id is not None else 0 thread_native_id = thread_native_id if thread_native_id is not None else 0 thread_name_bytes = ensure_binary_or_empty(thread_name) - ddup_push_threadinfo(self.ptr, thread_id, thread_native_id, string_view(thread_name_bytes, len(thread_name_bytes))) + ddup_push_threadinfo( + self.ptr, + thread_id, + thread_native_id, + string_view(thread_name_bytes, len(thread_name_bytes))i + ) def push_task_id(self, task_id: int) -> None: if self.ptr is not NULL: @@ -226,7 +236,10 @@ IF UNAME_SYSNAME == "Linux": ddup_push_trace_type(self.ptr, string_view(span_type_bytes, len(span_type_bytes))) if endpoint_collection_enabled: root_service_bytes = ensure_binary_or_empty(span._local_root.service) - ddup_push_trace_resource_container(self.ptr, string_view(root_service_bytes, len(root_service_bytes))) + ddup_push_trace_resource_container( + self.ptr, + string_view(root_service_bytes, len(root_service_bytes)) + ) def flush_sample(self) -> None: # Flushing the sample consumes it. The user will no longer be able to use diff --git a/ddtrace/internal/datadog/profiling/stack_v2/include/sampler.hpp b/ddtrace/internal/datadog/profiling/stack_v2/include/sampler.hpp index 027f7cbd00d..7a84a452d99 100644 --- a/ddtrace/internal/datadog/profiling/stack_v2/include/sampler.hpp +++ b/ddtrace/internal/datadog/profiling/stack_v2/include/sampler.hpp @@ -8,9 +8,9 @@ namespace Datadog { class Sampler { - // This class manages the initialization of echion as well as the sampling thread. - // The underlying echion instance it manages keeps much of its state globally, so this class is a singleton in order - // to keep it aligned with the echion state. + // This class manages the initialization of echion as well as the sampling thread. + // The underlying echion instance it manages keeps much of its state globally, so this class is a singleton in order + // to keep it aligned with the echion state. private: std::shared_ptr renderer_ptr; 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 0485efd2b2a..ad194bebbf6 100644 --- a/ddtrace/internal/datadog/profiling/stack_v2/src/stack_renderer.cpp +++ b/ddtrace/internal/datadog/profiling/stack_v2/src/stack_renderer.cpp @@ -23,7 +23,7 @@ StackRenderer::render_thread_begin(PyThreadState* tstate, return; } -//#warning stack_v2 should use a C++ interface instead of re-converting intermediates + //#warning stack_v2 should use a C++ interface instead of re-converting intermediates ddup_push_threadinfo(sample, static_cast(thread_id), static_cast(native_id), name); ddup_push_walltime(sample, 1000 * wall_time_us, 1); } diff --git a/releasenotes/notes/profiling-add-stack-v2-975ebe845cdadc8f.yaml b/releasenotes/notes/profiling-add-stack-v2-975ebe845cdadc8f.yaml new file mode 100644 index 00000000000..8e6d1c1aa20 --- /dev/null +++ b/releasenotes/notes/profiling-add-stack-v2-975ebe845cdadc8f.yaml @@ -0,0 +1,7 @@ +--- +features: + - | + profiling: implement an experimental stack sampling feature, which can be enabled by setting + DD_PROFILING_STACK_V2_ENABLED=true. This new sampler should resolve segfault issues on Python + 3.11 and later, while also decreasing the latency contribution of the profiler in many + situations, and also improving the accuracy of stack-sampling data. diff --git a/setup.py b/setup.py index 93fbcd59650..7f1ea23a889 100644 --- a/setup.py +++ b/setup.py @@ -448,9 +448,7 @@ def get_exts_for(name): ) ) - ext_modules.append( - CMakeExtension("ddtrace.appsec._iast._taint_tracking._native", source_dir=IAST_DIR) - ) + ext_modules.append(CMakeExtension("ddtrace.appsec._iast._taint_tracking._native", source_dir=IAST_DIR)) if platform.system() == "Linux" and is_64_bit_python(): # Get Python library paths development paths From ca27591fa5120eb196b56850fc548000d20a67cf Mon Sep 17 00:00:00 2001 From: sanchda Date: Tue, 27 Feb 2024 15:08:31 +0000 Subject: [PATCH 25/76] Make libdd and py mutually exclusive --- .../profiling/stack_v2/src/stack_renderer.cpp | 16 +- ddtrace/internal/telemetry/constants.py | 1 - ddtrace/internal/telemetry/writer.py | 2 - ddtrace/profiling/collector/memalloc.py | 7 +- ddtrace/profiling/collector/stack.pyx | 146 +++++++++--------- ddtrace/profiling/profiler.py | 16 +- ddtrace/profiling/scheduler.py | 6 +- ddtrace/settings/profiling.py | 22 ++- 8 files changed, 98 insertions(+), 118 deletions(-) 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 ad194bebbf6..5433e14ba8d 100644 --- a/ddtrace/internal/datadog/profiling/stack_v2/src/stack_renderer.cpp +++ b/ddtrace/internal/datadog/profiling/stack_v2/src/stack_renderer.cpp @@ -6,7 +6,7 @@ using namespace Datadog; void StackRenderer::render_message(std::string_view msg) { - // This is unused for now, since the dd-trace-py profiler doesn't support emitting arbitrary messages. + // This function is part of the necessary API, but it is unused by the Datadog profiler for now. (void)msg; } @@ -20,6 +20,7 @@ StackRenderer::render_thread_begin(PyThreadState* tstate, (void)tstate; sample = ddup_start_sample(); if (sample == nullptr) { + std::cerr << "Failed to create a sample. Profiling data will be lost." << std::endl; return; } @@ -31,15 +32,17 @@ StackRenderer::render_thread_begin(PyThreadState* tstate, void StackRenderer::render_stack_begin() { - // This is currently unused, the profiler determines state by keeping track of a sample start + // This function is part of the necessary API, but it is unused by the Datadog profiler for now. } void StackRenderer::render_python_frame(std::string_view name, std::string_view file, uint64_t line) { if (sample == nullptr) { + std::cerr << "Failed to create a sample. Profiling data will be lost." << std::endl; return; } + static const std::string_view invalid = ""; if (!utf8_check_is_valid(name.data(), name.size())) { name = invalid; @@ -55,19 +58,17 @@ StackRenderer::render_python_frame(std::string_view name, std::string_view file, void StackRenderer::render_native_frame(std::string_view name, std::string_view file, uint64_t line) { - if (sample == nullptr) { - return; - } - ddup_push_frame(sample, name, file, 0, line); + // This function is part of the necessary API, but it is unused by the Datadog profiler for now. } void StackRenderer::render_cpu_time(microsecond_t cpu_time_us) { - // ddup is configured to expect nanoseconds if (sample == nullptr) { return; } + + // ddup is configured to expect nanoseconds ddup_push_cputime(sample, 1000 * cpu_time_us, 1); } @@ -77,6 +78,7 @@ StackRenderer::render_stack_end() if (sample == nullptr) { return; } + ddup_flush_sample(sample); ddup_drop_sample(sample); sample = nullptr; diff --git a/ddtrace/internal/telemetry/constants.py b/ddtrace/internal/telemetry/constants.py index 8fa2c41dd48..d8f86e0392b 100644 --- a/ddtrace/internal/telemetry/constants.py +++ b/ddtrace/internal/telemetry/constants.py @@ -62,7 +62,6 @@ TELEMETRY_PROFILING_MEMORY_ENABLED = "DD_PROFILING_MEMORY_ENABLED" TELEMETRY_PROFILING_HEAP_ENABLED = "DD_PROFILING_HEAP_ENABLED" TELEMETRY_PROFILING_LOCK_ENABLED = "DD_PROFILING_LOCK_ENABLED" -TELEMETRY_PROFILING_EXPORT_PY_ENABLED = "DD_PROFILING_EXPORT_PY_ENABLED" TELEMETRY_PROFILING_EXPORT_LIBDD_ENABLED = "DD_PROFILING_EXPORT_LIBDD_ENABLED" TELEMETRY_PROFILING_CAPTURE_PCT = "DD_PROFILING_CAPTURE_PCT" TELEMETRY_PROFILING_UPLOAD_INTERVAL = "DD_PROFILING_UPLOAD_INTERVAL" diff --git a/ddtrace/internal/telemetry/writer.py b/ddtrace/internal/telemetry/writer.py index f8ef9ff0830..2582c08e83c 100644 --- a/ddtrace/internal/telemetry/writer.py +++ b/ddtrace/internal/telemetry/writer.py @@ -59,7 +59,6 @@ from .constants import TELEMETRY_PRIORITY_SAMPLING from .constants import TELEMETRY_PROFILING_CAPTURE_PCT from .constants import TELEMETRY_PROFILING_EXPORT_LIBDD_ENABLED -from .constants import TELEMETRY_PROFILING_EXPORT_PY_ENABLED from .constants import TELEMETRY_PROFILING_HEAP_ENABLED from .constants import TELEMETRY_PROFILING_LOCK_ENABLED from .constants import TELEMETRY_PROFILING_MAX_FRAMES @@ -449,7 +448,6 @@ def _app_started_event(self, register_app_shutdown=True): (TELEMETRY_PROFILING_MEMORY_ENABLED, prof_config.memory.enabled, "unknown"), (TELEMETRY_PROFILING_HEAP_ENABLED, prof_config.heap.sample_size > 0, "unknown"), (TELEMETRY_PROFILING_LOCK_ENABLED, prof_config.lock.enabled, "unknown"), - (TELEMETRY_PROFILING_EXPORT_PY_ENABLED, prof_config.export.py_enabled, "unknown"), (TELEMETRY_PROFILING_EXPORT_LIBDD_ENABLED, prof_config.export.libdd_enabled, "unknown"), (TELEMETRY_PROFILING_CAPTURE_PCT, prof_config.capture_pct, "unknown"), (TELEMETRY_PROFILING_MAX_FRAMES, prof_config.max_frames, "unknown"), diff --git a/ddtrace/profiling/collector/memalloc.py b/ddtrace/profiling/collector/memalloc.py index d8ab54e9d38..4ab0a36ee14 100644 --- a/ddtrace/profiling/collector/memalloc.py +++ b/ddtrace/profiling/collector/memalloc.py @@ -64,7 +64,6 @@ class MemoryCollector(collector.PeriodicCollector): heap_sample_size = attr.ib(type=int, default=config.heap.sample_size) ignore_profiler = attr.ib(default=config.ignore_profiler, type=bool) _export_libdd_enabled = attr.ib(type=bool, default=config.export.libdd_enabled) - _export_py_enabled = attr.ib(type=bool, default=config.export.py_enabled) def _start_service(self): # type: (...) -> None @@ -128,8 +127,7 @@ def snapshot(self): # DEV: This might happen if the memalloc sofile is unlinked and relinked without module # re-initialization. LOG.debug("Invalid state detected in memalloc module, suppressing profile") - - if self._export_py_enabled: + else: return ( tuple( MemoryHeapSampleEvent( @@ -182,8 +180,7 @@ def collect(self): # DEV: This might happen if the memalloc sofile is unlinked and relinked without module # re-initialization. LOG.debug("Invalid state detected in memalloc module, suppressing profile") - - if self._export_py_enabled: + else: return ( tuple( MemoryAllocSampleEvent( diff --git a/ddtrace/profiling/collector/stack.pyx b/ddtrace/profiling/collector/stack.pyx index dd96e23bf97..ffae179cf73 100644 --- a/ddtrace/profiling/collector/stack.pyx +++ b/ddtrace/profiling/collector/stack.pyx @@ -33,19 +33,14 @@ FEATURES = { "transparent_events": False, } -# These are flags indicating the enablement of the profiler. This is handled at the level of +# . This is handled at the level of # a global rather than a passed parameter because this is a time of transition cdef bint use_libdd = False -cdef bint use_py = True cdef void set_use_libdd(bint flag): global use_libdd use_libdd = flag -cdef void set_use_py(bint flag): - global use_py - use_py = flag - IF UNAME_SYSNAME == "Linux": FEATURES['cpu-time'] = True @@ -328,89 +323,89 @@ cdef stack_collect(ignore_profiler, thread_time, max_nframes, interval, wall_tim frames, nframes = _traceback.pyframe_to_frames(task_pyframes, max_nframes) - if use_libdd and nframes: - handle = ddup.SampleHandle() - handle.push_walltime(wall_time, 1) - handle.push_threadinfo(thread_id, thread_native_id, thread_name) - handle.push_task_id(task_id) - handle.push_task_name(task_name) - handle.push_class_name(frames[0].class_name) - for frame in frames: - handle.push_frame(frame.function_name, frame.file_name, 0, frame.lineno) - handle.flush_sample() - - if use_py and nframes: - stack_events.append( - stack_event.StackSampleEvent( - thread_id=thread_id, - thread_native_id=thread_native_id, - thread_name=thread_name, - task_id=task_id, - task_name=task_name, - nframes=nframes, frames=frames, - wall_time_ns=wall_time, - sampling_period=int(interval * 1e9), + if nframes: + if use_libdd: + handle = ddup.SampleHandle() + handle.push_walltime(wall_time, 1) + handle.push_threadinfo(thread_id, thread_native_id, thread_name) + handle.push_task_id(task_id) + handle.push_task_name(task_name) + handle.push_class_name(frames[0].class_name) + for frame in frames: + handle.push_frame(frame.function_name, frame.file_name, 0, frame.lineno) + handle.flush_sample() + else: + stack_events.append( + stack_event.StackSampleEvent( + thread_id=thread_id, + thread_native_id=thread_native_id, + thread_name=thread_name, + task_id=task_id, + task_name=task_name, + nframes=nframes, frames=frames, + wall_time_ns=wall_time, + sampling_period=int(interval * 1e9), + ) ) - ) frames, nframes = _traceback.pyframe_to_frames(thread_pyframes, max_nframes) - if use_libdd and nframes: - handle = ddup.SampleHandle() - handle.push_cputime( cpu_time, 1) - handle.push_walltime( wall_time, 1) - handle.push_threadinfo(thread_id, thread_native_id, thread_name) - handle.push_class_name(frames[0].class_name) - for frame in frames: - handle.push_frame(frame.function_name, frame.file_name, 0, frame.lineno) - handle.push_span(span, collect_endpoint) - handle.flush_sample() - - if use_py and nframes: - event = stack_event.StackSampleEvent( - thread_id=thread_id, - thread_native_id=thread_native_id, - thread_name=thread_name, - task_id=None, - task_name=None, - nframes=nframes, - frames=frames, - wall_time_ns=wall_time, - cpu_time_ns=cpu_time, - sampling_period=int(interval * 1e9), - ) - event.set_trace_info(span, collect_endpoint) - stack_events.append(event) - - if exception is not None: - exc_type, exc_traceback = exception - - frames, nframes = _traceback.traceback_to_frames(exc_traceback, max_nframes) - - if use_libdd and nframes: + if nframes: + if use_libdd: handle = ddup.SampleHandle() + handle.push_cputime( cpu_time, 1) + handle.push_walltime( wall_time, 1) handle.push_threadinfo(thread_id, thread_native_id, thread_name) - handle.push_exceptioninfo(exc_type, 1) handle.push_class_name(frames[0].class_name) for frame in frames: handle.push_frame(frame.function_name, frame.file_name, 0, frame.lineno) handle.push_span(span, collect_endpoint) handle.flush_sample() - - if use_py and nframes: - exc_event = stack_event.StackExceptionSampleEvent( + else: + event = stack_event.StackSampleEvent( thread_id=thread_id, - thread_name=thread_name, thread_native_id=thread_native_id, + thread_name=thread_name, task_id=None, task_name=None, nframes=nframes, frames=frames, + wall_time_ns=wall_time, + cpu_time_ns=cpu_time, sampling_period=int(interval * 1e9), - exc_type=exc_type, ) - exc_event.set_trace_info(span, collect_endpoint) - exc_events.append(exc_event) + event.set_trace_info(span, collect_endpoint) + stack_events.append(event) + + if exception is not None: + exc_type, exc_traceback = exception + + frames, nframes = _traceback.traceback_to_frames(exc_traceback, max_nframes) + + if nframes: + if use_libdd: + handle = ddup.SampleHandle() + handle.push_threadinfo(thread_id, thread_native_id, thread_name) + handle.push_exceptioninfo(exc_type, 1) + handle.push_class_name(frames[0].class_name) + for frame in frames: + handle.push_frame(frame.function_name, frame.file_name, 0, frame.lineno) + handle.push_span(span, collect_endpoint) + handle.flush_sample() + else: + exc_event = stack_event.StackExceptionSampleEvent( + thread_id=thread_id, + thread_name=thread_name, + thread_native_id=thread_native_id, + task_id=None, + task_name=None, + nframes=nframes, + frames=frames, + sampling_period=int(interval * 1e9), + exc_type=exc_type, + ) + exc_event.set_trace_info(span, collect_endpoint) + exc_events.append(exc_event) return stack_events, exc_events @@ -489,11 +484,12 @@ class StackCollector(collector.PeriodicCollector): if self.tracer is not None: self._thread_span_links = _ThreadSpanLinks() self.tracer.context_provider._on_activate(self._thread_span_links.link_span) - set_use_libdd(config.export.libdd_enabled) - set_use_py(config.export.py_enabled) - if self._stack_collector_v2_enabled and use_libdd: - stack_v2.start(max_frames=self.nframes, min_interval=self.min_interval_time) + # Force-set use_libdd if the v2 stack collector is enabled + if self._stack_collector_v2_enabled: + set_use_libdd(True) + else: + set_use_libdd(config.export.libdd_enabled) def _start_service(self): # type: (...) -> None @@ -512,7 +508,7 @@ class StackCollector(collector.PeriodicCollector): LOG.debug("Profiling StackCollector stopped") # Also tell the native thread running the v2 sampler to stop, if needed - if self._stack_collector_v2_enabled and use_libdd: + if self._stack_collector_v2_enabled: stack_v2.stop() def _compute_new_interval(self, used_wall_time_ns): diff --git a/ddtrace/profiling/profiler.py b/ddtrace/profiling/profiler.py index 0b5d9b87f40..85383f5bbe2 100644 --- a/ddtrace/profiling/profiler.py +++ b/ddtrace/profiling/profiler.py @@ -128,7 +128,6 @@ class _ProfilerInstance(service.Service): init=False, factory=lambda: os.environ.get("AWS_LAMBDA_FUNCTION_NAME"), type=Optional[str] ) _export_libdd_enabled = attr.ib(type=bool, default=config.export.libdd_enabled) - _export_py_enabled = attr.ib(type=bool, default=config.export.py_enabled) ENDPOINT_TEMPLATE = "https://intake.profile.{}" @@ -179,9 +178,10 @@ def _build_default_exporters(self): configured_features.append("mem") if config.heap.sample_size > 0: configured_features.append("heap") + if self._export_libdd_enabled: configured_features.append("exp_dd") - if self._export_py_enabled: + else: configured_features.append("exp_py") configured_features.append("CAP" + str(config.capture_pct)) configured_features.append("MAXF" + str(config.max_frames)) @@ -192,21 +192,15 @@ def _build_default_exporters(self): endpoint_call_counter_span_processor.enable() if self._export_libdd_enabled: - versionname = ( - "{}.libdd".format(self.version) - if self._export_py_enabled and self.version is not None - else self.version - ) ddup.init( env=self.env, service=self.service, - version=versionname, + version=self.version, tags=self.tags, max_nframes=config.max_frames, url=endpoint, ) - - if self._export_py_enabled: + else: # DEV: Import this only if needed to avoid importing protobuf # unnecessarily from ddtrace.profiling.exporter import http @@ -380,4 +374,4 @@ def _stop_service(self, flush=True, join=True): col.join() def visible_events(self): - return self._export_py_enabled + return not self._export_libdd_enabled diff --git a/ddtrace/profiling/scheduler.py b/ddtrace/profiling/scheduler.py index b47c73a065e..d2fa9e76363 100644 --- a/ddtrace/profiling/scheduler.py +++ b/ddtrace/profiling/scheduler.py @@ -25,7 +25,6 @@ class Scheduler(periodic.PeriodicService): _configured_interval = attr.ib(init=False) _last_export = attr.ib(init=False, default=None, eq=False) _export_libdd_enabled = attr.ib(type=bool, default=config.export.libdd_enabled) - _export_py_enabled = attr.ib(type=bool, default=config.export.py_enabled) def __attrs_post_init__(self): # Copy the value to use it later since we're going to adjust the real interval @@ -45,9 +44,8 @@ def flush(self): if self._export_libdd_enabled: ddup.upload() - if not self._export_py_enabled: - # If we're not using the Python profiler, then stop now - # But set these fields for compatibility + # These are only used by the Python uploader, but set them here to keep logs/etc + # consistent for now start = self._last_export self._last_export = compat.time_ns() return diff --git a/ddtrace/settings/profiling.py b/ddtrace/settings/profiling.py index 2f5e0e2077e..a8cd74e9c4c 100644 --- a/ddtrace/settings/profiling.py +++ b/ddtrace/settings/profiling.py @@ -40,7 +40,11 @@ def _derive_default_heap_sample_size(heap_config, default_heap_sample_size=1024 def _is_valid_libdatadog(): # type: () -> bool - return platform.machine() in ["x86_64", "aarch64"] and "glibc" in platform.libc_ver()[0] + return platform.system() == "Linux" + +def _is_valid_v2_stack(): + # type: () -> bool + return platform.python_version_tuple() >= ("3", "7") and _is_valid_libdatadog() class ProfilingConfig(En): @@ -168,13 +172,14 @@ class Stack(En): class V2(En): __item__ = __prefix__ = "v2" - enabled = En.v( + _enabled = En.v( bool, "enabled", default=False, help_type="Boolean", - help="Whether to enable the v2 stack profiler", + help="Whether to enable the v2 stack profiler. Also enables the libdatadog collector.", ) + enabled = En.d(bool, lambda c: c._enabled and _is_valid_v2_stack()) class Lock(En): __item__ = __prefix__ = "lock" @@ -237,16 +242,7 @@ class Export(En): help="Enables collection and export using the experimental exporter", ) - # For now, only allow libdd to be enabled if the user asks for it + # Only available in certain configurations libdd_enabled = En.d(bool, lambda c: c._libdd_enabled and _is_valid_libdatadog()) - py_enabled = En.v( - bool, - "py_enabled", - default=True, - help_type="Boolean", - help="Enables collection and export using the classic Python exporter", - ) - - config = ProfilingConfig() From 5198cc10eb3791f35cfdd94f48282328afd7f1ea Mon Sep 17 00:00:00 2001 From: sanchda Date: Tue, 27 Feb 2024 15:26:52 +0000 Subject: [PATCH 26/76] Style and fixups --- ddtrace/profiling/collector/memalloc.py | 6 ++---- ddtrace/settings/profiling.py | 2 ++ 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/ddtrace/profiling/collector/memalloc.py b/ddtrace/profiling/collector/memalloc.py index 4ab0a36ee14..5e81b58a126 100644 --- a/ddtrace/profiling/collector/memalloc.py +++ b/ddtrace/profiling/collector/memalloc.py @@ -127,6 +127,7 @@ def snapshot(self): # DEV: This might happen if the memalloc sofile is unlinked and relinked without module # re-initialization. LOG.debug("Invalid state detected in memalloc module, suppressing profile") + return tuple() else: return ( tuple( @@ -143,8 +144,6 @@ def snapshot(self): if not self.ignore_profiler or thread_id not in thread_id_ignore_set ), ) - else: - return tuple() def collect(self): # TODO: The event timestamp is slightly off since it's going to be the time we copy the data from the @@ -180,6 +179,7 @@ def collect(self): # DEV: This might happen if the memalloc sofile is unlinked and relinked without module # re-initialization. LOG.debug("Invalid state detected in memalloc module, suppressing profile") + return [] else: return ( tuple( @@ -197,5 +197,3 @@ def collect(self): if not self.ignore_profiler or thread_id not in thread_id_ignore_set ), ) - else: - return [] diff --git a/ddtrace/settings/profiling.py b/ddtrace/settings/profiling.py index a8cd74e9c4c..9a6f66f3c49 100644 --- a/ddtrace/settings/profiling.py +++ b/ddtrace/settings/profiling.py @@ -42,6 +42,7 @@ def _is_valid_libdatadog(): # type: () -> bool return platform.system() == "Linux" + def _is_valid_v2_stack(): # type: () -> bool return platform.python_version_tuple() >= ("3", "7") and _is_valid_libdatadog() @@ -245,4 +246,5 @@ class Export(En): # Only available in certain configurations libdd_enabled = En.d(bool, lambda c: c._libdd_enabled and _is_valid_libdatadog()) + config = ProfilingConfig() From 2d6dd5a20690cc145b65df45c7eda15cf61ccb0f Mon Sep 17 00:00:00 2001 From: sanchda Date: Tue, 27 Feb 2024 17:17:25 +0000 Subject: [PATCH 27/76] Kludge to help expose some info in CI --- setup.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/setup.py b/setup.py index 7f1ea23a889..e5a1929b40b 100644 --- a/setup.py +++ b/setup.py @@ -473,7 +473,7 @@ def get_exts_for(name): ext_modules.append( CMakeExtension( "ddtrace.internal.datadog.profiling.stack_v2", - optional=False, + optional=CURRENT_OS != "Linux" or sys.version_info < (3, 8), source_dir=STACK_V2_DIR, cmake_args=[ "-DPython3_INCLUDE_DIRS={}".format(python_include), From 7cb4e313dc93fdf22a8c51389644480f38f844be Mon Sep 17 00:00:00 2001 From: sanchda Date: Tue, 27 Feb 2024 17:26:21 +0000 Subject: [PATCH 28/76] Fix typo --- ddtrace/internal/datadog/profiling/ddup/_ddup.pyx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ddtrace/internal/datadog/profiling/ddup/_ddup.pyx b/ddtrace/internal/datadog/profiling/ddup/_ddup.pyx index 1c0acf39179..94521a888e4 100644 --- a/ddtrace/internal/datadog/profiling/ddup/_ddup.pyx +++ b/ddtrace/internal/datadog/profiling/ddup/_ddup.pyx @@ -196,7 +196,7 @@ IF UNAME_SYSNAME == "Linux": self.ptr, thread_id, thread_native_id, - string_view(thread_name_bytes, len(thread_name_bytes))i + string_view(thread_name_bytes, len(thread_name_bytes)) ) def push_task_id(self, task_id: int) -> None: From e2912dc3f2b415d84e22aa2a36eac5ea392777e2 Mon Sep 17 00:00:00 2001 From: sanchda Date: Tue, 27 Feb 2024 17:29:19 +0000 Subject: [PATCH 29/76] Fix warnings --- .../internal/datadog/profiling/stack_v2/src/stack_renderer.cpp | 3 +++ 1 file changed, 3 insertions(+) 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 5433e14ba8d..05177486944 100644 --- a/ddtrace/internal/datadog/profiling/stack_v2/src/stack_renderer.cpp +++ b/ddtrace/internal/datadog/profiling/stack_v2/src/stack_renderer.cpp @@ -59,6 +59,9 @@ void StackRenderer::render_native_frame(std::string_view name, std::string_view file, uint64_t line) { // This function is part of the necessary API, but it is unused by the Datadog profiler for now. + (void)name; + (void)file; + (void)line; } void From 4948cae83e105baf74f90a5aaa5208ee76d12449 Mon Sep 17 00:00:00 2001 From: sanchda Date: Tue, 27 Feb 2024 17:49:34 +0000 Subject: [PATCH 30/76] Get better visibility into smoketest breaks --- setup.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/setup.py b/setup.py index e5a1929b40b..21024a15355 100644 --- a/setup.py +++ b/setup.py @@ -347,7 +347,7 @@ def __init__( build_args=[], install_args=[], build_type=None, - optional=True, # By default, extensions are optional for now + optional=False # By default, extensions are optional for now TODO make this true lol ): super().__init__(name, sources=[]) self.source_dir = source_dir From 98c73537046e9cfa8a30a82b9d455a4bb3420a27 Mon Sep 17 00:00:00 2001 From: sanchda Date: Tue, 27 Feb 2024 18:36:15 +0000 Subject: [PATCH 31/76] Roll back temp measures, update changelog --- ...ofiling-add-stack-v2-975ebe845cdadc8f.yaml | 3 ++- setup.py | 27 +++++++++---------- 2 files changed, 15 insertions(+), 15 deletions(-) diff --git a/releasenotes/notes/profiling-add-stack-v2-975ebe845cdadc8f.yaml b/releasenotes/notes/profiling-add-stack-v2-975ebe845cdadc8f.yaml index 8e6d1c1aa20..305f5bc96fc 100644 --- a/releasenotes/notes/profiling-add-stack-v2-975ebe845cdadc8f.yaml +++ b/releasenotes/notes/profiling-add-stack-v2-975ebe845cdadc8f.yaml @@ -4,4 +4,5 @@ features: profiling: implement an experimental stack sampling feature, which can be enabled by setting DD_PROFILING_STACK_V2_ENABLED=true. This new sampler should resolve segfault issues on Python 3.11 and later, while also decreasing the latency contribution of the profiler in many - situations, and also improving the accuracy of stack-sampling data. + situations, and also improving the accuracy of stack-sampling data. This feature is currently + only available on Linux using cpython 3.8 or greater. diff --git a/setup.py b/setup.py index 21024a15355..2bfefdd68f4 100644 --- a/setup.py +++ b/setup.py @@ -347,7 +347,7 @@ def __init__( build_args=[], install_args=[], build_type=None, - optional=False # By default, extensions are optional for now TODO make this true lol + optional=True # By default, extensions are optional ): super().__init__(name, sources=[]) self.source_dir = source_dir @@ -399,7 +399,6 @@ def get_exts_for(name): encoding_macros = [("__LITTLE_ENDIAN__", "1")] -# TODO can we specify the exact compiler version less literally? if CURRENT_OS == "Windows": encoding_libraries = ["ws2_32"] extra_compile_args = [] @@ -459,7 +458,6 @@ def get_exts_for(name): CMakeExtension( "ddtrace.internal.datadog.profiling.ddup._ddup", source_dir=DDUP_DIR, - optional=False, cmake_args=[ "-DPY_MAJOR_VERSION={}".format(sys.version_info.major), "-DPY_MINOR_VERSION={}".format(sys.version_info.minor), @@ -470,18 +468,19 @@ def get_exts_for(name): ) ) - ext_modules.append( - CMakeExtension( - "ddtrace.internal.datadog.profiling.stack_v2", - optional=CURRENT_OS != "Linux" or sys.version_info < (3, 8), - source_dir=STACK_V2_DIR, - cmake_args=[ - "-DPython3_INCLUDE_DIRS={}".format(python_include), - "-DPYTHON_EXECUTABLE={}".format(sys.executable), - ], - build_args=["--verbose"], + # Don't even try building on Python 3.7 + if sys.version_info >= (3, 8): + ext_modules.append( + CMakeExtension( + "ddtrace.internal.datadog.profiling.stack_v2", + source_dir=STACK_V2_DIR, + cmake_args=[ + "-DPython3_INCLUDE_DIRS={}".format(python_include), + "-DPYTHON_EXECUTABLE={}".format(sys.executable), + ], + build_args=["--verbose"], + ) ) - ) else: ext_modules = [] From d606fbb50cef26c843dc251576ba9e1f7aae2d64 Mon Sep 17 00:00:00 2001 From: sanchda Date: Tue, 27 Feb 2024 21:06:00 +0000 Subject: [PATCH 32/76] Refactor interval stuff --- .../profiling/stack_v2/include/sampler.hpp | 8 +++---- .../profiling/stack_v2/src/sampler.cpp | 24 ++++++++----------- .../profiling/stack_v2/src/stack_v2.cpp | 6 ++--- ddtrace/profiling/collector/stack.pyx | 8 +++++++ setup.py | 2 +- 5 files changed, 24 insertions(+), 24 deletions(-) diff --git a/ddtrace/internal/datadog/profiling/stack_v2/include/sampler.hpp b/ddtrace/internal/datadog/profiling/stack_v2/include/sampler.hpp index 7a84a452d99..6419551d059 100644 --- a/ddtrace/internal/datadog/profiling/stack_v2/include/sampler.hpp +++ b/ddtrace/internal/datadog/profiling/stack_v2/include/sampler.hpp @@ -20,14 +20,13 @@ class Sampler // This is not a running total of the number of launched threads; it is a sequence for the // transactions upon the sampling threads (usually starts + stops). This allows threads to be // stopped or started in a straightforward manner without finer-grained control (locks) - std::atomic thread_seq_num{ 0 }; + std::atomic thread_seq_num{ 0 }; // Parameters - unsigned int echion_frame_cache_size = g_default_echion_frame_cache_size; - unsigned int max_nframes = g_default_max_nframes; + uint64_t echion_frame_cache_size = g_default_echion_frame_cache_size; // Helper function; implementation of the echion sampling thread - void sampling_thread(unsigned long seq_num); + void sampling_thread(uint64_t seq_num); // This is a singleton, so no public constructor Sampler(); @@ -47,7 +46,6 @@ class Sampler // we're not currently accounting for the echion self-time. void set_interval(double new_interval); - void set_max_nframes(unsigned int _max_nfames); }; } // namespace Datadog diff --git a/ddtrace/internal/datadog/profiling/stack_v2/src/sampler.cpp b/ddtrace/internal/datadog/profiling/stack_v2/src/sampler.cpp index a7afb7f4baa..89d12072baf 100644 --- a/ddtrace/internal/datadog/profiling/stack_v2/src/sampler.cpp +++ b/ddtrace/internal/datadog/profiling/stack_v2/src/sampler.cpp @@ -7,7 +7,7 @@ using namespace Datadog; void -Sampler::sampling_thread(unsigned long seq_num) +Sampler::sampling_thread(uint64_t seq_num) { using namespace std::chrono; auto sample_time_prev = steady_clock::now(); @@ -24,6 +24,11 @@ Sampler::sampling_thread(unsigned long seq_num) }); }); + // Before sleeping, check whether the user has called for this thread to die. + if (seq_num != thread_seq_num.load()) { + break; + } + // Sleep for the remainder of the interval, get it atomically // Generally speaking system "sleep" times will wait _at least_ as long as the specified time, so // in actual fact the duration may be more than we indicated. This tends to be more true on busy @@ -35,20 +40,10 @@ Sampler::sampling_thread(unsigned long seq_num) void Sampler::set_interval(double new_interval_s) { - microsecond_t new_interval_us = static_cast(new_interval_s * 1e6); + microsecond_t new_interval_us = static_cast(new_interval_s * 1e6); sample_interval_us.store(new_interval_us); } -void -Sampler::set_max_nframes(unsigned int _max_nframes) -{ - if (thread_seq_num.load() == 0) { - // Only change this if the thread isn't running. This is a defensive check since the - // caller, at the moment, might only use this during init anyway. - max_nframes = _max_nframes; - } -} - Sampler::Sampler() { renderer_ptr = std::make_shared(); @@ -78,8 +73,9 @@ Sampler::start() static std::once_flag once; std::call_once(once, [this]() { this->one_time_setup(); }); - // OK, now we can start the profiler. When we launch the profiler, we mutate the sequence number and give it to the - // thread. This will (eventually) terminate any outstanding thread. + // Launch the sampling thread. + // Thread lifetime is bounded by the value of the sequence number. When it is changed from the value the thread was + // launched with, the thread will exit. std::thread t(&Sampler::sampling_thread, this, ++thread_seq_num); t.detach(); } diff --git a/ddtrace/internal/datadog/profiling/stack_v2/src/stack_v2.cpp b/ddtrace/internal/datadog/profiling/stack_v2/src/stack_v2.cpp index 60234f53a48..9045a2206f0 100644 --- a/ddtrace/internal/datadog/profiling/stack_v2/src/stack_v2.cpp +++ b/ddtrace/internal/datadog/profiling/stack_v2/src/stack_v2.cpp @@ -8,17 +8,15 @@ static PyObject* _stack_v2_start(PyObject* self, PyObject* args, PyObject* kwargs) { (void)self; - static const char* const_kwlist[] = { "min_interval", "max_frames", NULL }; + static const char* const_kwlist[] = { "min_interval", NULL }; static char** kwlist = const_cast(const_kwlist); double min_interval_s = g_default_sampling_period_s; - double max_nframes = g_default_max_nframes; - if (!PyArg_ParseTupleAndKeywords(args, kwargs, "|dd", kwlist, &min_interval_s, &max_nframes)) { + if (!PyArg_ParseTupleAndKeywords(args, kwargs, "|d", kwlist, &min_interval_s)) { return NULL; // If an error occurs during argument parsing } Sampler::get().set_interval(min_interval_s); - Sampler::get().set_max_nframes(max_nframes); return PyLong_FromLong(1); } diff --git a/ddtrace/profiling/collector/stack.pyx b/ddtrace/profiling/collector/stack.pyx index ffae179cf73..a9670f00735 100644 --- a/ddtrace/profiling/collector/stack.pyx +++ b/ddtrace/profiling/collector/stack.pyx @@ -488,6 +488,11 @@ class StackCollector(collector.PeriodicCollector): # Force-set use_libdd if the v2 stack collector is enabled if self._stack_collector_v2_enabled: set_use_libdd(True) + + # Restarting the profiler is infrequent; always start with the minimum interval + # instead of the dynamically configured one. The difference will get picked up + # by the next sample. + stack_v2.start(min_interval=self.min_interval_time) else: set_use_libdd(config.export.libdd_enabled) @@ -534,4 +539,7 @@ class StackCollector(collector.PeriodicCollector): used_wall_time_ns = compat.monotonic_ns() - now self.interval = self._compute_new_interval(used_wall_time_ns) + if self.stack_collector_v2_enabled: + stack_v2.set_interval(self.interval) + return all_events diff --git a/setup.py b/setup.py index 2bfefdd68f4..ab5fa611e90 100644 --- a/setup.py +++ b/setup.py @@ -468,7 +468,7 @@ def get_exts_for(name): ) ) - # Don't even try building on Python 3.7 + # One of the stack v2 dependencies doesn't quite work on 3.7, so only support later for now if sys.version_info >= (3, 8): ext_modules.append( CMakeExtension( From 8aff7b2b24b62e0a2ab76a17acb81a2a5eca6a32 Mon Sep 17 00:00:00 2001 From: sanchda Date: Wed, 28 Feb 2024 09:48:42 +0000 Subject: [PATCH 33/76] Move python declarations back into cmake harness --- setup.py | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/setup.py b/setup.py index ab5fa611e90..a79a22f5858 100644 --- a/setup.py +++ b/setup.py @@ -290,11 +290,18 @@ def build_extension_cmake(self, ext): cmake_build_dir = Path(self.build_lib.replace("lib.", "cmake."), ext.name).resolve() cmake_build_dir.mkdir(parents=True, exist_ok=True) + # Get development paths + python_include = sysconfig.get_paths()["include"] + python_lib = sysconfig.get_config_var("LIBDIR") + # Which commands are passed to _every_ cmake invocation cmake_args = ext.cmake_args or [] cmake_args += [ "-S{}".format(ext.source_dir), # cmake>=3.13 "-B{}".format(cmake_build_dir), # cmake>=3.13 + "-DPython3_INCLUDE_DIRS={}".format(python_include), + "-DPython3_LIBRARIES={}".format(python_lib), + "-DPYTHON_EXECUTABLE={}".format(sys.executable), "-DCMAKE_BUILD_TYPE={}".format(ext.build_type), "-DCMAKE_LIBRARY_OUTPUT_DIRECTORY={}".format(output_dir), "-DLIB_INSTALL_DIR={}".format(output_dir), @@ -450,10 +457,6 @@ def get_exts_for(name): ext_modules.append(CMakeExtension("ddtrace.appsec._iast._taint_tracking._native", source_dir=IAST_DIR)) if platform.system() == "Linux" and is_64_bit_python(): - # Get Python library paths development paths - python_include = sysconfig.get_paths()["include"] - python_lib = sysconfig.get_config_var("LIBDIR") - ext_modules.append( CMakeExtension( "ddtrace.internal.datadog.profiling.ddup._ddup", @@ -462,8 +465,6 @@ def get_exts_for(name): "-DPY_MAJOR_VERSION={}".format(sys.version_info.major), "-DPY_MINOR_VERSION={}".format(sys.version_info.minor), "-DPY_MICRO_VERSION={}".format(sys.version_info.micro), - "-DPython3_INCLUDE_DIRS={}".format(python_include), - "-DPYTHON_EXECUTABLE={}".format(sys.executable), ], ) ) @@ -474,10 +475,6 @@ def get_exts_for(name): CMakeExtension( "ddtrace.internal.datadog.profiling.stack_v2", source_dir=STACK_V2_DIR, - cmake_args=[ - "-DPython3_INCLUDE_DIRS={}".format(python_include), - "-DPYTHON_EXECUTABLE={}".format(sys.executable), - ], build_args=["--verbose"], ) ) From d4d86df2057e93a3c749e4a1e3c1b15ed0284c09 Mon Sep 17 00:00:00 2001 From: sanchda Date: Wed, 28 Feb 2024 11:00:24 +0000 Subject: [PATCH 34/76] Const correctness, formatting --- ddtrace/internal/datadog/profiling/stack_v2/include/sampler.hpp | 2 +- ddtrace/internal/datadog/profiling/stack_v2/src/sampler.cpp | 2 +- setup.py | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/ddtrace/internal/datadog/profiling/stack_v2/include/sampler.hpp b/ddtrace/internal/datadog/profiling/stack_v2/include/sampler.hpp index 6419551d059..3869138a997 100644 --- a/ddtrace/internal/datadog/profiling/stack_v2/include/sampler.hpp +++ b/ddtrace/internal/datadog/profiling/stack_v2/include/sampler.hpp @@ -26,7 +26,7 @@ class Sampler uint64_t echion_frame_cache_size = g_default_echion_frame_cache_size; // Helper function; implementation of the echion sampling thread - void sampling_thread(uint64_t seq_num); + void sampling_thread(const uint64_t seq_num); // This is a singleton, so no public constructor Sampler(); diff --git a/ddtrace/internal/datadog/profiling/stack_v2/src/sampler.cpp b/ddtrace/internal/datadog/profiling/stack_v2/src/sampler.cpp index 89d12072baf..0e9e4a99e05 100644 --- a/ddtrace/internal/datadog/profiling/stack_v2/src/sampler.cpp +++ b/ddtrace/internal/datadog/profiling/stack_v2/src/sampler.cpp @@ -7,7 +7,7 @@ using namespace Datadog; void -Sampler::sampling_thread(uint64_t seq_num) +Sampler::sampling_thread(const uint64_t seq_num) { using namespace std::chrono; auto sample_time_prev = steady_clock::now(); diff --git a/setup.py b/setup.py index a79a22f5858..9a4fddd7a7c 100644 --- a/setup.py +++ b/setup.py @@ -354,7 +354,7 @@ def __init__( build_args=[], install_args=[], build_type=None, - optional=True # By default, extensions are optional + optional=True, # By default, extensions are optional ): super().__init__(name, sources=[]) self.source_dir = source_dir From 50c06d89f3d0b10cf0af1c2fe7d81d4f86f2f7ff Mon Sep 17 00:00:00 2001 From: sanchda Date: Wed, 28 Feb 2024 11:12:10 +0000 Subject: [PATCH 35/76] style --- ddtrace/internal/datadog/profiling/stack_v2/include/sampler.hpp | 1 - 1 file changed, 1 deletion(-) diff --git a/ddtrace/internal/datadog/profiling/stack_v2/include/sampler.hpp b/ddtrace/internal/datadog/profiling/stack_v2/include/sampler.hpp index 3869138a997..9ebb76c4085 100644 --- a/ddtrace/internal/datadog/profiling/stack_v2/include/sampler.hpp +++ b/ddtrace/internal/datadog/profiling/stack_v2/include/sampler.hpp @@ -45,7 +45,6 @@ class Sampler // the next rate with the latest interval. This is not perfect because the adjustment is based on self-time, and // we're not currently accounting for the echion self-time. void set_interval(double new_interval); - }; } // namespace Datadog From 4ca6a22b59464df5eb953c66b1ba7ba947a70796 Mon Sep 17 00:00:00 2001 From: sanchda Date: Wed, 28 Feb 2024 12:48:21 +0000 Subject: [PATCH 36/76] cppcheck cleanup --- .../profiling/cmake/AnalysisFunc.cmake | 5 -- .../profiling/cmake/FindCppcheck.cmake | 54 ++++++++++++++----- .../profiling/dd_wrapper/CMakeLists.txt | 9 ++++ .../dd_wrapper/src/uploader_builder.cpp | 2 +- .../datadog/profiling/ddup/CMakeLists.txt | 1 - .../datadog/profiling/setup_custom.sh | 31 ++++++----- .../datadog/profiling/stack_v2/CMakeLists.txt | 10 +++- .../stack_v2/include/stack_renderer.hpp | 16 +++--- .../profiling/stack_v2/src/sampler.cpp | 6 +-- 9 files changed, 87 insertions(+), 47 deletions(-) diff --git a/ddtrace/internal/datadog/profiling/cmake/AnalysisFunc.cmake b/ddtrace/internal/datadog/profiling/cmake/AnalysisFunc.cmake index 9f0bd65ae29..1f65e0fa53c 100644 --- a/ddtrace/internal/datadog/profiling/cmake/AnalysisFunc.cmake +++ b/ddtrace/internal/datadog/profiling/cmake/AnalysisFunc.cmake @@ -1,5 +1,3 @@ -include(FindCppcheck) - function(add_ddup_config target) target_compile_options(${target} PRIVATE "$<$:-Og;-ggdb3>" @@ -24,7 +22,4 @@ function(add_ddup_config target) if (DO_FANALYZER AND CMAKE_CXX_COMPILER_ID MATCHES "GNU") target_compile_options(${target} PRIVATE -fanalyzer) endif() - - # If DO_CPPCHECK is specified, then we can use cppcheck - add_cppcheck_target(cppcheck_dd_${target} ${CMAKE_CURRENT_SOURCE_DIR}) endfunction() diff --git a/ddtrace/internal/datadog/profiling/cmake/FindCppcheck.cmake b/ddtrace/internal/datadog/profiling/cmake/FindCppcheck.cmake index 458d0c824e2..b215e1dfad2 100644 --- 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/dd_wrapper/CMakeLists.txt b/ddtrace/internal/datadog/profiling/dd_wrapper/CMakeLists.txt index 1075ab25104..47d028f849d 100644 --- a/ddtrace/internal/datadog/profiling/dd_wrapper/CMakeLists.txt +++ b/ddtrace/internal/datadog/profiling/dd_wrapper/CMakeLists.txt @@ -15,6 +15,7 @@ include(FetchContent) include(ExternalProject) include(FindLibdatadog) include(AnalysisFunc) +include(FindCppcheck) # Library sources add_library(dd_wrapper SHARED @@ -59,6 +60,14 @@ if (LIB_INSTALL_DIR) ) endif() +# 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 if (BUILD_TESTING) enable_testing() diff --git a/ddtrace/internal/datadog/profiling/dd_wrapper/src/uploader_builder.cpp b/ddtrace/internal/datadog/profiling/dd_wrapper/src/uploader_builder.cpp index 54c69184bad..d69d41fb3cf 100644 --- a/ddtrace/internal/datadog/profiling/dd_wrapper/src/uploader_builder.cpp +++ b/ddtrace/internal/datadog/profiling/dd_wrapper/src/uploader_builder.cpp @@ -114,8 +114,8 @@ UploaderBuilder::build() }; for (const auto& [tag, data] : tag_data) { - std::string errmsg; if (!data.empty()) { + std::string errmsg; if (!add_tag(tags, tag, data, errmsg)) { reasons.push_back(std::string(to_string(tag)) + ": " + errmsg); } diff --git a/ddtrace/internal/datadog/profiling/ddup/CMakeLists.txt b/ddtrace/internal/datadog/profiling/ddup/CMakeLists.txt index a596d6a941e..3a76f673805 100644 --- a/ddtrace/internal/datadog/profiling/ddup/CMakeLists.txt +++ b/ddtrace/internal/datadog/profiling/ddup/CMakeLists.txt @@ -57,7 +57,6 @@ set_target_properties(${EXTENSION_NAME} PROPERTIES SUFFIX "") set_target_properties(${EXTENSION_NAME} PROPERTIES INSTALL_RPATH "$ORIGIN") target_include_directories(${EXTENSION_NAME} PRIVATE - include ../dd_wrapper/include ${Datadog_INCLUDE_DIRS} ${Python3_INCLUDE_DIRS} diff --git a/ddtrace/internal/datadog/profiling/setup_custom.sh b/ddtrace/internal/datadog/profiling/setup_custom.sh index b3f5d4e4c32..0c41aed8517 100755 --- a/ddtrace/internal/datadog/profiling/setup_custom.sh +++ b/ddtrace/internal/datadog/profiling/setup_custom.sh @@ -68,6 +68,9 @@ cmake_args=( -DCMAKE_EXPORT_COMPILE_COMMANDS=ON -DCMAKE_VERBOSE_MAKEFILE=ON -DLIB_INSTALL_DIR=$(realpath $MY_DIR)/lib + -DPython3_INCLUDE_DIRS=$(python3 -c "from distutils.sysconfig import get_python_inc; print(get_python_inc())") + -DPython3_EXECUTABLE=$(which python3) + -DPython3_LIBRARY=$(python3 -c "import distutils.sysconfig as sysconfig; print(sysconfig.get_config_var('LIBDIR'))") ) # Initial build targets; no matter what, dd_wrapper is the base dependency, so it's always built @@ -127,13 +130,13 @@ print_help() { echo "Usage: ${MY_NAME} [options] [build_mode] [target]" echo "Options (one of)" echo " -h, --help Show this help message and exit" - echo " -s, --safety Clang + " compile_args["safety"] - echo " -t, --thread Clang + " compile_args["thread"] - echo " -n, --numerical Clang + " compile_args["numerical"] - echo " -d, --dataflow Clang + " compile_args["dataflow"] - echo " -m --memory Clang + " compile_args["memory"] - echo " -C --cppcheck Clang + " compile_args["cppcheck"] - echo " -f, --fanalyze GCC + " compile_args["fanalyzer"] + echo " -s, --safety Clang + " ${compile_args["safety"]} + echo " -t, --thread Clang + " ${compile_args["thread"]} + echo " -n, --numerical Clang + " ${compile_args["numerical"]} + echo " -d, --dataflow Clang + " ${compile_args["dataflow"]} + echo " -m --memory Clang + " ${compile_args["memory"]} + echo " -C --cppcheck Clang + " ${compile_args["cppcheck"]} + echo " -f, --fanalyze GCC + " ${compile_args["fanalyzer"]} echo " -c, --clang Clang (alone)" echo " -g, --gcc GCC (alone)" echo " -- Don't do anything special" @@ -171,31 +174,31 @@ add_compiler_args() { exit 0 ;; -s|--safety) - cmake_args+=compiler_args["safety"] + cmake_args+=(${compiler_args["safety"]}) set_clang ;; -t|--thread) - cmake_args+=compiler_args["thread"] + cmake_args+=(${compiler_args["thread"]}) set_clang ;; -n|--numerical) - cmake_args+=compiler_args["numerical"] + cmake_args+=(${compiler_args["numerical"]}) set_clang ;; -d|--dataflow) - cmake_args+=compiler_args["dataflow"] + cmake_args+=(${compiler_args["dataflow"]}) set_clang ;; -m|--memory) - cmake_args+=compiler_args["memory"] + cmake_args+=(${compiler_args["memory"]}) set_clang ;; -C|--cppcheck) - cmake_args+=compiler_args["cppcheck"] + cmake_args+=(${compiler_args["cppcheck"]}) set_clang ;; -f|--fanalyze) - cmake_args+=compiler_args["fanalyzer"] + cmake_args+=(${compiler_args["fanalyzer"]}) set_gcc ;; -c|--clang) diff --git a/ddtrace/internal/datadog/profiling/stack_v2/CMakeLists.txt b/ddtrace/internal/datadog/profiling/stack_v2/CMakeLists.txt index 4d169d3e96f..09740a890ea 100644 --- a/ddtrace/internal/datadog/profiling/stack_v2/CMakeLists.txt +++ b/ddtrace/internal/datadog/profiling/stack_v2/CMakeLists.txt @@ -6,13 +6,14 @@ set(EXTENSION_NAME "stack_v2" CACHE STRING "Name of the extension") project(${EXTENSION_NAME}) # Custom cmake modules are in the parent directory -list(APPEND CMAKE_MODULE_PATH "${CMAKE_CURRENT_LIST_DIR}/../cmake") +list(APPEND CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/../cmake") # Includes include(FetchContent) include(ExternalProject) include(FindPython3) include(AnalysisFunc) +include(FindCppcheck) add_subdirectory(../dd_wrapper "${LIB_INSTALL_DIR}") # Find the Python interpreter @@ -100,3 +101,10 @@ if (LIB_INSTALL_DIR) RUNTIME DESTINATION ${LIB_INSTALL_DIR} ) endif() + +# Configure cppcheck +add_cppcheck_target(${EXTENSION_NAME} + DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR} + INCLUDE ${CMAKE_CURRENT_SOURCE_DIR}/include + SRC ${CMAKE_CURRENT_SOURCE_DIR}/src +) diff --git a/ddtrace/internal/datadog/profiling/stack_v2/include/stack_renderer.hpp b/ddtrace/internal/datadog/profiling/stack_v2/include/stack_renderer.hpp index 63be0b95d2f..2faad1beb7c 100644 --- a/ddtrace/internal/datadog/profiling/stack_v2/include/stack_renderer.hpp +++ b/ddtrace/internal/datadog/profiling/stack_v2/include/stack_renderer.hpp @@ -20,19 +20,19 @@ class StackRenderer : public RendererInterface { Sample* sample = nullptr; - void render_message(std::string_view msg) override; - void render_thread_begin(PyThreadState* tstate, + virtual void render_message(std::string_view msg) override; + virtual void render_thread_begin(PyThreadState* tstate, std::string_view name, microsecond_t wall_time_us, uintptr_t thread_id, unsigned long native_id) override; - void render_stack_begin() override; - void render_python_frame(std::string_view name, std::string_view file, uint64_t line) override; - void render_native_frame(std::string_view name, std::string_view file, uint64_t line) override; - void render_cpu_time(microsecond_t cpu_time_us) override; - void render_stack_end() override; - bool is_valid() override; + virtual void render_stack_begin() override; + virtual void render_python_frame(std::string_view name, std::string_view file, uint64_t line) override; + virtual void render_native_frame(std::string_view name, std::string_view file, uint64_t line) override; + virtual void render_cpu_time(microsecond_t cpu_time_us) override; + virtual void render_stack_end() override; + virtual bool is_valid() override; }; } // namespace Datadog diff --git a/ddtrace/internal/datadog/profiling/stack_v2/src/sampler.cpp b/ddtrace/internal/datadog/profiling/stack_v2/src/sampler.cpp index 0e9e4a99e05..11364eef42b 100644 --- a/ddtrace/internal/datadog/profiling/stack_v2/src/sampler.cpp +++ b/ddtrace/internal/datadog/profiling/stack_v2/src/sampler.cpp @@ -44,10 +44,8 @@ Sampler::set_interval(double new_interval_s) sample_interval_us.store(new_interval_us); } -Sampler::Sampler() -{ - renderer_ptr = std::make_shared(); -} +Sampler::Sampler() : renderer_ptr{std::make_shared()} +{ } Sampler& Sampler::get() From acf0459a66d68b7007b1cb27c786c1b76bbd2584 Mon Sep 17 00:00:00 2001 From: sanchda Date: Wed, 28 Feb 2024 14:22:19 +0000 Subject: [PATCH 37/76] Profiling interface fixup --- .../datadog/profiling/stack_v2/include/stack_renderer.hpp | 8 ++++---- .../internal/datadog/profiling/stack_v2/src/sampler.cpp | 5 +++-- ddtrace/profiling/collector/stack.pyx | 2 +- 3 files changed, 8 insertions(+), 7 deletions(-) diff --git a/ddtrace/internal/datadog/profiling/stack_v2/include/stack_renderer.hpp b/ddtrace/internal/datadog/profiling/stack_v2/include/stack_renderer.hpp index 2faad1beb7c..57139f5e481 100644 --- a/ddtrace/internal/datadog/profiling/stack_v2/include/stack_renderer.hpp +++ b/ddtrace/internal/datadog/profiling/stack_v2/include/stack_renderer.hpp @@ -22,10 +22,10 @@ class StackRenderer : public RendererInterface virtual void render_message(std::string_view msg) override; virtual void render_thread_begin(PyThreadState* tstate, - std::string_view name, - microsecond_t wall_time_us, - uintptr_t thread_id, - unsigned long native_id) override; + std::string_view name, + microsecond_t wall_time_us, + uintptr_t thread_id, + unsigned long native_id) override; virtual void render_stack_begin() override; virtual void render_python_frame(std::string_view name, std::string_view file, uint64_t line) override; diff --git a/ddtrace/internal/datadog/profiling/stack_v2/src/sampler.cpp b/ddtrace/internal/datadog/profiling/stack_v2/src/sampler.cpp index 11364eef42b..f32eeeb648e 100644 --- a/ddtrace/internal/datadog/profiling/stack_v2/src/sampler.cpp +++ b/ddtrace/internal/datadog/profiling/stack_v2/src/sampler.cpp @@ -44,8 +44,9 @@ Sampler::set_interval(double new_interval_s) sample_interval_us.store(new_interval_us); } -Sampler::Sampler() : renderer_ptr{std::make_shared()} -{ } +Sampler::Sampler() + : renderer_ptr{ std::make_shared() } +{} Sampler& Sampler::get() diff --git a/ddtrace/profiling/collector/stack.pyx b/ddtrace/profiling/collector/stack.pyx index a9670f00735..1fded2fa83a 100644 --- a/ddtrace/profiling/collector/stack.pyx +++ b/ddtrace/profiling/collector/stack.pyx @@ -539,7 +539,7 @@ class StackCollector(collector.PeriodicCollector): used_wall_time_ns = compat.monotonic_ns() - now self.interval = self._compute_new_interval(used_wall_time_ns) - if self.stack_collector_v2_enabled: + if self._stack_collector_v2_enabled: stack_v2.set_interval(self.interval) return all_events From e2e5237b4f69fc9415c4a44bdcc17b8855b78cf3 Mon Sep 17 00:00:00 2001 From: sanchda Date: Wed, 28 Feb 2024 15:02:42 +0000 Subject: [PATCH 38/76] Make engaging stack_v2 safer --- ddtrace/profiling/collector/stack.pyx | 9 ++++++++- ddtrace/profiling/collector/stack_v2.py | 19 +++++++++++++++++++ 2 files changed, 27 insertions(+), 1 deletion(-) create mode 100644 ddtrace/profiling/collector/stack_v2.py diff --git a/ddtrace/profiling/collector/stack.pyx b/ddtrace/profiling/collector/stack.pyx index 1fded2fa83a..6ca87a95974 100644 --- a/ddtrace/profiling/collector/stack.pyx +++ b/ddtrace/profiling/collector/stack.pyx @@ -13,13 +13,13 @@ from ddtrace import context from ddtrace import span as ddspan from ddtrace.internal import compat from ddtrace.internal.datadog.profiling import ddup -from ddtrace.internal.datadog.profiling import stack_v2 from ddtrace.internal.utils import formats from ddtrace.profiling import _threading from ddtrace.profiling import collector from ddtrace.profiling.collector import _task from ddtrace.profiling.collector import _traceback from ddtrace.profiling.collector import stack_event +from ddtrace.profiling.collector import stack_v2 from ddtrace.settings.profiling import config @@ -485,6 +485,13 @@ class StackCollector(collector.PeriodicCollector): self._thread_span_links = _ThreadSpanLinks() self.tracer.context_provider._on_activate(self._thread_span_links.link_span) + # If stack_v2 is enabled, verify that it loaded properly. If not, fallback to the v1 stack collector + # and log a warning. + if self._stack_collector_v2_enabled: + if not stack_v2.is_available(): + self._stack_collector_v2_enabled = False + LOG.warning("Failed to load the v2 stack collector; falling back to the v1 stack collector") + # Force-set use_libdd if the v2 stack collector is enabled if self._stack_collector_v2_enabled: set_use_libdd(True) diff --git a/ddtrace/profiling/collector/stack_v2.py b/ddtrace/profiling/collector/stack_v2.py new file mode 100644 index 00000000000..4c98b59a7e9 --- /dev/null +++ b/ddtrace/profiling/collector/stack_v2.py @@ -0,0 +1,19 @@ +try: + from ddtrace.internal.datadog.profiling.stack_v2 import * # noqa: F401, F403 + + def is_available(): + return True + +except Exception: + + def is_available(): + return False + + def start(*args, **kwargs): + pass + + def stop(*args, **kwargs): + pass + + def set_interval(*args, **kwargs): + pass From bd2bb65e088f2c5eeb6410f8f25fafd996456e62 Mon Sep 17 00:00:00 2001 From: sanchda Date: Wed, 28 Feb 2024 17:09:35 +0000 Subject: [PATCH 39/76] Fix test, oops --- tests/profiling/collector/test_stack.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/profiling/collector/test_stack.py b/tests/profiling/collector/test_stack.py index 681c34d95d3..497fc9bd3e0 100644 --- a/tests/profiling/collector/test_stack.py +++ b/tests/profiling/collector/test_stack.py @@ -322,7 +322,7 @@ def test_repr(): stack.StackCollector, "StackCollector(status=, " "recorder=Recorder(default_max_events=16384, max_events={}), min_interval_time=0.01, max_time_usage_pct=1.0, " - "nframes=64, ignore_profiler=False, endpoint_collection_enabled=None, tracer=None)", + "nframes=64, ignore_profiler=False, endpoint_collection_enabled=None, tracer=None, _stack_collector_v2_enabled=False)", ) From ed57403c19f41bd7147bf849456ff97f5041f5fc Mon Sep 17 00:00:00 2001 From: sanchda Date: Wed, 28 Feb 2024 17:35:57 +0000 Subject: [PATCH 40/76] Style --- tests/profiling/collector/test_stack.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/profiling/collector/test_stack.py b/tests/profiling/collector/test_stack.py index 497fc9bd3e0..4f10d4bbb82 100644 --- a/tests/profiling/collector/test_stack.py +++ b/tests/profiling/collector/test_stack.py @@ -322,7 +322,8 @@ def test_repr(): stack.StackCollector, "StackCollector(status=, " "recorder=Recorder(default_max_events=16384, max_events={}), min_interval_time=0.01, max_time_usage_pct=1.0, " - "nframes=64, ignore_profiler=False, endpoint_collection_enabled=None, tracer=None, _stack_collector_v2_enabled=False)", + "nframes=64, ignore_profiler=False, endpoint_collection_enabled=None, tracer=None," + "_stack_collector_v2_enabled=False)", ) From 2cecac50fc45edb622950538c0ed2bc1be81ba6e Mon Sep 17 00:00:00 2001 From: sanchda Date: Wed, 28 Feb 2024 18:05:45 +0000 Subject: [PATCH 41/76] Fix telemetry test --- tests/telemetry/test_writer.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/tests/telemetry/test_writer.py b/tests/telemetry/test_writer.py index 4d4255136b5..c6e9d33b3d5 100644 --- a/tests/telemetry/test_writer.py +++ b/tests/telemetry/test_writer.py @@ -89,7 +89,6 @@ def test_app_started_event(telemetry_writer, test_agent_session, mock_time): {"name": "DD_PROFILING_MEMORY_ENABLED", "origin": "unknown", "value": True}, {"name": "DD_PROFILING_HEAP_ENABLED", "origin": "unknown", "value": True}, {"name": "DD_PROFILING_LOCK_ENABLED", "origin": "unknown", "value": True}, - {"name": "DD_PROFILING_EXPORT_PY_ENABLED", "origin": "unknown", "value": True}, {"name": "DD_PROFILING_EXPORT_LIBDD_ENABLED", "origin": "unknown", "value": False}, {"name": "DD_PROFILING_CAPTURE_PCT", "origin": "unknown", "value": 1.0}, {"name": "DD_PROFILING_UPLOAD_INTERVAL", "origin": "unknown", "value": 60.0}, @@ -203,7 +202,6 @@ def test_app_started_event_configuration_override(test_agent_session, run_python env["DD_PROFILING_MEMORY_ENABLED"] = "False" env["DD_PROFILING_HEAP_ENABLED"] = "False" env["DD_PROFILING_LOCK_ENABLED"] = "False" - env["DD_PROFILING_EXPORT_PY_ENABLED"] = "False" env["DD_PROFILING_EXPORT_LIBDD_ENABLED"] = "True" env["DD_PROFILING_CAPTURE_PCT"] = "5.0" env["DD_PROFILING_UPLOAD_INTERVAL"] = "10.0" @@ -248,7 +246,6 @@ def test_app_started_event_configuration_override(test_agent_session, run_python {"name": "DD_PROFILING_MEMORY_ENABLED", "origin": "unknown", "value": False}, {"name": "DD_PROFILING_HEAP_ENABLED", "origin": "unknown", "value": False}, {"name": "DD_PROFILING_LOCK_ENABLED", "origin": "unknown", "value": False}, - {"name": "DD_PROFILING_EXPORT_PY_ENABLED", "origin": "unknown", "value": False}, {"name": "DD_PROFILING_EXPORT_LIBDD_ENABLED", "origin": "unknown", "value": True}, {"name": "DD_PROFILING_CAPTURE_PCT", "origin": "unknown", "value": 5.0}, {"name": "DD_PROFILING_UPLOAD_INTERVAL", "origin": "unknown", "value": 10.0}, From 04f914e5bf807cad2b01e09944a288521fe31cae Mon Sep 17 00:00:00 2001 From: David Sanchez <838104+sanchda@users.noreply.github.com> Date: Wed, 28 Feb 2024 20:35:23 -0600 Subject: [PATCH 42/76] Update ddtrace/profiling/collector/memalloc.py Co-authored-by: Gabriele N. Tornetta --- ddtrace/profiling/collector/memalloc.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ddtrace/profiling/collector/memalloc.py b/ddtrace/profiling/collector/memalloc.py index 5e81b58a126..06dd16463c0 100644 --- a/ddtrace/profiling/collector/memalloc.py +++ b/ddtrace/profiling/collector/memalloc.py @@ -179,7 +179,7 @@ def collect(self): # DEV: This might happen if the memalloc sofile is unlinked and relinked without module # re-initialization. LOG.debug("Invalid state detected in memalloc module, suppressing profile") - return [] + return tuple() else: return ( tuple( From f1114d96e166bce2ea49fb3644081147343cafea Mon Sep 17 00:00:00 2001 From: David Sanchez <838104+sanchda@users.noreply.github.com> Date: Wed, 28 Feb 2024 20:35:37 -0600 Subject: [PATCH 43/76] Update ddtrace/internal/datadog/profiling/stack_v2/include/util/cast_to_pyfunc.hpp Co-authored-by: Gabriele N. Tornetta --- .../datadog/profiling/stack_v2/include/util/cast_to_pyfunc.hpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ddtrace/internal/datadog/profiling/stack_v2/include/util/cast_to_pyfunc.hpp b/ddtrace/internal/datadog/profiling/stack_v2/include/util/cast_to_pyfunc.hpp index a370b50a0b2..1ba9ef131a1 100644 --- a/ddtrace/internal/datadog/profiling/stack_v2/include/util/cast_to_pyfunc.hpp +++ b/ddtrace/internal/datadog/profiling/stack_v2/include/util/cast_to_pyfunc.hpp @@ -3,7 +3,7 @@ // Abiding by the contract required in the Python interface requires abandoning certain checks in one's main project. // Rather than abandoning discipline, we can include this header in a way that bypasses these checks. -inline PyCFunction +inline static PyCFunction cast_to_pycfunction(PyObject* (*func)(PyObject*, PyObject*, PyObject*)) { // Direct C-style cast From 86d02cb7ef44e48de70d5cef9ffb397d6f639a6c Mon Sep 17 00:00:00 2001 From: David Sanchez <838104+sanchda@users.noreply.github.com> Date: Wed, 28 Feb 2024 20:35:56 -0600 Subject: [PATCH 44/76] Update ddtrace/settings/profiling.py Co-authored-by: Gabriele N. Tornetta --- ddtrace/settings/profiling.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ddtrace/settings/profiling.py b/ddtrace/settings/profiling.py index 9a6f66f3c49..b51a46b0b34 100644 --- a/ddtrace/settings/profiling.py +++ b/ddtrace/settings/profiling.py @@ -45,7 +45,7 @@ def _is_valid_libdatadog(): def _is_valid_v2_stack(): # type: () -> bool - return platform.python_version_tuple() >= ("3", "7") and _is_valid_libdatadog() + return sys.version_info >= (3, 7) and _is_valid_libdatadog() class ProfilingConfig(En): From a9c293594f989d7ab84e4e3f5b211ef719e91cb7 Mon Sep 17 00:00:00 2001 From: sanchda Date: Thu, 29 Feb 2024 13:38:18 +0000 Subject: [PATCH 45/76] Add import --- ddtrace/settings/profiling.py | 1 + 1 file changed, 1 insertion(+) diff --git a/ddtrace/settings/profiling.py b/ddtrace/settings/profiling.py index b51a46b0b34..1f8dcaaa04c 100644 --- a/ddtrace/settings/profiling.py +++ b/ddtrace/settings/profiling.py @@ -1,5 +1,6 @@ import math import platform +import sys import typing as t from envier import En From 86d46b845128606b80bacce97398933ec5f0c308 Mon Sep 17 00:00:00 2001 From: sanchda Date: Thu, 29 Feb 2024 15:17:55 +0000 Subject: [PATCH 46/76] Off-by-one lol --- tests/profiling/collector/test_stack.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/profiling/collector/test_stack.py b/tests/profiling/collector/test_stack.py index 4f10d4bbb82..2e6e577b912 100644 --- a/tests/profiling/collector/test_stack.py +++ b/tests/profiling/collector/test_stack.py @@ -322,7 +322,7 @@ def test_repr(): stack.StackCollector, "StackCollector(status=, " "recorder=Recorder(default_max_events=16384, max_events={}), min_interval_time=0.01, max_time_usage_pct=1.0, " - "nframes=64, ignore_profiler=False, endpoint_collection_enabled=None, tracer=None," + "nframes=64, ignore_profiler=False, endpoint_collection_enabled=None, tracer=None, " "_stack_collector_v2_enabled=False)", ) From 99d5c8ee9b12c3521f07ef0affed98364406848b Mon Sep 17 00:00:00 2001 From: sanchda Date: Thu, 29 Feb 2024 18:15:49 +0000 Subject: [PATCH 47/76] Try to fix the cmake config --- .../datadog/profiling/cmake/AnalysisFunc.cmake | 6 ++++++ .../datadog/profiling/dd_wrapper/CMakeLists.txt | 9 +++------ .../datadog/profiling/ddup/CMakeLists.txt | 16 ++++++---------- .../datadog/profiling/stack_v2/CMakeLists.txt | 9 ++------- 4 files changed, 17 insertions(+), 23 deletions(-) diff --git a/ddtrace/internal/datadog/profiling/cmake/AnalysisFunc.cmake b/ddtrace/internal/datadog/profiling/cmake/AnalysisFunc.cmake index 1f65e0fa53c..da2584a08d1 100644 --- a/ddtrace/internal/datadog/profiling/cmake/AnalysisFunc.cmake +++ b/ddtrace/internal/datadog/profiling/cmake/AnalysisFunc.cmake @@ -1,3 +1,5 @@ +include(CheckIPOSupported) + function(add_ddup_config target) target_compile_options(${target} PRIVATE "$<$:-Og;-ggdb3>" @@ -9,6 +11,10 @@ function(add_ddup_config target) -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) diff --git a/ddtrace/internal/datadog/profiling/dd_wrapper/CMakeLists.txt b/ddtrace/internal/datadog/profiling/dd_wrapper/CMakeLists.txt index 47d028f849d..07f04c52cf2 100644 --- a/ddtrace/internal/datadog/profiling/dd_wrapper/CMakeLists.txt +++ b/ddtrace/internal/datadog/profiling/dd_wrapper/CMakeLists.txt @@ -48,15 +48,12 @@ 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 LIB_INSTALL_DIR is set, install the library if (LIB_INSTALL_DIR) install(TARGETS dd_wrapper - DESTINATION "${LIB_INSTALL_DIR}" + LIBRARY DESTINATION ${LIB_INSTALL_DIR} + ARCHIVE DESTINATION ${LIB_INSTALL_DIR} + RUNTIME DESTINATION ${LIB_INSTALL_DIR} ) endif() diff --git a/ddtrace/internal/datadog/profiling/ddup/CMakeLists.txt b/ddtrace/internal/datadog/profiling/ddup/CMakeLists.txt index 3a76f673805..3d418bf947b 100644 --- a/ddtrace/internal/datadog/profiling/ddup/CMakeLists.txt +++ b/ddtrace/internal/datadog/profiling/ddup/CMakeLists.txt @@ -15,11 +15,10 @@ list(APPEND CMAKE_MODULE_PATH "${CMAKE_CURRENT_LIST_DIR}/../cmake") # Includes include(FetchContent) include(ExternalProject) -include(FindPython3) include(FindLibdatadog) include(AnalysisFunc) -add_subdirectory(../dd_wrapper "${LIB_INSTALL_DIR}") +add_subdirectory(../dd_wrapper) # Find the Python interpreter find_package(Python3 COMPONENTS Interpreter REQUIRED) @@ -33,15 +32,16 @@ set (ENV{PY_MINOR_VERSION} ${PY_MINOR_VERSION}) set (ENV{PY_MICRO_VERSION} ${PY_MICRO_VERSION}) # Cythonize the .pyx file +set(DDUP_CPP_SRC ${CMAKE_CURRENT_BINARY_DIR}/_ddup.cpp) add_custom_command( - OUTPUT ${CMAKE_CURRENT_LIST_DIR}/_ddup.cpp - COMMAND ${PYTHON_EXECUTABLE} -m cython ${CMAKE_CURRENT_LIST_DIR}/_ddup.pyx -o ${CMAKE_CURRENT_LIST_DIR}/_ddup.cpp + OUTPUT ${DDUP_CPP_SRC} + COMMAND ${PYTHON_EXECUTABLE} -m cython ${CMAKE_CURRENT_LIST_DIR}/_ddup.pyx -o ${DDUP_CPP_SRC} DEPENDS ${CMAKE_CURRENT_LIST_DIR}/_ddup.pyx ) # Specify the target C-extension that we want to build add_library(${EXTENSION_NAME} SHARED - _ddup.cpp + ${DDUP_CPP_SRC} ) target_compile_features(${EXTENSION_NAME} PUBLIC cxx_std_17) @@ -69,13 +69,9 @@ target_link_libraries(${EXTENSION_NAME} PRIVATE # Extensions are built as dynamic libraries, so PIC is required. set_target_properties(${EXTENSION_NAME} PROPERTIES POSITION_INDEPENDENT_CODE ON) -set_target_properties(${EXTENSION_NAME} PROPERTIES - LIBRARY_OUTPUT_DIRECTORY "${CMAKE_CURRENT_LIST_DIR}" -) - # Set the output directory for the built library if (LIB_INSTALL_DIR) - install(TARGETS ${EXTENSION_NAME} DESTINATION + install(TARGETS ${EXTENSION_NAME} LIBRARY DESTINATION ${LIB_INSTALL_DIR} ARCHIVE DESTINATION ${LIB_INSTALL_DIR} RUNTIME DESTINATION ${LIB_INSTALL_DIR} diff --git a/ddtrace/internal/datadog/profiling/stack_v2/CMakeLists.txt b/ddtrace/internal/datadog/profiling/stack_v2/CMakeLists.txt index 09740a890ea..5bc7a3d1311 100644 --- a/ddtrace/internal/datadog/profiling/stack_v2/CMakeLists.txt +++ b/ddtrace/internal/datadog/profiling/stack_v2/CMakeLists.txt @@ -11,10 +11,9 @@ list(APPEND CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/../cmake") # Includes include(FetchContent) include(ExternalProject) -include(FindPython3) include(AnalysisFunc) include(FindCppcheck) -add_subdirectory(../dd_wrapper "${LIB_INSTALL_DIR}") +add_subdirectory(../dd_wrapper) # Find the Python interpreter find_package(Python3 COMPONENTS Interpreter REQUIRED) @@ -89,13 +88,9 @@ target_link_libraries(${EXTENSION_NAME} PRIVATE # Extensions are built as dynamic libraries, so PIC is required. set_target_properties(${EXTENSION_NAME} PROPERTIES POSITION_INDEPENDENT_CODE ON) -set_target_properties(${EXTENSION_NAME} PROPERTIES - LIBRARY_OUTPUT_DIRECTORY "${CMAKE_CURRENT_LIST_DIR}" -) - # Set the output directory for the built library if (LIB_INSTALL_DIR) - install(TARGETS ${EXTENSION_NAME} DESTINATION + install(TARGETS ${EXTENSION_NAME} LIBRARY DESTINATION ${LIB_INSTALL_DIR} ARCHIVE DESTINATION ${LIB_INSTALL_DIR} RUNTIME DESTINATION ${LIB_INSTALL_DIR} From 31a28e94e90697d359e1190bea0ef999c43fb813 Mon Sep 17 00:00:00 2001 From: sanchda Date: Thu, 29 Feb 2024 18:23:23 +0000 Subject: [PATCH 48/76] Hack: add binary dir to add_sub This is a slight hack in order to keep me from having to write boilerplate around dd_wrapper, but maybe I have to just do that. --- ddtrace/internal/datadog/profiling/ddup/CMakeLists.txt | 2 +- ddtrace/internal/datadog/profiling/stack_v2/CMakeLists.txt | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/ddtrace/internal/datadog/profiling/ddup/CMakeLists.txt b/ddtrace/internal/datadog/profiling/ddup/CMakeLists.txt index 3d418bf947b..3586be308e7 100644 --- a/ddtrace/internal/datadog/profiling/ddup/CMakeLists.txt +++ b/ddtrace/internal/datadog/profiling/ddup/CMakeLists.txt @@ -18,7 +18,7 @@ include(ExternalProject) include(FindLibdatadog) include(AnalysisFunc) -add_subdirectory(../dd_wrapper) +add_subdirectory(../dd_wrapper ${CMAKE_CURRENT_BINARY_DIR}/dd_wrapper_build) # Find the Python interpreter find_package(Python3 COMPONENTS Interpreter REQUIRED) diff --git a/ddtrace/internal/datadog/profiling/stack_v2/CMakeLists.txt b/ddtrace/internal/datadog/profiling/stack_v2/CMakeLists.txt index 5bc7a3d1311..cbe4f4962cf 100644 --- a/ddtrace/internal/datadog/profiling/stack_v2/CMakeLists.txt +++ b/ddtrace/internal/datadog/profiling/stack_v2/CMakeLists.txt @@ -13,7 +13,7 @@ include(FetchContent) include(ExternalProject) include(AnalysisFunc) include(FindCppcheck) -add_subdirectory(../dd_wrapper) +add_subdirectory(../dd_wrapper ${CMAKE_CURRENT_BINARY_DIR}/dd_wrapper_build) # Find the Python interpreter find_package(Python3 COMPONENTS Interpreter REQUIRED) From 1d16297f1851229fa48b41ae7594530d171f1f26 Mon Sep 17 00:00:00 2001 From: sanchda Date: Thu, 29 Feb 2024 21:07:01 +0000 Subject: [PATCH 49/76] Fixups for cmake, tests --- .../datadog/profiling/ddup/CMakeLists.txt | 18 ++++++++++++++---- .../datadog/profiling/stack_v2/CMakeLists.txt | 13 +++++-------- setup.py | 4 +--- 3 files changed, 20 insertions(+), 15 deletions(-) diff --git a/ddtrace/internal/datadog/profiling/ddup/CMakeLists.txt b/ddtrace/internal/datadog/profiling/ddup/CMakeLists.txt index 3586be308e7..14d5dc64892 100644 --- a/ddtrace/internal/datadog/profiling/ddup/CMakeLists.txt +++ b/ddtrace/internal/datadog/profiling/ddup/CMakeLists.txt @@ -16,12 +16,10 @@ list(APPEND CMAKE_MODULE_PATH "${CMAKE_CURRENT_LIST_DIR}/../cmake") include(FetchContent) include(ExternalProject) include(FindLibdatadog) -include(AnalysisFunc) add_subdirectory(../dd_wrapper ${CMAKE_CURRENT_BINARY_DIR}/dd_wrapper_build) -# Find the Python interpreter -find_package(Python3 COMPONENTS Interpreter REQUIRED) +# Make sure we have certain variables if (NOT Python3_INCLUDE_DIRS) message(FATAL_ERROR "Python3_INCLUDE_DIRS not found") endif() @@ -44,6 +42,19 @@ add_library(${EXTENSION_NAME} SHARED ${DDUP_CPP_SRC} ) +# We can't add common Profiling configuration because cython generates messy code, so we just setup some +# basic flags and features +target_compile_options(${EXTENSION_NAME} PRIVATE + "$<$:-Og;-ggdb3>" + "$<$:-Os>" + -ffunction-sections -fno-semantic-interposition +) +target_link_options(${EXTENSION_NAME} PRIVATE + "$<$:-s>" + -Wl,--as-needed -Wl,-Bsymbolic-functions -Wl,--gc-sections +) +set_property(TARGET ${EXTENSION_NAME} PROPERTY INTERPROCEDURAL_OPTIMIZATION TRUE) + target_compile_features(${EXTENSION_NAME} PUBLIC cxx_std_17) # cmake may mutate the name of the library (e.g., lib- and -.so for dynamic libraries). @@ -55,7 +66,6 @@ set_target_properties(${EXTENSION_NAME} PROPERTIES SUFFIX "") # RPATH is needed for sofile discovery at runtime, since Python packages are not # installed in the system path. This is typical. set_target_properties(${EXTENSION_NAME} PROPERTIES INSTALL_RPATH "$ORIGIN") - target_include_directories(${EXTENSION_NAME} PRIVATE ../dd_wrapper/include ${Datadog_INCLUDE_DIRS} diff --git a/ddtrace/internal/datadog/profiling/stack_v2/CMakeLists.txt b/ddtrace/internal/datadog/profiling/stack_v2/CMakeLists.txt index cbe4f4962cf..2cedc6fba12 100644 --- a/ddtrace/internal/datadog/profiling/stack_v2/CMakeLists.txt +++ b/ddtrace/internal/datadog/profiling/stack_v2/CMakeLists.txt @@ -41,6 +41,11 @@ add_library(${EXTENSION_NAME} SHARED # Add common config add_ddup_config(${EXTENSION_NAME}) +add_cppcheck_target(${EXTENSION_NAME} + DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR} + INCLUDE ${CMAKE_CURRENT_SOURCE_DIR}/include + SRC ${CMAKE_CURRENT_SOURCE_DIR}/src +) # This project is build with C++17, even though the underlying repo adheres to the manylinux 2014 standard. # This isn't currently a problem, but if it becomes one, we may have to structure the library differently. @@ -80,7 +85,6 @@ set_target_properties(${EXTENSION_NAME} PROPERTIES SUFFIX "") # RPATH is needed for sofile discovery at runtime, since Python packages are not # installed in the system path. This is typical. set_target_properties(${EXTENSION_NAME} PROPERTIES INSTALL_RPATH "$ORIGIN") - target_link_libraries(${EXTENSION_NAME} PRIVATE dd_wrapper ) @@ -96,10 +100,3 @@ if (LIB_INSTALL_DIR) RUNTIME DESTINATION ${LIB_INSTALL_DIR} ) endif() - -# Configure cppcheck -add_cppcheck_target(${EXTENSION_NAME} - DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR} - INCLUDE ${CMAKE_CURRENT_SOURCE_DIR}/include - SRC ${CMAKE_CURRENT_SOURCE_DIR}/src -) diff --git a/setup.py b/setup.py index 704ee053a19..82f3269f2a8 100644 --- a/setup.py +++ b/setup.py @@ -303,7 +303,6 @@ def build_extension_cmake(self, ext): "-DPython3_LIBRARIES={}".format(python_lib), "-DPYTHON_EXECUTABLE={}".format(sys.executable), "-DCMAKE_BUILD_TYPE={}".format(ext.build_type), - "-DCMAKE_LIBRARY_OUTPUT_DIRECTORY={}".format(output_dir), "-DLIB_INSTALL_DIR={}".format(output_dir), "-DEXTENSION_NAME={}".format(extension_basename), ] @@ -315,7 +314,7 @@ def build_extension_cmake(self, ext): # CMAKE_BUILD_PARALLEL_LEVEL works across all generators # self.parallel is a Python 3 only way to set parallel jobs by hand # using -j in the build_ext call, not supported by pip or PyPA-build. - # DEV: -j is only supported in CMake 3.12+ only. + # DEV: -j is supported in CMake 3.12+ only. if hasattr(self, "parallel") and self.parallel: build_args += ["-j{}".format(self.parallel)] @@ -475,7 +474,6 @@ def get_exts_for(name): CMakeExtension( "ddtrace.internal.datadog.profiling.stack_v2", source_dir=STACK_V2_DIR, - build_args=["--verbose"], ) ) From 44508687ffc5a07f24663145a763cc58be22ed72 Mon Sep 17 00:00:00 2001 From: sanchda Date: Fri, 1 Mar 2024 04:01:46 +0000 Subject: [PATCH 50/76] Some fixups --- .../datadog/profiling/dd_wrapper/CMakeLists.txt | 8 ++++---- .../internal/datadog/profiling/ddup/CMakeLists.txt | 12 +++++++----- .../datadog/profiling/stack_v2/CMakeLists.txt | 14 +++++++++----- setup.py | 10 +++++++--- 4 files changed, 27 insertions(+), 17 deletions(-) diff --git a/ddtrace/internal/datadog/profiling/dd_wrapper/CMakeLists.txt b/ddtrace/internal/datadog/profiling/dd_wrapper/CMakeLists.txt index 07f04c52cf2..1adb13c81e5 100644 --- a/ddtrace/internal/datadog/profiling/dd_wrapper/CMakeLists.txt +++ b/ddtrace/internal/datadog/profiling/dd_wrapper/CMakeLists.txt @@ -49,11 +49,11 @@ target_link_libraries(dd_wrapper PRIVATE set_target_properties(dd_wrapper PROPERTIES POSITION_INDEPENDENT_CODE ON) # If LIB_INSTALL_DIR is set, install the library -if (LIB_INSTALL_DIR) +if (PROFILING_ROOT) install(TARGETS dd_wrapper - LIBRARY DESTINATION ${LIB_INSTALL_DIR} - ARCHIVE DESTINATION ${LIB_INSTALL_DIR} - RUNTIME DESTINATION ${LIB_INSTALL_DIR} + LIBRARY DESTINATION ${PROFILING_ROOT} + ARCHIVE DESTINATION ${OFILING_ROOT} + RUNTIME DESTINATION ${OFILING_ROOT} ) endif() diff --git a/ddtrace/internal/datadog/profiling/ddup/CMakeLists.txt b/ddtrace/internal/datadog/profiling/ddup/CMakeLists.txt index 14d5dc64892..b3b578b1d62 100644 --- a/ddtrace/internal/datadog/profiling/ddup/CMakeLists.txt +++ b/ddtrace/internal/datadog/profiling/ddup/CMakeLists.txt @@ -17,17 +17,19 @@ include(FetchContent) include(ExternalProject) include(FindLibdatadog) -add_subdirectory(../dd_wrapper ${CMAKE_CURRENT_BINARY_DIR}/dd_wrapper_build) +# Technically, this should be its own project which we `include()`, but I don't +# want to deal with that when so many things may yet be factored differently. +add_subdirectory(../dd_wrapper ${CMAKE_CURRENT_BINARY_DIR}/../dd_wrapper_build) -# Make sure we have certain variables +# Make sure we have necessary Python variables if (NOT Python3_INCLUDE_DIRS) message(FATAL_ERROR "Python3_INCLUDE_DIRS not found") endif() # This sets some parameters for the target build, which can only be defined by setup.py -set (ENV{PY_MAJOR_VERSION} ${PY_MAJOR_VERSION}) -set (ENV{PY_MINOR_VERSION} ${PY_MINOR_VERSION}) -set (ENV{PY_MICRO_VERSION} ${PY_MICRO_VERSION}) +set(ENV{PY_MAJOR_VERSION} ${PY_MAJOR_VERSION}) +set(ENV{PY_MINOR_VERSION} ${PY_MINOR_VERSION}) +set(ENV{PY_MICRO_VERSION} ${PY_MICRO_VERSION}) # Cythonize the .pyx file set(DDUP_CPP_SRC ${CMAKE_CURRENT_BINARY_DIR}/_ddup.cpp) diff --git a/ddtrace/internal/datadog/profiling/stack_v2/CMakeLists.txt b/ddtrace/internal/datadog/profiling/stack_v2/CMakeLists.txt index 2cedc6fba12..8dc76194270 100644 --- a/ddtrace/internal/datadog/profiling/stack_v2/CMakeLists.txt +++ b/ddtrace/internal/datadog/profiling/stack_v2/CMakeLists.txt @@ -1,9 +1,11 @@ +cmake_minimum_required(VERSION 3.19) + # The exact name of this extension determines aspects of the installation and build paths, # which need to be kept in sync with setup.py. Accordingly, take the name passed in by the # caller, defaulting to "stack_v2" if needed. -cmake_minimum_required(VERSION 3.19) set(EXTENSION_NAME "stack_v2" CACHE STRING "Name of the extension") project(${EXTENSION_NAME}) +message(STATUS "Building extension: ${EXTENSION_NAME}") # Custom cmake modules are in the parent directory list(APPEND CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/../cmake") @@ -13,10 +15,12 @@ include(FetchContent) include(ExternalProject) include(AnalysisFunc) include(FindCppcheck) -add_subdirectory(../dd_wrapper ${CMAKE_CURRENT_BINARY_DIR}/dd_wrapper_build) -# Find the Python interpreter -find_package(Python3 COMPONENTS Interpreter REQUIRED) +# dd_wrapper should be its own project at one point, if the current design is kept, +# but whether or not we keep that design is unknown. Hack it for now. +add_subdirectory(../dd_wrapper ${CMAKE_CURRENT_BINARY_DIR}/../dd_wrapper_build) + +# Make sure we have necessary Python variables if (NOT Python3_INCLUDE_DIRS) message(FATAL_ERROR "Python3_INCLUDE_DIRS not found") endif() @@ -57,7 +61,7 @@ target_compile_definitions(${EXTENSION_NAME} PRIVATE UNWIND_NATIVE_DISABLE) # Includes; echion and python are marked "system" to suppress warnings, # but note in MSVC we'll have to #pragma warning(push, 0 then pop for the same effect. target_include_directories(${EXTENSION_NAME} PRIVATE - .. + .. # include dd_wrapper from the root in order to make its paths transparent in the code include ) target_include_directories(${EXTENSION_NAME} SYSTEM PRIVATE diff --git a/setup.py b/setup.py index 82f3269f2a8..a7f0dff9f16 100644 --- a/setup.py +++ b/setup.py @@ -42,8 +42,9 @@ LIBDDWAF_DOWNLOAD_DIR = HERE / "ddtrace" / "appsec" / "_ddwaf" / "libddwaf" IAST_DIR = HERE / "ddtrace" / "appsec" / "_iast" / "_taint_tracking" -DDUP_DIR = HERE / "ddtrace" / "internal" / "datadog" / "profiling" / "ddup" -STACK_V2_DIR = HERE / "ddtrace" / "internal" / "datadog" / "profiling" / "stack_v2" +PROF_NATIVE_DIR = HERE / "ddtrace" / "internal" / "datadog" / "profiling" +DDUP_DIR = PROF_NATIVE_DIR / "ddup" +STACK_V2_DIR = PROF_NATIVE_DIR / "stack_v2" CURRENT_OS = platform.system() @@ -464,6 +465,7 @@ def get_exts_for(name): "-DPY_MAJOR_VERSION={}".format(sys.version_info.major), "-DPY_MINOR_VERSION={}".format(sys.version_info.minor), "-DPY_MICRO_VERSION={}".format(sys.version_info.micro), + "-DPROFILING_ROOT={}".format(PROF_NATIVE_DIR), ], ) ) @@ -474,6 +476,9 @@ def get_exts_for(name): CMakeExtension( "ddtrace.internal.datadog.profiling.stack_v2", source_dir=STACK_V2_DIR, + cmake_args=[ + "-DPROFILING_ROOT={}".format(PROF_NATIVE_DIR), + ], ) ) @@ -513,7 +518,6 @@ def get_exts_for(name): "ddtrace.appsec": ["rules.json"], "ddtrace.appsec._ddwaf": [str(Path("libddwaf") / "*" / "lib" / "libddwaf.*")], "ddtrace.appsec._iast._taint_tracking": ["CMakeLists.txt"], - "ddtrace.internal.datadog.profiling": ["libdd_wrapper.*"], }, python_requires=">=3.7", zip_safe=False, From cef8843f6d12d2e3951c1d8bf4b0d47d24398662 Mon Sep 17 00:00:00 2001 From: sanchda Date: Fri, 1 Mar 2024 04:34:51 +0000 Subject: [PATCH 51/76] Fix silly package_data bug --- setup.py | 1 + 1 file changed, 1 insertion(+) diff --git a/setup.py b/setup.py index a7f0dff9f16..50411f448c5 100644 --- a/setup.py +++ b/setup.py @@ -518,6 +518,7 @@ def get_exts_for(name): "ddtrace.appsec": ["rules.json"], "ddtrace.appsec._ddwaf": [str(Path("libddwaf") / "*" / "lib" / "libddwaf.*")], "ddtrace.appsec._iast._taint_tracking": ["CMakeLists.txt"], + "ddtrace.internal.datadog.profiling": ["libdd_wrapper.*"], }, python_requires=">=3.7", zip_safe=False, From e9bcce61f397865beba005cb491351103890bdcc Mon Sep 17 00:00:00 2001 From: sanchda Date: Fri, 1 Mar 2024 06:24:55 +0000 Subject: [PATCH 52/76] Add module, comment --- ddtrace/internal/datadog/profiling/__init__.py | 0 .../datadog/profiling/stack_v2/src/stack_renderer.cpp | 4 ++++ 2 files changed, 4 insertions(+) create mode 100644 ddtrace/internal/datadog/profiling/__init__.py diff --git a/ddtrace/internal/datadog/profiling/__init__.py b/ddtrace/internal/datadog/profiling/__init__.py new file mode 100644 index 00000000000..e69de29bb2d 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 05177486944..8b8ce850c45 100644 --- a/ddtrace/internal/datadog/profiling/stack_v2/src/stack_renderer.cpp +++ b/ddtrace/internal/datadog/profiling/stack_v2/src/stack_renderer.cpp @@ -43,6 +43,10 @@ StackRenderer::render_python_frame(std::string_view name, std::string_view file, return; } + // Normally, further utf-8 validation would be pointless here, but we may be reading data where the + // string pointer was valid, but the string is actually garbage data at the exact time of the read. + // This is rare, but blowing some cycles on early validation allows the sample to be retained by + // libdatadog, so we can evaluate the actual impact of this scenario in live scenarios. static const std::string_view invalid = ""; if (!utf8_check_is_valid(name.data(), name.size())) { name = invalid; From cc71c35d839984d0e6b74935fcb3c1adb6d17996 Mon Sep 17 00:00:00 2001 From: sanchda Date: Fri, 1 Mar 2024 06:45:58 +0000 Subject: [PATCH 53/76] init all the things --- ddtrace/internal/datadog/profiling/stack_v2/__init__.py | 0 1 file changed, 0 insertions(+), 0 deletions(-) create mode 100644 ddtrace/internal/datadog/profiling/stack_v2/__init__.py diff --git a/ddtrace/internal/datadog/profiling/stack_v2/__init__.py b/ddtrace/internal/datadog/profiling/stack_v2/__init__.py new file mode 100644 index 00000000000..e69de29bb2d From cd95027705955ce88f9dc1d4ab1516570cec1b77 Mon Sep 17 00:00:00 2001 From: sanchda Date: Fri, 1 Mar 2024 08:29:22 +0000 Subject: [PATCH 54/76] Tighten things up --- .../datadog/profiling/ddup/CMakeLists.txt | 2 +- .../datadog/profiling/ddup/__init__.py | 16 +++++++-- .../datadog/profiling/stack_v2/CMakeLists.txt | 4 +-- .../datadog/profiling/stack_v2/__init__.py | 33 ++++++++++++++++++ .../profiling/stack_v2/src/stack_v2.cpp | 2 +- ddtrace/profiling/collector/stack.pyx | 34 ++++++++++++------- ddtrace/profiling/collector/stack_v2.py | 19 ----------- setup.py | 2 +- 8 files changed, 73 insertions(+), 39 deletions(-) delete mode 100644 ddtrace/profiling/collector/stack_v2.py diff --git a/ddtrace/internal/datadog/profiling/ddup/CMakeLists.txt b/ddtrace/internal/datadog/profiling/ddup/CMakeLists.txt index b3b578b1d62..9a59d9344a0 100644 --- a/ddtrace/internal/datadog/profiling/ddup/CMakeLists.txt +++ b/ddtrace/internal/datadog/profiling/ddup/CMakeLists.txt @@ -67,7 +67,7 @@ set_target_properties(${EXTENSION_NAME} PROPERTIES SUFFIX "") # RPATH is needed for sofile discovery at runtime, since Python packages are not # installed in the system path. This is typical. -set_target_properties(${EXTENSION_NAME} PROPERTIES INSTALL_RPATH "$ORIGIN") +set_target_properties(${EXTENSION_NAME} PROPERTIES INSTALL_RPATH "$ORIGIN/..") target_include_directories(${EXTENSION_NAME} PRIVATE ../dd_wrapper/include ${Datadog_INCLUDE_DIRS} diff --git a/ddtrace/internal/datadog/profiling/ddup/__init__.py b/ddtrace/internal/datadog/profiling/ddup/__init__.py index 8f5ad4fe0b0..2a188df07ac 100644 --- a/ddtrace/internal/datadog/profiling/ddup/__init__.py +++ b/ddtrace/internal/datadog/profiling/ddup/__init__.py @@ -3,11 +3,23 @@ try: from ._ddup import * # noqa: F403, F401 -except Exception: + + def is_available(): + # type: () -> bool + return True + +except Exception as e: from typing import Dict # noqa:F401 from typing import Optional # noqa:F401 - from ddtrace._trace.span import Span # noqa:F401 + from ddtrace.internal.logger import get_logger + + LOG = get_logger(__name__) + LOG.error("Failed to import _ddup: %s", e) + + def is_available(): + # type: () -> bool + return False # Decorator for not-implemented def not_implemented(func): diff --git a/ddtrace/internal/datadog/profiling/stack_v2/CMakeLists.txt b/ddtrace/internal/datadog/profiling/stack_v2/CMakeLists.txt index 8dc76194270..2a021fb49b8 100644 --- a/ddtrace/internal/datadog/profiling/stack_v2/CMakeLists.txt +++ b/ddtrace/internal/datadog/profiling/stack_v2/CMakeLists.txt @@ -3,7 +3,7 @@ cmake_minimum_required(VERSION 3.19) # The exact name of this extension determines aspects of the installation and build paths, # which need to be kept in sync with setup.py. Accordingly, take the name passed in by the # caller, defaulting to "stack_v2" if needed. -set(EXTENSION_NAME "stack_v2" CACHE STRING "Name of the extension") +set(EXTENSION_NAME "_stack_v2" CACHE STRING "Name of the extension") project(${EXTENSION_NAME}) message(STATUS "Building extension: ${EXTENSION_NAME}") @@ -88,7 +88,7 @@ set_target_properties(${EXTENSION_NAME} PROPERTIES SUFFIX "") # RPATH is needed for sofile discovery at runtime, since Python packages are not # installed in the system path. This is typical. -set_target_properties(${EXTENSION_NAME} PROPERTIES INSTALL_RPATH "$ORIGIN") +set_target_properties(${EXTENSION_NAME} PROPERTIES INSTALL_RPATH "$ORIGIN/..") target_link_libraries(${EXTENSION_NAME} PRIVATE dd_wrapper ) diff --git a/ddtrace/internal/datadog/profiling/stack_v2/__init__.py b/ddtrace/internal/datadog/profiling/stack_v2/__init__.py index e69de29bb2d..500e5bc51c2 100644 --- a/ddtrace/internal/datadog/profiling/stack_v2/__init__.py +++ b/ddtrace/internal/datadog/profiling/stack_v2/__init__.py @@ -0,0 +1,33 @@ +try: + from ._stack_v2 import * # noqa: F401, F403 + + def is_available(): + # type: () -> bool + return True + +except Exception as e: + from ddtrace.internal.logger import get_logger + + LOG = get_logger(__name__) + LOG.error("Failed to import _stack_v2: %s", e) + + def is_available(): + # type: () -> bool + return False + + # Decorator for not-implemented + def not_implemented(func): + def wrapper(*args, **kwargs): + raise NotImplementedError("{} is not implemented on this platform".format(func.__name__)) + + @not_implemented + def start(*args, **kwargs): + pass + + @not_implemented + def stop(*args, **kwargs): + pass + + @not_implemented + def set_interval(*args, **kwargs): + pass diff --git a/ddtrace/internal/datadog/profiling/stack_v2/src/stack_v2.cpp b/ddtrace/internal/datadog/profiling/stack_v2/src/stack_v2.cpp index 9045a2206f0..fd99a0dbe12 100644 --- a/ddtrace/internal/datadog/profiling/stack_v2/src/stack_v2.cpp +++ b/ddtrace/internal/datadog/profiling/stack_v2/src/stack_v2.cpp @@ -56,7 +56,7 @@ static PyMethodDef _stack_v2_methods[] = { }; PyMODINIT_FUNC -PyInit_stack_v2(void) +PyInit__stack_v2(void) { PyObject* m; static struct PyModuleDef moduledef = { diff --git a/ddtrace/profiling/collector/stack.pyx b/ddtrace/profiling/collector/stack.pyx index 6ca87a95974..a0cc54ca75b 100644 --- a/ddtrace/profiling/collector/stack.pyx +++ b/ddtrace/profiling/collector/stack.pyx @@ -13,13 +13,13 @@ from ddtrace import context from ddtrace import span as ddspan from ddtrace.internal import compat from ddtrace.internal.datadog.profiling import ddup +from ddtrace.internal.datadog.profiling import stack_v2 from ddtrace.internal.utils import formats from ddtrace.profiling import _threading from ddtrace.profiling import collector from ddtrace.profiling.collector import _task from ddtrace.profiling.collector import _traceback from ddtrace.profiling.collector import stack_event -from ddtrace.profiling.collector import stack_v2 from ddtrace.settings.profiling import config @@ -486,11 +486,14 @@ class StackCollector(collector.PeriodicCollector): self.tracer.context_provider._on_activate(self._thread_span_links.link_span) # If stack_v2 is enabled, verify that it loaded properly. If not, fallback to the v1 stack collector - # and log a warning. + # and log an error. if self._stack_collector_v2_enabled: if not stack_v2.is_available(): self._stack_collector_v2_enabled = False - LOG.warning("Failed to load the v2 stack collector; falling back to the v1 stack collector") + LOG.error("Failed to load the v2 stack sampler; falling back to the v1 stack sampler") + if not ddup.is_available(): + self._stack_collector_v2_enabled = False + LOG.error("Failed to load the libdd collector; falling back to the v1 stack sampler") # Force-set use_libdd if the v2 stack collector is enabled if self._stack_collector_v2_enabled: @@ -501,6 +504,8 @@ class StackCollector(collector.PeriodicCollector): # by the next sample. stack_v2.start(min_interval=self.min_interval_time) else: + if config.export.libdd_enabled and not ddup.is_available(): + LOG.error("Failed to load the libdd collector; falling back to legacy collector") set_use_libdd(config.export.libdd_enabled) def _start_service(self): @@ -532,16 +537,19 @@ class StackCollector(collector.PeriodicCollector): now = compat.monotonic_ns() wall_time = now - self._last_wall_time self._last_wall_time = now - - all_events = stack_collect( - self.ignore_profiler, - self._thread_time, - self.nframes, - self.interval, - wall_time, - self._thread_span_links, - self.endpoint_collection_enabled, - ) + all_events = [] + + # If the stack v2 collector is enabled, then do not collect the stack samples here. + if not self._stack_collector_v2_enabled: + all_events = stack_collect( + self.ignore_profiler, + self._thread_time, + self.nframes, + self.interval, + wall_time, + self._thread_span_links, + self.endpoint_collection_enabled, + ) used_wall_time_ns = compat.monotonic_ns() - now self.interval = self._compute_new_interval(used_wall_time_ns) diff --git a/ddtrace/profiling/collector/stack_v2.py b/ddtrace/profiling/collector/stack_v2.py deleted file mode 100644 index 4c98b59a7e9..00000000000 --- a/ddtrace/profiling/collector/stack_v2.py +++ /dev/null @@ -1,19 +0,0 @@ -try: - from ddtrace.internal.datadog.profiling.stack_v2 import * # noqa: F401, F403 - - def is_available(): - return True - -except Exception: - - def is_available(): - return False - - def start(*args, **kwargs): - pass - - def stop(*args, **kwargs): - pass - - def set_interval(*args, **kwargs): - pass diff --git a/setup.py b/setup.py index 50411f448c5..3d873de5ce9 100644 --- a/setup.py +++ b/setup.py @@ -474,7 +474,7 @@ def get_exts_for(name): if sys.version_info >= (3, 8): ext_modules.append( CMakeExtension( - "ddtrace.internal.datadog.profiling.stack_v2", + "ddtrace.internal.datadog.profiling.stack_v2._stack_v2", source_dir=STACK_V2_DIR, cmake_args=[ "-DPROFILING_ROOT={}".format(PROF_NATIVE_DIR), From ee8e4d719bf814c9fc4db3652989c983565f772b Mon Sep 17 00:00:00 2001 From: sanchda Date: Fri, 1 Mar 2024 08:48:56 +0000 Subject: [PATCH 55/76] Predict whether initialization will be required in ddup --- ddtrace/profiling/profiler.py | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/ddtrace/profiling/profiler.py b/ddtrace/profiling/profiler.py index 85383f5bbe2..755c25cfd54 100644 --- a/ddtrace/profiling/profiler.py +++ b/ddtrace/profiling/profiler.py @@ -168,6 +168,21 @@ def _build_default_exporters(self): if self._lambda_function_name is not None: self.tags.update({"functionname": self._lambda_function_name}) + # The libdatadog collector gets force-enabled when the user specifies v2 of the stack sampler. + # However, that collector can only be initialized here, so we check whether the user has enabled + # v2 stack sampling and that it can be used. + if config.stack.v2_enabled: + from ddtrace.internal.datadog.profiling import stack_v2 + + if stack_v2.is_available(): + self._export_libdd_enabled = True + + # It's possible to fail to load the libdatadog collector, so we check. There's no need to do anything + # with the v2 stack sampler here, as it can figure this out on its own. + if self._export_libdd_enabled and not ddup.is_available(): + LOG.error("Failed to load the libdd collector, falling back to the legacy collector") + self._export_libdd_enabled = False + # Build the list of enabled Profiling features and send along as a tag configured_features = [] if self._stack_collector_enabled: From 2bc2c61af0c9a8f9371f639043d4ab9c343654d8 Mon Sep 17 00:00:00 2001 From: sanchda Date: Fri, 1 Mar 2024 09:26:07 +0000 Subject: [PATCH 56/76] Invert config forcing --- .../dd_wrapper/src/uploader_builder.cpp | 2 -- ddtrace/profiling/collector/stack.pyx | 28 ++++++++----------- ddtrace/profiling/profiler.py | 20 ++++++------- 3 files changed, 20 insertions(+), 30 deletions(-) diff --git a/ddtrace/internal/datadog/profiling/dd_wrapper/src/uploader_builder.cpp b/ddtrace/internal/datadog/profiling/dd_wrapper/src/uploader_builder.cpp index d69d41fb3cf..8c8496e7a40 100644 --- a/ddtrace/internal/datadog/profiling/dd_wrapper/src/uploader_builder.cpp +++ b/ddtrace/internal/datadog/profiling/dd_wrapper/src/uploader_builder.cpp @@ -119,8 +119,6 @@ UploaderBuilder::build() if (!add_tag(tags, tag, data, errmsg)) { reasons.push_back(std::string(to_string(tag)) + ": " + errmsg); } - } else { - reasons.push_back(std::string(to_string(tag)) + ": " + "empty"); } } diff --git a/ddtrace/profiling/collector/stack.pyx b/ddtrace/profiling/collector/stack.pyx index a0cc54ca75b..990d4ba677a 100644 --- a/ddtrace/profiling/collector/stack.pyx +++ b/ddtrace/profiling/collector/stack.pyx @@ -485,28 +485,24 @@ class StackCollector(collector.PeriodicCollector): self._thread_span_links = _ThreadSpanLinks() self.tracer.context_provider._on_activate(self._thread_span_links.link_span) - # If stack_v2 is enabled, verify that it loaded properly. If not, fallback to the v1 stack collector - # and log an error. + # If libdd is enabled, propagate the configuration + if config.export.libdd_enabled: + if not ddup.is_available(): + # We probably already told the user about this in profiler.py, but let's do it again here. + LOG.error("Failed to load the libdd collector; falling back to legacy collector") + set_use_libdd(False) + else: + set_use_libdd(True) + + # If stack v2 is requested, verify it is loaded properly and that libdd has been enabled. if self._stack_collector_v2_enabled: if not stack_v2.is_available(): self._stack_collector_v2_enabled = False LOG.error("Failed to load the v2 stack sampler; falling back to the v1 stack sampler") - if not ddup.is_available(): + if not use_libdd: self._stack_collector_v2_enabled = False - LOG.error("Failed to load the libdd collector; falling back to the v1 stack sampler") + LOG.error("libdd collector not enabled; falling back to the v1 stack sampler. Did you set DD_PROFILING_EXPORT_LIBDD_ENABLED=true?") - # Force-set use_libdd if the v2 stack collector is enabled - if self._stack_collector_v2_enabled: - set_use_libdd(True) - - # Restarting the profiler is infrequent; always start with the minimum interval - # instead of the dynamically configured one. The difference will get picked up - # by the next sample. - stack_v2.start(min_interval=self.min_interval_time) - else: - if config.export.libdd_enabled and not ddup.is_available(): - LOG.error("Failed to load the libdd collector; falling back to legacy collector") - set_use_libdd(config.export.libdd_enabled) def _start_service(self): # type: (...) -> None diff --git a/ddtrace/profiling/profiler.py b/ddtrace/profiling/profiler.py index 755c25cfd54..a663b0cda90 100644 --- a/ddtrace/profiling/profiler.py +++ b/ddtrace/profiling/profiler.py @@ -168,25 +168,21 @@ def _build_default_exporters(self): if self._lambda_function_name is not None: self.tags.update({"functionname": self._lambda_function_name}) - # The libdatadog collector gets force-enabled when the user specifies v2 of the stack sampler. - # However, that collector can only be initialized here, so we check whether the user has enabled - # v2 stack sampling and that it can be used. - if config.stack.v2_enabled: - from ddtrace.internal.datadog.profiling import stack_v2 - - if stack_v2.is_available(): - self._export_libdd_enabled = True - - # It's possible to fail to load the libdatadog collector, so we check. There's no need to do anything - # with the v2 stack sampler here, as it can figure this out on its own. + # It's possible to fail to load the libdatadog collector, so we check. Any other consumers + # of libdd can do their own check, so we just log. if self._export_libdd_enabled and not ddup.is_available(): LOG.error("Failed to load the libdd collector, falling back to the legacy collector") self._export_libdd_enabled = False + elif self._export_libdd_enabled: + LOG.debug("Using the libdd collector") # Build the list of enabled Profiling features and send along as a tag configured_features = [] if self._stack_collector_enabled: - configured_features.append("stack") + if config.stack.v2.enabled: + configured_features.append("stack_v2") + else: + configured_features.append("stack") if self._lock_collector_enabled: configured_features.append("lock") if self._memory_collector_enabled: From d6a4f1a55e2e24dbb1722b63babe2e61402a9ad7 Mon Sep 17 00:00:00 2001 From: sanchda Date: Fri, 1 Mar 2024 09:27:08 +0000 Subject: [PATCH 57/76] Update release note --- .../notes/profiling-add-stack-v2-975ebe845cdadc8f.yaml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/releasenotes/notes/profiling-add-stack-v2-975ebe845cdadc8f.yaml b/releasenotes/notes/profiling-add-stack-v2-975ebe845cdadc8f.yaml index 305f5bc96fc..3187d152c75 100644 --- a/releasenotes/notes/profiling-add-stack-v2-975ebe845cdadc8f.yaml +++ b/releasenotes/notes/profiling-add-stack-v2-975ebe845cdadc8f.yaml @@ -5,4 +5,5 @@ features: DD_PROFILING_STACK_V2_ENABLED=true. This new sampler should resolve segfault issues on Python 3.11 and later, while also decreasing the latency contribution of the profiler in many situations, and also improving the accuracy of stack-sampling data. This feature is currently - only available on Linux using cpython 3.8 or greater. + only available on Linux using cpython 3.8 or greater. Requires + DD_PROFILING_EXPORT_LIBDD_ENABLED=true to be set. From 71180c47fec4239c755050c6c4f224decc4d5466 Mon Sep 17 00:00:00 2001 From: sanchda Date: Fri, 1 Mar 2024 09:45:02 +0000 Subject: [PATCH 58/76] Comment fixups, make sofile propagation tighter --- ddtrace/profiling/collector/stack.pyx | 2 +- setup.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/ddtrace/profiling/collector/stack.pyx b/ddtrace/profiling/collector/stack.pyx index 990d4ba677a..341c3baa6df 100644 --- a/ddtrace/profiling/collector/stack.pyx +++ b/ddtrace/profiling/collector/stack.pyx @@ -489,7 +489,7 @@ class StackCollector(collector.PeriodicCollector): if config.export.libdd_enabled: if not ddup.is_available(): # We probably already told the user about this in profiler.py, but let's do it again here. - LOG.error("Failed to load the libdd collector; falling back to legacy collector") + LOG.error("Failed to load the libdd collector from stack.pyx; falling back to the legacy collector") set_use_libdd(False) else: set_use_libdd(True) diff --git a/setup.py b/setup.py index 3d873de5ce9..14e76e69c64 100644 --- a/setup.py +++ b/setup.py @@ -518,7 +518,7 @@ def get_exts_for(name): "ddtrace.appsec": ["rules.json"], "ddtrace.appsec._ddwaf": [str(Path("libddwaf") / "*" / "lib" / "libddwaf.*")], "ddtrace.appsec._iast._taint_tracking": ["CMakeLists.txt"], - "ddtrace.internal.datadog.profiling": ["libdd_wrapper.*"], + "ddtrace.internal.datadog.profiling.ddup": [str(Path("..") / "libdd_wrapper.*")], }, python_requires=">=3.7", zip_safe=False, From 0c33656063bf2cc427006dab1f2c9f661afe9a98 Mon Sep 17 00:00:00 2001 From: sanchda Date: Fri, 1 Mar 2024 17:49:08 +0000 Subject: [PATCH 59/76] Various * Fix and add error messages in sampler * Remove dead/unused function * Stop profiling if we ever fail to create a sample, since there is no guarantee the underlying system would still work --- .../datadog/profiling/dd_wrapper/include/sample.hpp | 3 --- .../datadog/profiling/dd_wrapper/src/sample.cpp | 4 ---- .../profiling/dd_wrapper/src/sample_manager.cpp | 4 +--- .../profiling/stack_v2/src/stack_renderer.cpp | 13 +++++++++---- .../datadog/profiling/stack_v2/src/stack_v2.cpp | 7 ++++++- ddtrace/profiling/collector/stack.pyx | 5 +++++ setup.py | 6 ++++-- 7 files changed, 25 insertions(+), 17 deletions(-) diff --git a/ddtrace/internal/datadog/profiling/dd_wrapper/include/sample.hpp b/ddtrace/internal/datadog/profiling/dd_wrapper/include/sample.hpp index da65416c240..c70b831847b 100644 --- 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 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); diff --git a/ddtrace/internal/datadog/profiling/dd_wrapper/src/sample.cpp b/ddtrace/internal/datadog/profiling/dd_wrapper/src/sample.cpp index 617007f26b4..40f5792696b 100644 --- a/ddtrace/internal/datadog/profiling/dd_wrapper/src/sample.cpp +++ b/ddtrace/internal/datadog/profiling/dd_wrapper/src/sample.cpp @@ -16,10 +16,6 @@ Sample::Sample(SampleType _type_mask, unsigned int _max_nframes) locations.reserve(max_nframes + 1); // +1 for a "truncated frames" virtual frame } -void -Sample::start_sample() -{} - void Sample::profile_clear_state() { diff --git a/ddtrace/internal/datadog/profiling/dd_wrapper/src/sample_manager.cpp b/ddtrace/internal/datadog/profiling/dd_wrapper/src/sample_manager.cpp index 6841368b75b..87fbf25d8e1 100644 --- a/ddtrace/internal/datadog/profiling/dd_wrapper/src/sample_manager.cpp +++ b/ddtrace/internal/datadog/profiling/dd_wrapper/src/sample_manager.cpp @@ -27,9 +27,7 @@ SampleManager::set_max_nframes(unsigned int _max_nframes) Sample* SampleManager::start_sample() { - auto sample = new Sample(type_mask, max_nframes); - sample->start_sample(); - return sample; + return new Sample(type_mask, max_nframes); } void 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 8b8ce850c45..8c1e704f38d 100644 --- a/ddtrace/internal/datadog/profiling/stack_v2/src/stack_renderer.cpp +++ b/ddtrace/internal/datadog/profiling/stack_v2/src/stack_renderer.cpp @@ -18,9 +18,14 @@ StackRenderer::render_thread_begin(PyThreadState* tstate, unsigned long native_id) { (void)tstate; + static bool failed = false; + if (failed) { + return; + } sample = ddup_start_sample(); if (sample == nullptr) { - std::cerr << "Failed to create a sample. Profiling data will be lost." << std::endl; + std::cerr << "Failed to create a sample. Stack v2 sampler will be disabled." << std::endl; + failed = true; return; } @@ -39,7 +44,7 @@ void StackRenderer::render_python_frame(std::string_view name, std::string_view file, uint64_t line) { if (sample == nullptr) { - std::cerr << "Failed to create a sample. Profiling data will be lost." << std::endl; + std::cerr << "Received a new frame without sample storage. Some profiling data has been lost." << std::endl; return; } @@ -55,8 +60,6 @@ StackRenderer::render_python_frame(std::string_view name, std::string_view file, file = invalid; } ddup_push_frame(sample, name, file, 0, line); - ddup_drop_sample(sample); - sample = nullptr; } void @@ -72,6 +75,7 @@ void StackRenderer::render_cpu_time(microsecond_t cpu_time_us) { if (sample == nullptr) { + std::cerr << "Received a CPU time without sample storage. Some profiling data has been lost." << std::endl; return; } @@ -83,6 +87,7 @@ void StackRenderer::render_stack_end() { if (sample == nullptr) { + std::cerr << "Ending a stack without any context. Some profiling data has been lost." << std::endl; return; } diff --git a/ddtrace/internal/datadog/profiling/stack_v2/src/stack_v2.cpp b/ddtrace/internal/datadog/profiling/stack_v2/src/stack_v2.cpp index fd99a0dbe12..bde6bec8cc8 100644 --- a/ddtrace/internal/datadog/profiling/stack_v2/src/stack_v2.cpp +++ b/ddtrace/internal/datadog/profiling/stack_v2/src/stack_v2.cpp @@ -2,12 +2,15 @@ #include "python_headers.hpp" #include "sampler.hpp" +#include + using namespace Datadog; static PyObject* _stack_v2_start(PyObject* self, PyObject* args, PyObject* kwargs) { (void)self; + std::cout << "Starting the sampler" << std::endl; static const char* const_kwlist[] = { "min_interval", NULL }; static char** kwlist = const_cast(const_kwlist); double min_interval_s = g_default_sampling_period_s; @@ -17,8 +20,10 @@ _stack_v2_start(PyObject* self, PyObject* args, PyObject* kwargs) } Sampler::get().set_interval(min_interval_s); + Sampler::get().start(); - return PyLong_FromLong(1); + Py_INCREF(Py_None); + return Py_None; } // Bypasses the old-style cast warning with an unchecked helper function diff --git a/ddtrace/profiling/collector/stack.pyx b/ddtrace/profiling/collector/stack.pyx index 341c3baa6df..d43718b8e3f 100644 --- a/ddtrace/profiling/collector/stack.pyx +++ b/ddtrace/profiling/collector/stack.pyx @@ -503,6 +503,11 @@ class StackCollector(collector.PeriodicCollector): self._stack_collector_v2_enabled = False LOG.error("libdd collector not enabled; falling back to the v1 stack sampler. Did you set DD_PROFILING_EXPORT_LIBDD_ENABLED=true?") + # If at the end of things, stack v2 is still enabled, then start the native thread running the v2 sampler + if self._stack_collector_v2_enabled: + LOG.debug("Starting the stack v2 sampler") + stack_v2.start() + def _start_service(self): # type: (...) -> None diff --git a/setup.py b/setup.py index 14e76e69c64..c21f8a4b660 100644 --- a/setup.py +++ b/setup.py @@ -461,6 +461,7 @@ def get_exts_for(name): CMakeExtension( "ddtrace.internal.datadog.profiling.ddup._ddup", source_dir=DDUP_DIR, + optional=False, cmake_args=[ "-DPY_MAJOR_VERSION={}".format(sys.version_info.major), "-DPY_MINOR_VERSION={}".format(sys.version_info.minor), @@ -476,6 +477,7 @@ def get_exts_for(name): CMakeExtension( "ddtrace.internal.datadog.profiling.stack_v2._stack_v2", source_dir=STACK_V2_DIR, + optional=False, cmake_args=[ "-DPROFILING_ROOT={}".format(PROF_NATIVE_DIR), ], @@ -516,9 +518,9 @@ def get_exts_for(name): package_data={ "ddtrace": ["py.typed"], "ddtrace.appsec": ["rules.json"], - "ddtrace.appsec._ddwaf": [str(Path("libddwaf") / "*" / "lib" / "libddwaf.*")], + "ddtrace.appsec._ddwaf": ["libddwaf/*/lib/libddwaf.*"], "ddtrace.appsec._iast._taint_tracking": ["CMakeLists.txt"], - "ddtrace.internal.datadog.profiling.ddup": [str(Path("..") / "libdd_wrapper.*")], + "ddtrace.internal.datadog.profiling": ["libdd_wrapper.*"], }, python_requires=">=3.7", zip_safe=False, From 6c381f9aadc192aa739b930588d1ff6ba75082ff Mon Sep 17 00:00:00 2001 From: sanchda Date: Fri, 1 Mar 2024 18:05:01 +0000 Subject: [PATCH 60/76] Slight refactor to help package_data work --- .../datadog/profiling/dd_wrapper/CMakeLists.txt | 11 ++++++----- setup.py | 4 ---- 2 files changed, 6 insertions(+), 9 deletions(-) diff --git a/ddtrace/internal/datadog/profiling/dd_wrapper/CMakeLists.txt b/ddtrace/internal/datadog/profiling/dd_wrapper/CMakeLists.txt index 1adb13c81e5..9013774b7a5 100644 --- a/ddtrace/internal/datadog/profiling/dd_wrapper/CMakeLists.txt +++ b/ddtrace/internal/datadog/profiling/dd_wrapper/CMakeLists.txt @@ -48,12 +48,13 @@ target_link_libraries(dd_wrapper PRIVATE ) set_target_properties(dd_wrapper PROPERTIES POSITION_INDEPENDENT_CODE ON) -# If LIB_INSTALL_DIR is set, install the library -if (PROFILING_ROOT) +# 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 ${PROFILING_ROOT} - ARCHIVE DESTINATION ${OFILING_ROOT} - RUNTIME DESTINATION ${OFILING_ROOT} + LIBRARY DESTINATION ${LIB_INSTALL_DIR}/.. + ARCHIVE DESTINATION ${LIB_INSTALL_DIR}/.. + RUNTIME DESTINATION ${LIB_INSTALL_DIR}/.. ) endif() diff --git a/setup.py b/setup.py index c21f8a4b660..e3f93b598b3 100644 --- a/setup.py +++ b/setup.py @@ -466,7 +466,6 @@ def get_exts_for(name): "-DPY_MAJOR_VERSION={}".format(sys.version_info.major), "-DPY_MINOR_VERSION={}".format(sys.version_info.minor), "-DPY_MICRO_VERSION={}".format(sys.version_info.micro), - "-DPROFILING_ROOT={}".format(PROF_NATIVE_DIR), ], ) ) @@ -478,9 +477,6 @@ def get_exts_for(name): "ddtrace.internal.datadog.profiling.stack_v2._stack_v2", source_dir=STACK_V2_DIR, optional=False, - cmake_args=[ - "-DPROFILING_ROOT={}".format(PROF_NATIVE_DIR), - ], ) ) From 50b24b073f31d76d88f0caee36637f3b302b8cf6 Mon Sep 17 00:00:00 2001 From: sanchda Date: Fri, 1 Mar 2024 18:29:39 +0000 Subject: [PATCH 61/76] Fix slotscheck issue --- pyproject.toml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/pyproject.toml b/pyproject.toml index 1c0688a8c5d..92c2c17b1a8 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -150,6 +150,9 @@ exclude-modules = ''' | ddtrace.internal.datastreams.kafka # libdd_wrapper is a common native dependency, not a module | ddtrace.internal.datadog.profiling.libdd_wrapper + # _ddup and _stack_v2 miss a runtime dependency in slotscheck, but ddup and stack_v2 are fine + | ddtrace.internal.datadog.profiling.ddup._ddup + | ddtrace.internal.datadog.profiling.collector._stack_v2 ) ''' From 6b85fea43cf4b5cdad856659786e8e26749ab94c Mon Sep 17 00:00:00 2001 From: sanchda Date: Fri, 1 Mar 2024 18:39:33 +0000 Subject: [PATCH 62/76] Path fix in slotscheck Some days I have a hard time convincing myself that I am actually completely here. --- pyproject.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pyproject.toml b/pyproject.toml index 92c2c17b1a8..c8235fa7cf3 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -152,7 +152,7 @@ exclude-modules = ''' | ddtrace.internal.datadog.profiling.libdd_wrapper # _ddup and _stack_v2 miss a runtime dependency in slotscheck, but ddup and stack_v2 are fine | ddtrace.internal.datadog.profiling.ddup._ddup - | ddtrace.internal.datadog.profiling.collector._stack_v2 + | ddtrace.internal.datadog.profiling.stack_v2._stack_v2 ) ''' From c1b2b9542c57232cbe43505c3e550421bf2624a3 Mon Sep 17 00:00:00 2001 From: sanchda Date: Fri, 1 Mar 2024 19:27:56 +0000 Subject: [PATCH 63/76] Downgrade module error to warning --- ddtrace/internal/datadog/profiling/ddup/__init__.py | 2 +- ddtrace/internal/datadog/profiling/stack_v2/__init__.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/ddtrace/internal/datadog/profiling/ddup/__init__.py b/ddtrace/internal/datadog/profiling/ddup/__init__.py index 2a188df07ac..f2c39e5e93d 100644 --- a/ddtrace/internal/datadog/profiling/ddup/__init__.py +++ b/ddtrace/internal/datadog/profiling/ddup/__init__.py @@ -15,7 +15,7 @@ def is_available(): from ddtrace.internal.logger import get_logger LOG = get_logger(__name__) - LOG.error("Failed to import _ddup: %s", e) + LOG.warning("Failed to import _ddup: %s", e) def is_available(): # type: () -> bool diff --git a/ddtrace/internal/datadog/profiling/stack_v2/__init__.py b/ddtrace/internal/datadog/profiling/stack_v2/__init__.py index 500e5bc51c2..945851fcd34 100644 --- a/ddtrace/internal/datadog/profiling/stack_v2/__init__.py +++ b/ddtrace/internal/datadog/profiling/stack_v2/__init__.py @@ -9,7 +9,7 @@ def is_available(): from ddtrace.internal.logger import get_logger LOG = get_logger(__name__) - LOG.error("Failed to import _stack_v2: %s", e) + LOG.warning("Failed to import _stack_v2: %s", e) def is_available(): # type: () -> bool From 1de4559746db25331deb2acfc041ceab0b1200f8 Mon Sep 17 00:00:00 2001 From: sanchda Date: Fri, 1 Mar 2024 19:46:35 +0000 Subject: [PATCH 64/76] Further downgrade to debug --- ddtrace/internal/datadog/profiling/ddup/__init__.py | 2 +- ddtrace/internal/datadog/profiling/stack_v2/__init__.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/ddtrace/internal/datadog/profiling/ddup/__init__.py b/ddtrace/internal/datadog/profiling/ddup/__init__.py index f2c39e5e93d..4e241a02e42 100644 --- a/ddtrace/internal/datadog/profiling/ddup/__init__.py +++ b/ddtrace/internal/datadog/profiling/ddup/__init__.py @@ -15,7 +15,7 @@ def is_available(): from ddtrace.internal.logger import get_logger LOG = get_logger(__name__) - LOG.warning("Failed to import _ddup: %s", e) + LOG.debug("Failed to import _ddup: %s", e) def is_available(): # type: () -> bool diff --git a/ddtrace/internal/datadog/profiling/stack_v2/__init__.py b/ddtrace/internal/datadog/profiling/stack_v2/__init__.py index 945851fcd34..e3c04367ef2 100644 --- a/ddtrace/internal/datadog/profiling/stack_v2/__init__.py +++ b/ddtrace/internal/datadog/profiling/stack_v2/__init__.py @@ -9,7 +9,7 @@ def is_available(): from ddtrace.internal.logger import get_logger LOG = get_logger(__name__) - LOG.warning("Failed to import _stack_v2: %s", e) + LOG.debug("Failed to import _stack_v2: %s", e) def is_available(): # type: () -> bool From de030824f780430623813b30b19f02b8b429f66c Mon Sep 17 00:00:00 2001 From: sanchda Date: Fri, 1 Mar 2024 20:38:30 +0000 Subject: [PATCH 65/76] Clarifying comment --- ddtrace/profiling/collector/stack.pyx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ddtrace/profiling/collector/stack.pyx b/ddtrace/profiling/collector/stack.pyx index d43718b8e3f..9691c7034da 100644 --- a/ddtrace/profiling/collector/stack.pyx +++ b/ddtrace/profiling/collector/stack.pyx @@ -33,8 +33,8 @@ FEATURES = { "transparent_events": False, } -# . This is handled at the level of -# a global rather than a passed parameter because this is a time of transition +# Switch to use libdd (ddup) collector/exporter. Handled as a global since we're transitioning into +# the now-experimental interface. cdef bint use_libdd = False cdef void set_use_libdd(bint flag): From 29029f65580436d5499acef808d6af03192bf3b1 Mon Sep 17 00:00:00 2001 From: sanchda Date: Fri, 1 Mar 2024 20:52:56 +0000 Subject: [PATCH 66/76] Make native extensions all optional --- setup.py | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/setup.py b/setup.py index e3f93b598b3..13ee9243473 100644 --- a/setup.py +++ b/setup.py @@ -42,9 +42,8 @@ LIBDDWAF_DOWNLOAD_DIR = HERE / "ddtrace" / "appsec" / "_ddwaf" / "libddwaf" IAST_DIR = HERE / "ddtrace" / "appsec" / "_iast" / "_taint_tracking" -PROF_NATIVE_DIR = HERE / "ddtrace" / "internal" / "datadog" / "profiling" -DDUP_DIR = PROF_NATIVE_DIR / "ddup" -STACK_V2_DIR = PROF_NATIVE_DIR / "stack_v2" +DDUP_DIR = HERE / "ddtrace" / "internal" / "datadog" / "profiling" / "ddup" +STACK_V2_DIR = HERE / "ddtrace" / "internal" / "datadog" / "profiling" / "stack_v2" CURRENT_OS = platform.system() @@ -461,7 +460,6 @@ def get_exts_for(name): CMakeExtension( "ddtrace.internal.datadog.profiling.ddup._ddup", source_dir=DDUP_DIR, - optional=False, cmake_args=[ "-DPY_MAJOR_VERSION={}".format(sys.version_info.major), "-DPY_MINOR_VERSION={}".format(sys.version_info.minor), @@ -476,7 +474,6 @@ def get_exts_for(name): CMakeExtension( "ddtrace.internal.datadog.profiling.stack_v2._stack_v2", source_dir=STACK_V2_DIR, - optional=False, ) ) From bfa096f9787a5343b8a580d811df6b74b8f03937 Mon Sep 17 00:00:00 2001 From: David Sanchez <838104+sanchda@users.noreply.github.com> Date: Mon, 4 Mar 2024 09:50:13 -0600 Subject: [PATCH 67/76] Update releasenotes/notes/profiling-add-stack-v2-975ebe845cdadc8f.yaml Co-authored-by: Tahir H. Butt --- releasenotes/notes/profiling-add-stack-v2-975ebe845cdadc8f.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/releasenotes/notes/profiling-add-stack-v2-975ebe845cdadc8f.yaml b/releasenotes/notes/profiling-add-stack-v2-975ebe845cdadc8f.yaml index 3187d152c75..ea18576ec45 100644 --- a/releasenotes/notes/profiling-add-stack-v2-975ebe845cdadc8f.yaml +++ b/releasenotes/notes/profiling-add-stack-v2-975ebe845cdadc8f.yaml @@ -5,5 +5,5 @@ features: DD_PROFILING_STACK_V2_ENABLED=true. This new sampler should resolve segfault issues on Python 3.11 and later, while also decreasing the latency contribution of the profiler in many situations, and also improving the accuracy of stack-sampling data. This feature is currently - only available on Linux using cpython 3.8 or greater. Requires + only available on Linux using CPython 3.8 or greater. Requires DD_PROFILING_EXPORT_LIBDD_ENABLED=true to be set. From a07efd9fb3a73439f60952905951c0655917ec58 Mon Sep 17 00:00:00 2001 From: David Sanchez <838104+sanchda@users.noreply.github.com> Date: Mon, 4 Mar 2024 09:50:18 -0600 Subject: [PATCH 68/76] Update releasenotes/notes/profiling-add-stack-v2-975ebe845cdadc8f.yaml Co-authored-by: Tahir H. Butt --- releasenotes/notes/profiling-add-stack-v2-975ebe845cdadc8f.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/releasenotes/notes/profiling-add-stack-v2-975ebe845cdadc8f.yaml b/releasenotes/notes/profiling-add-stack-v2-975ebe845cdadc8f.yaml index ea18576ec45..c8590528abf 100644 --- a/releasenotes/notes/profiling-add-stack-v2-975ebe845cdadc8f.yaml +++ b/releasenotes/notes/profiling-add-stack-v2-975ebe845cdadc8f.yaml @@ -6,4 +6,4 @@ features: 3.11 and later, while also decreasing the latency contribution of the profiler in many situations, and also improving the accuracy of stack-sampling data. This feature is currently only available on Linux using CPython 3.8 or greater. Requires - DD_PROFILING_EXPORT_LIBDD_ENABLED=true to be set. + ``DD_PROFILING_EXPORT_LIBDD_ENABLED=true`` to be set. From af937a7f0a7f29188c0f01c135a42fbe52516459 Mon Sep 17 00:00:00 2001 From: David Sanchez <838104+sanchda@users.noreply.github.com> Date: Mon, 4 Mar 2024 09:50:23 -0600 Subject: [PATCH 69/76] Update releasenotes/notes/profiling-add-stack-v2-975ebe845cdadc8f.yaml Co-authored-by: Tahir H. Butt --- releasenotes/notes/profiling-add-stack-v2-975ebe845cdadc8f.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/releasenotes/notes/profiling-add-stack-v2-975ebe845cdadc8f.yaml b/releasenotes/notes/profiling-add-stack-v2-975ebe845cdadc8f.yaml index c8590528abf..a6ebdbd0b62 100644 --- a/releasenotes/notes/profiling-add-stack-v2-975ebe845cdadc8f.yaml +++ b/releasenotes/notes/profiling-add-stack-v2-975ebe845cdadc8f.yaml @@ -2,7 +2,7 @@ features: - | profiling: implement an experimental stack sampling feature, which can be enabled by setting - DD_PROFILING_STACK_V2_ENABLED=true. This new sampler should resolve segfault issues on Python + ``DD_PROFILING_STACK_V2_ENABLED=true``. This new sampler should resolve segfault issues on Python 3.11 and later, while also decreasing the latency contribution of the profiler in many situations, and also improving the accuracy of stack-sampling data. This feature is currently only available on Linux using CPython 3.8 or greater. Requires From 6d5622744a3a5e6472bb26bc15a52b97348bfa47 Mon Sep 17 00:00:00 2001 From: David Sanchez <838104+sanchda@users.noreply.github.com> Date: Mon, 4 Mar 2024 09:51:52 -0600 Subject: [PATCH 70/76] Update setup.py --- setup.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/setup.py b/setup.py index 13ee9243473..5d10e4834dc 100644 --- a/setup.py +++ b/setup.py @@ -468,7 +468,7 @@ def get_exts_for(name): ) ) - # One of the stack v2 dependencies doesn't quite work on 3.7, so only support later for now + # Echion doesn't build on 3.7, so just skip it outright for now if sys.version_info >= (3, 8): ext_modules.append( CMakeExtension( From ad1e0cc857dcf2e35915e6228455766ea9e35b05 Mon Sep 17 00:00:00 2001 From: David Sanchez <838104+sanchda@users.noreply.github.com> Date: Tue, 5 Mar 2024 07:11:53 -0600 Subject: [PATCH 71/76] Update ddtrace/internal/datadog/profiling/stack_v2/src/stack_v2.cpp Co-authored-by: Gabriele N. Tornetta --- ddtrace/internal/datadog/profiling/stack_v2/src/stack_v2.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/ddtrace/internal/datadog/profiling/stack_v2/src/stack_v2.cpp b/ddtrace/internal/datadog/profiling/stack_v2/src/stack_v2.cpp index bde6bec8cc8..9b89c0417c1 100644 --- a/ddtrace/internal/datadog/profiling/stack_v2/src/stack_v2.cpp +++ b/ddtrace/internal/datadog/profiling/stack_v2/src/stack_v2.cpp @@ -49,8 +49,7 @@ stack_v2_set_interval(PyObject* self, PyObject* args) return NULL; // If an error occurs during argument parsing } Sampler::get().set_interval(new_interval); - Py_INCREF(Py_None); - return Py_None; + Py_RETURN_NONE; } static PyMethodDef _stack_v2_methods[] = { From 5da44b05b4266cc016217054480c7cf147f2895e Mon Sep 17 00:00:00 2001 From: David Sanchez <838104+sanchda@users.noreply.github.com> Date: Tue, 5 Mar 2024 07:12:13 -0600 Subject: [PATCH 72/76] Update ddtrace/internal/datadog/profiling/stack_v2/src/stack_v2.cpp Co-authored-by: Gabriele N. Tornetta --- ddtrace/internal/datadog/profiling/stack_v2/src/stack_v2.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/ddtrace/internal/datadog/profiling/stack_v2/src/stack_v2.cpp b/ddtrace/internal/datadog/profiling/stack_v2/src/stack_v2.cpp index 9b89c0417c1..dd816836a17 100644 --- a/ddtrace/internal/datadog/profiling/stack_v2/src/stack_v2.cpp +++ b/ddtrace/internal/datadog/profiling/stack_v2/src/stack_v2.cpp @@ -35,8 +35,7 @@ stack_v2_stop(PyObject* self, PyObject* args) (void)self; (void)args; Sampler::get().stop(); - Py_INCREF(Py_None); - return Py_None; + Py_RETURN_NONE } static PyObject* From 5aee9c03bdaab1ebf16a5fad88af0c11b85af53b Mon Sep 17 00:00:00 2001 From: David Sanchez <838104+sanchda@users.noreply.github.com> Date: Tue, 5 Mar 2024 07:13:32 -0600 Subject: [PATCH 73/76] Update profiling.py --- ddtrace/settings/profiling.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ddtrace/settings/profiling.py b/ddtrace/settings/profiling.py index 1f8dcaaa04c..7496910ca22 100644 --- a/ddtrace/settings/profiling.py +++ b/ddtrace/settings/profiling.py @@ -46,7 +46,7 @@ def _is_valid_libdatadog(): def _is_valid_v2_stack(): # type: () -> bool - return sys.version_info >= (3, 7) and _is_valid_libdatadog() + return sys.version_info > (3, 7) and _is_valid_libdatadog() class ProfilingConfig(En): From 30ec823eaed5fdd0fa1b20d55918517763930b2f Mon Sep 17 00:00:00 2001 From: David Sanchez <838104+sanchda@users.noreply.github.com> Date: Tue, 5 Mar 2024 07:19:06 -0600 Subject: [PATCH 74/76] Remove eager config checks --- ddtrace/settings/profiling.py | 18 ++---------------- 1 file changed, 2 insertions(+), 16 deletions(-) diff --git a/ddtrace/settings/profiling.py b/ddtrace/settings/profiling.py index 7496910ca22..a394bbd7adf 100644 --- a/ddtrace/settings/profiling.py +++ b/ddtrace/settings/profiling.py @@ -39,16 +39,6 @@ def _derive_default_heap_sample_size(heap_config, default_heap_sample_size=1024 return int(max(math.ceil(total_mem / max_samples), default_heap_sample_size)) -def _is_valid_libdatadog(): - # type: () -> bool - return platform.system() == "Linux" - - -def _is_valid_v2_stack(): - # type: () -> bool - return sys.version_info > (3, 7) and _is_valid_libdatadog() - - class ProfilingConfig(En): __prefix__ = "dd.profiling" @@ -174,14 +164,13 @@ class Stack(En): class V2(En): __item__ = __prefix__ = "v2" - _enabled = En.v( + enabled = En.v( bool, "enabled", default=False, help_type="Boolean", help="Whether to enable the v2 stack profiler. Also enables the libdatadog collector.", ) - enabled = En.d(bool, lambda c: c._enabled and _is_valid_v2_stack()) class Lock(En): __item__ = __prefix__ = "lock" @@ -236,7 +225,7 @@ class Heap(En): class Export(En): __item__ = __prefix__ = "export" - _libdd_enabled = En.v( + libdd_enabled = En.v( bool, "libdd_enabled", default=False, @@ -244,8 +233,5 @@ class Export(En): help="Enables collection and export using the experimental exporter", ) - # Only available in certain configurations - libdd_enabled = En.d(bool, lambda c: c._libdd_enabled and _is_valid_libdatadog()) - config = ProfilingConfig() From 1fa55c881aea8d00dd793c95033c0db824d43726 Mon Sep 17 00:00:00 2001 From: sanchda Date: Tue, 5 Mar 2024 13:28:11 +0000 Subject: [PATCH 75/76] Make is_available a module attribute --- ddtrace/internal/datadog/profiling/ddup/__init__.py | 8 ++------ ddtrace/internal/datadog/profiling/stack_v2/__init__.py | 8 ++------ ddtrace/profiling/collector/stack.pyx | 4 ++-- ddtrace/profiling/profiler.py | 2 +- 4 files changed, 7 insertions(+), 15 deletions(-) diff --git a/ddtrace/internal/datadog/profiling/ddup/__init__.py b/ddtrace/internal/datadog/profiling/ddup/__init__.py index 4e241a02e42..32bd273c5a4 100644 --- a/ddtrace/internal/datadog/profiling/ddup/__init__.py +++ b/ddtrace/internal/datadog/profiling/ddup/__init__.py @@ -4,9 +4,7 @@ try: from ._ddup import * # noqa: F403, F401 - def is_available(): - # type: () -> bool - return True + is_available = True except Exception as e: from typing import Dict # noqa:F401 @@ -17,9 +15,7 @@ def is_available(): LOG = get_logger(__name__) LOG.debug("Failed to import _ddup: %s", e) - def is_available(): - # type: () -> bool - return False + is_available = False # Decorator for not-implemented def not_implemented(func): diff --git a/ddtrace/internal/datadog/profiling/stack_v2/__init__.py b/ddtrace/internal/datadog/profiling/stack_v2/__init__.py index e3c04367ef2..040035082b3 100644 --- a/ddtrace/internal/datadog/profiling/stack_v2/__init__.py +++ b/ddtrace/internal/datadog/profiling/stack_v2/__init__.py @@ -1,9 +1,7 @@ try: from ._stack_v2 import * # noqa: F401, F403 - def is_available(): - # type: () -> bool - return True + is_available = True except Exception as e: from ddtrace.internal.logger import get_logger @@ -11,9 +9,7 @@ def is_available(): LOG = get_logger(__name__) LOG.debug("Failed to import _stack_v2: %s", e) - def is_available(): - # type: () -> bool - return False + is_available = False # Decorator for not-implemented def not_implemented(func): diff --git a/ddtrace/profiling/collector/stack.pyx b/ddtrace/profiling/collector/stack.pyx index 9691c7034da..c9d15bd124d 100644 --- a/ddtrace/profiling/collector/stack.pyx +++ b/ddtrace/profiling/collector/stack.pyx @@ -487,7 +487,7 @@ class StackCollector(collector.PeriodicCollector): # If libdd is enabled, propagate the configuration if config.export.libdd_enabled: - if not ddup.is_available(): + if not ddup.is_available: # We probably already told the user about this in profiler.py, but let's do it again here. LOG.error("Failed to load the libdd collector from stack.pyx; falling back to the legacy collector") set_use_libdd(False) @@ -496,7 +496,7 @@ class StackCollector(collector.PeriodicCollector): # If stack v2 is requested, verify it is loaded properly and that libdd has been enabled. if self._stack_collector_v2_enabled: - if not stack_v2.is_available(): + if not stack_v2.is_available: self._stack_collector_v2_enabled = False LOG.error("Failed to load the v2 stack sampler; falling back to the v1 stack sampler") if not use_libdd: diff --git a/ddtrace/profiling/profiler.py b/ddtrace/profiling/profiler.py index a663b0cda90..e7382106f34 100644 --- a/ddtrace/profiling/profiler.py +++ b/ddtrace/profiling/profiler.py @@ -170,7 +170,7 @@ def _build_default_exporters(self): # It's possible to fail to load the libdatadog collector, so we check. Any other consumers # of libdd can do their own check, so we just log. - if self._export_libdd_enabled and not ddup.is_available(): + if self._export_libdd_enabled and not ddup.is_available: LOG.error("Failed to load the libdd collector, falling back to the legacy collector") self._export_libdd_enabled = False elif self._export_libdd_enabled: From 24489c6e8b87aecbca38c43813cefbff3956f599 Mon Sep 17 00:00:00 2001 From: sanchda Date: Tue, 5 Mar 2024 13:29:11 +0000 Subject: [PATCH 76/76] Remove now-unneeded deps from profiler settings --- ddtrace/settings/profiling.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/ddtrace/settings/profiling.py b/ddtrace/settings/profiling.py index a394bbd7adf..a84549ed71c 100644 --- a/ddtrace/settings/profiling.py +++ b/ddtrace/settings/profiling.py @@ -1,6 +1,4 @@ import math -import platform -import sys import typing as t from envier import En