fix(profiling): make stack_v2 and ddup more robust against failure [backport #8471 to 2.7] (#8636)

Backport of #8615 to 2.7

This fixes a few defects in how the ddup and stack_v2 native extensions
permitted failure.

It also modifies the cmake invocation in setup.py so as to use the one
provided by the `cmake` module we already depended upon. I thought the
old way would have given it to us, but it appears as though the 2.7.1
release revealed the profiling-related native extensions failed to
build.

## Checklist

- [x] Change(s) are motivated and described in the PR description
- [x] Testing strategy is described if automated tests are not included
in the PR
- [x] Risks are described (performance impact, potential for breakage,
maintainability)
- [x] Change is maintainable (easy to change, telemetry, documentation)
- [x] [Library release note
guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html)
are followed or label `changelog/no-changelog` is set
- [x] Documentation is included (in-code, generated user docs, [public
corp docs](https://github.com/DataDog/documentation/))
- [x] Backport labels are set (if
[applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting))
- [x] If this PR changes the public interface, I've notified
`@DataDog/apm-tees`.
- [x] If change touches code that signs or publishes builds or packages,
or handles credentials of any kind, I've requested a review from
`@DataDog/security-design-and-guidance`.

## Reviewer Checklist

- [x] Title is accurate
- [x] All changes are related to the pull request's stated goal
- [x] Description motivates each change
- [x] Avoids breaking
[API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces)
changes
- [x] Testing strategy adequately addresses listed risks
- [x] Change is maintainable (easy to change, telemetry, documentation)
- [x] Release note makes sense to a user of the library
- [x] Author has acknowledged and discussed the performance implications
of this PR as reported in the benchmarks PR comment
- [x] Backport labels are set in a manner that is consistent with the
[release branch maintenance

policy](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)

---------

Co-authored-by: sanchda <[email protected]>

Author: sanchda

.github/workflows/build_deploy.yml

@@ -78,7 +78,7 @@ jobs:
 
       - name: Build sdist
         run: |
-          pip install cython
+          pip install cython cmake
           python setup.py sdist
       - uses: actions/upload-artifact@v3
         with:

.github/workflows/build_python_3.yml

@@ -56,6 +56,7 @@ jobs:
           CMAKE_BUILD_PARALLEL_LEVEL: 12
           CIBW_REPAIR_WHEEL_COMMAND_LINUX: |
             mkdir ./tempwheelhouse &&
+            unzip -l {wheel} | grep '\.so' &&
             auditwheel repair -w ./tempwheelhouse {wheel} &&
             (yum install -y zip || apk add zip) &&
             for w in ./tempwheelhouse/*.whl; do

ddtrace/internal/datadog/profiling/ddup/__init__.py

@@ -1,95 +1,99 @@
+from typing import Dict  # noqa:F401
+from typing import Optional  # noqa:F401
+
 from .utils import sanitize_string  # noqa: F401
 
 
-try:
-    from ._ddup import *  # noqa: F403, F401
+is_available = False
 
-    is_available = True
 
-except Exception as e:
-    from typing import Dict  # noqa:F401
-    from typing import Optional  # noqa:F401
+def not_implemented(func):
+    def wrapper(*args, **kwargs):
+        raise NotImplementedError("{} is not implemented on this platform".format(func.__name__))
 
-    from ddtrace.internal.logger import get_logger
 
-    LOG = get_logger(__name__)
-    LOG.debug("Failed to import _ddup: %s", e)
+@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
+
 
-    is_available = False
+@not_implemented
+def upload():  # type: () -> None
+    pass
 
-    # Decorator for not-implemented
-    def not_implemented(func):
-        def wrapper(*args, **kwargs):
-            raise NotImplementedError("{} is not implemented on this platform".format(func.__name__))
 
+class SampleHandle:
     @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]
-    ):
+    def push_cputime(self, value, count):  # type: (int, int) -> None
         pass
 
     @not_implemented
-    def upload():  # type: () -> None
+    def push_walltime(self, value, count):  # type: (int, int) -> None
         pass
 
-    class SampleHandle:
-        @not_implemented
-        def push_cputime(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_walltime(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_acquire(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_release(self, value, count):  # type: (int, int) -> None
-            pass
+    @not_implemented
+    def push_heap(self, value):  # type: (int) -> None
+        pass
 
-        @not_implemented
-        def push_alloc(self, value, count):  # type: (int, int) -> None
-            pass
+    @not_implemented
+    def push_lock_name(self, lock_name):  # type: (str) -> None
+        pass
 
-        @not_implemented
-        def push_heap(self, value):  # type: (int) -> None
-            pass
+    @not_implemented
+    def push_frame(self, name, filename, address, line):  # type: (str, str, int, int) -> None
+        pass
 
-        @not_implemented
-        def push_lock_name(self, lock_name):  # type: (str) -> 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_frame(self, name, filename, address, line):  # type: (str, str, int, int) -> None
-            pass
+    @not_implemented
+    def push_taskinfo(self, task_id, task_name):  # type: (int, str) -> 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_exceptioninfo(self, exc_type, count):  # type: (type, int) -> None
+        pass
 
-        @not_implemented
-        def push_taskinfo(self, task_id, task_name):  # type: (int, str) -> None
-            pass
+    @not_implemented
+    def push_class_name(self, class_name):  # type: (str) -> None
+        pass
 
-        @not_implemented
-        def push_exceptioninfo(self, exc_type, count):  # type: (type, int) -> None
-            pass
+    @not_implemented
+    def push_span(self, span, endpoint_collection_enabled):  # type: (Optional[Span], bool) -> None
+        pass
 
-        @not_implemented
-        def push_class_name(self, class_name):  # type: (str) -> None
-            pass
+    @not_implemented
+    def flush_sample(self):  # type: () -> 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
+try:
+    from ._ddup import *  # noqa: F403, F401
+
+    is_available = True
+
+except Exception as e:
+    from ddtrace.internal.logger import get_logger
+
+    LOG = get_logger(__name__)
+    LOG.debug("Failed to import _ddup: %s", e)

ddtrace/internal/datadog/profiling/stack_v2/__init__.py

@@ -1,3 +1,27 @@
+is_available = 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
+
+
 try:
     from ._stack_v2 import *  # noqa: F401, F403
 
@@ -7,23 +31,5 @@
     from ddtrace.internal.logger import get_logger
 
     LOG = get_logger(__name__)
-    LOG.debug("Failed to import _stack_v2: %s", e)
-
-    is_available = 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
+    LOG.debug("Failed to import _stack_v2: %s", e)

ddtrace/internal/datadog/profiling/stack_v2/src/stack_v2.cpp

@@ -2,15 +2,12 @@
 #include "python_headers.hpp"
 #include "sampler.hpp"
 
-#include <iostream>
-
 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<char**>(const_kwlist);
     double min_interval_s = g_default_sampling_period_s;
@@ -21,9 +18,7 @@ _stack_v2_start(PyObject* self, PyObject* args, PyObject* kwargs)
 
     Sampler::get().set_interval(min_interval_s);
     Sampler::get().start();
-
-    Py_INCREF(Py_None);
-    return Py_None;
+    Py_RETURN_NONE;
 }
 
 // Bypasses the old-style cast warning with an unchecked helper function
@@ -35,7 +30,7 @@ stack_v2_stop(PyObject* self, PyObject* args)
     (void)self;
     (void)args;
     Sampler::get().stop();
-    Py_RETURN_NONE
+    Py_RETURN_NONE;
 }
 
 static PyObject*

ddtrace/profiling/collector/memalloc.py

@@ -71,6 +71,9 @@ def _start_service(self):
         if _memalloc is None:
             raise collector.CollectorUnavailable
 
+        if self._export_libdd_enabled and not ddup.is_available:
+            self._export_libdd_enabled = False
+
         try:
             _memalloc.start(self.max_nframe, self._max_events, self.heap_sample_size)
         except RuntimeError:

ddtrace/profiling/collector/stack.pyx

@@ -488,20 +488,23 @@ class StackCollector(collector.PeriodicCollector):
         # 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 from stack.pyx; falling back to the legacy collector")
+                # We don't report on this, since it's already been reported in profiler.py
                 set_use_libdd(False)
+
+                # If the user had also set stack.v2.enabled, then we need to disable that as well.
+                if self._stack_collector_v2_enabled:
+                    self._stack_collector_v2_enabled = False
+                    LOG.error("Stack v2 was requested, but the libdd collector could not be enabled.  Falling back to the v1 stack sampler.")
             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")
+                LOG.error("Stack v2 was requested, but it could not be enabled.  Check debug logs for more information.")
             if not use_libdd:
                 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:

ddtrace/profiling/scheduler.py

@@ -33,6 +33,8 @@ def __attrs_post_init__(self):
     def _start_service(self):
         # type: (...) -> None
         """Start the scheduler."""
+        if self._export_libdd_enabled and not ddup.is_available:
+            self._export_libdd_enabled = False
         LOG.debug("Starting scheduler")
         super(Scheduler, self)._start_service()
         self._last_export = compat.time_ns()

setup.py

@@ -8,6 +8,8 @@
 import sysconfig
 import tarfile
 
+import cmake
+
 
 from setuptools import Extension, find_packages, setup  # isort: skip
 from setuptools.command.build_ext import build_ext  # isort: skip
@@ -38,6 +40,9 @@
 
 DEBUG_COMPILE = "DD_COMPILE_DEBUG" in os.environ
 
+# stack_v2 profiling extensions are optional, unless they are made explicitly required by this environment variable
+STACK_V2_REQUIRED = "DD_STACK_V2_REQUIRED" in os.environ
+
 IS_PYSTON = hasattr(sys, "pyston_version_info")
 
 LIBDDWAF_DOWNLOAD_DIR = HERE / "ddtrace" / "appsec" / "_ddwaf" / "libddwaf"
@@ -338,7 +343,9 @@ def build_extension_cmake(self, ext):
                     "-DCMAKE_OSX_ARCHITECTURES={}".format(";".join(archs)),
                 ]
 
-        cmake_command = os.environ.get("CMAKE_COMMAND", "cmake")
+        cmake_command = (
+            Path(cmake.CMAKE_BIN_DIR) / "cmake"
+        ).resolve()  # explicitly use the cmake provided by the cmake package
         subprocess.run([cmake_command, *cmake_args], cwd=cmake_build_dir, check=True)
         subprocess.run([cmake_command, "--build", ".", *build_args], cwd=cmake_build_dir, check=True)
         subprocess.run([cmake_command, "--install", ".", *install_args], cwd=cmake_build_dir, check=True)
@@ -465,6 +472,7 @@ def get_exts_for(name):
                     "-DPY_MINOR_VERSION={}".format(sys.version_info.minor),
                     "-DPY_MICRO_VERSION={}".format(sys.version_info.micro),
                 ],
+                optional=not STACK_V2_REQUIRED,
             )
         )
 
@@ -474,7 +482,8 @@ def get_exts_for(name):
                 CMakeExtension(
                     "ddtrace.internal.datadog.profiling.stack_v2._stack_v2",
                     source_dir=STACK_V2_DIR,
-                )
+                    optional=not STACK_V2_REQUIRED,
+                ),
             )
 
 else: