From 1324b34a61b81a465656ccd7bcb14a0a20e77d51 Mon Sep 17 00:00:00 2001 From: David Sanchez <838104+sanchda@users.noreply.github.com> Date: Mon, 22 Jul 2024 17:29:47 -0700 Subject: [PATCH] chore(profiling): add interface for crashtracker tags (#9892) Adds an interface and a test for crashtracker tags 1. can set with crashtracer.add_tag(key, val) 2. can set with DD_CRASHTRACKER_TAGS in the normal way ## Checklist - [x] PR author has checked that all the criteria below are met - The PR description includes an overview of the change - The PR description articulates the motivation for the change - The change includes tests OR the PR description describes a testing strategy - The PR description notes risks associated with the change, if any - Newly-added code is easy to change - The change follows the [library release note guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html) - The change includes or references documentation updates if necessary - Backport labels are set (if [applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)) ## Reviewer Checklist - [x] Reviewer has checked that all the criteria below are met - Title is accurate - All changes are related to the pull request's stated goal - Avoids breaking [API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces) changes - Testing strategy adequately addresses listed risks - Newly-added code is easy to change - Release note makes sense to a user of the library - If necessary, author has acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment - Backport labels are set in a manner that is consistent with the [release branch maintenance policy](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting) --- ddtrace/internal/core/crashtracking.py | 9 ++ .../profiling/crashtracker/_crashtracker.pyi | 1 + .../profiling/crashtracker/_crashtracker.pyx | 10 ++ .../dd_wrapper/include/crashtracker.hpp | 4 + .../include/crashtracker_interface.hpp | 1 + .../profiling/dd_wrapper/src/crashtracker.cpp | 25 +++- .../dd_wrapper/src/crashtracker_interface.cpp | 6 + ddtrace/settings/crashtracker.py | 19 ++- .../crashtracker/test_crashtracker.py | 128 ++++++++++++++++++ 9 files changed, 197 insertions(+), 6 deletions(-) diff --git a/ddtrace/internal/core/crashtracking.py b/ddtrace/internal/core/crashtracking.py index a2d81deb32a..9e96341af04 100644 --- a/ddtrace/internal/core/crashtracking.py +++ b/ddtrace/internal/core/crashtracking.py @@ -19,6 +19,11 @@ def _update_runtime_id(runtime_id: str) -> None: crashtracker.set_runtime_id(runtime_id) +def add_tag(key: str, value: str) -> None: + if is_available: + crashtracker.set_tag(key, value) + + def start() -> bool: if not is_available: return False @@ -44,6 +49,10 @@ def start() -> bool: if crashtracker_config.stderr_filename: crashtracker.set_stderr_filename(crashtracker_config.stderr_filename) + # Add user tags + for key, value in crashtracker_config.tags.items(): + add_tag(key, value) + # Only start if it is enabled if crashtracker_config.enabled: return crashtracker.start() diff --git a/ddtrace/internal/datadog/profiling/crashtracker/_crashtracker.pyi b/ddtrace/internal/datadog/profiling/crashtracker/_crashtracker.pyi index f455456b45b..a8eea6a2348 100644 --- a/ddtrace/internal/datadog/profiling/crashtracker/_crashtracker.pyi +++ b/ddtrace/internal/datadog/profiling/crashtracker/_crashtracker.pyi @@ -17,5 +17,6 @@ def set_resolve_frames_full() -> None: ... def set_profiling_state_sampling(on: bool) -> None: ... def set_profiling_state_unwinding(on: bool) -> None: ... def set_profiling_state_serializing(on: bool) -> None: ... +def set_tag(key: StringType, value: StringType) -> None: ... def start() -> bool: ... def is_started() -> bool: ... diff --git a/ddtrace/internal/datadog/profiling/crashtracker/_crashtracker.pyx b/ddtrace/internal/datadog/profiling/crashtracker/_crashtracker.pyx index 0f1d5606686..b7b6fdb684d 100644 --- a/ddtrace/internal/datadog/profiling/crashtracker/_crashtracker.pyx +++ b/ddtrace/internal/datadog/profiling/crashtracker/_crashtracker.pyx @@ -33,6 +33,7 @@ cdef extern from "crashtracker_interface.hpp": void crashtracker_set_resolve_frames_full() void crashtracker_set_resolve_frames_safe() bint crashtracker_set_receiver_binary_path(string_view path) + void crashtracker_set_tag(string_view key, string_view value) void crashtracker_profiling_state_sampling_start() void crashtracker_profiling_state_sampling_stop() void crashtracker_profiling_state_unwinding_start() @@ -134,6 +135,15 @@ def set_profiling_state_serializing(on: bool) -> None: crashtracker_profiling_state_serializing_stop() +def set_tag(key: StringType, value: StringType) -> None: + key_bytes = ensure_binary_or_empty(key) + value_bytes = ensure_binary_or_empty(value) + crashtracker_set_tag( + string_view(key_bytes, len(key_bytes)), + string_view(value_bytes, len(value_bytes)) + ) + + def start() -> bool: # The file is "crashtracker_exe" in the same directory as the libdd_wrapper.so exe_dir = os.path.dirname(__file__) diff --git a/ddtrace/internal/datadog/profiling/dd_wrapper/include/crashtracker.hpp b/ddtrace/internal/datadog/profiling/dd_wrapper/include/crashtracker.hpp index ef141cd5ce4..a54b8a9c8ad 100644 --- a/ddtrace/internal/datadog/profiling/dd_wrapper/include/crashtracker.hpp +++ b/ddtrace/internal/datadog/profiling/dd_wrapper/include/crashtracker.hpp @@ -7,6 +7,7 @@ #include #include #include +#include namespace Datadog { @@ -46,6 +47,8 @@ class Crashtracker std::string library_version; std::string url; + std::unordered_map user_tags; + static constexpr std::string_view family{ g_language_name }; static constexpr std::string_view library_name{ g_library_name }; @@ -65,6 +68,7 @@ class Crashtracker void set_runtime_version(std::string_view _runtime_version); void set_library_version(std::string_view _library_version); void set_url(std::string_view _url); + void set_tag(std::string_view _key, std::string_view _value); void set_create_alt_stack(bool _create_alt_stack); void set_stderr_filename(std::string_view _stderr_filename); diff --git a/ddtrace/internal/datadog/profiling/dd_wrapper/include/crashtracker_interface.hpp b/ddtrace/internal/datadog/profiling/dd_wrapper/include/crashtracker_interface.hpp index 101e68f87d1..f3c747a2236 100644 --- a/ddtrace/internal/datadog/profiling/dd_wrapper/include/crashtracker_interface.hpp +++ b/ddtrace/internal/datadog/profiling/dd_wrapper/include/crashtracker_interface.hpp @@ -24,6 +24,7 @@ extern "C" void crashtracker_set_resolve_frames_full(); void crashtracker_set_resolve_frames_safe(); bool crashtracker_set_receiver_binary_path(std::string_view path); + void crashtracker_set_tag(std::string_view key, std::string_view value); void crashtracker_profiling_state_sampling_start(); void crashtracker_profiling_state_sampling_stop(); void crashtracker_profiling_state_unwinding_start(); diff --git a/ddtrace/internal/datadog/profiling/dd_wrapper/src/crashtracker.cpp b/ddtrace/internal/datadog/profiling/dd_wrapper/src/crashtracker.cpp index e82b1d737a7..d1925cb0d27 100644 --- a/ddtrace/internal/datadog/profiling/dd_wrapper/src/crashtracker.cpp +++ b/ddtrace/internal/datadog/profiling/dd_wrapper/src/crashtracker.cpp @@ -86,6 +86,16 @@ Datadog::Crashtracker::set_library_version(std::string_view _library_version) library_version = std::string(_library_version); } +void +Datadog::Crashtracker::set_tag(std::string_view key, std::string_view value) +{ + // Maybe this should be called "add tag," but the interface to the uploader is already called "set_tag" + // and we have another refactor incoming anyway, so let's just kick the can for now + if (!key.empty() && !value.empty()) { + user_tags[std::string(key)] = std::string(value); + } +} + bool Datadog::Crashtracker::set_receiver_binary_path(std::string_view _path) { @@ -147,10 +157,19 @@ Datadog::Crashtracker::get_tags() { ExportTagKey::library_version, library_version }, }; + // Add system tags std::string errmsg; // Populated, but unused - for (const auto& [tag, data] : tag_data) { - if (!data.empty()) { - add_tag(tags, tag, data, errmsg); // We don't have a good way of handling errors here + for (const auto& [key, value] : tag_data) { + // NB - keys here are members of an enum; `add_tag()` specialization below will stringify them + if (!value.empty()) { + add_tag(tags, key, value, errmsg); // We don't have a good way of handling errors here + } + } + + // Add user tags + for (const auto& [key, value] : user_tags) { + if (!key.empty() && !value.empty()) { + add_tag(tags, key, value, errmsg); } } diff --git a/ddtrace/internal/datadog/profiling/dd_wrapper/src/crashtracker_interface.cpp b/ddtrace/internal/datadog/profiling/dd_wrapper/src/crashtracker_interface.cpp index 677f128dd06..9c9e3788541 100644 --- a/ddtrace/internal/datadog/profiling/dd_wrapper/src/crashtracker_interface.cpp +++ b/ddtrace/internal/datadog/profiling/dd_wrapper/src/crashtracker_interface.cpp @@ -112,6 +112,12 @@ crashtracker_set_receiver_binary_path(std::string_view path) // cppcheck-suppres return crashtracker.set_receiver_binary_path(path); } +void +crashtracker_set_tag(std::string_view key, std::string_view value) // cppcheck-suppress unusedFunction +{ + crashtracker.set_tag(key, value); +} + // Store the old segfault handler (uses sigaction prototype) void (*old_sigsegv_handler)(int, siginfo_t*, void*) = nullptr; void (*old_sigbus_handler)(int, siginfo_t*, void*) = nullptr; diff --git a/ddtrace/settings/crashtracker.py b/ddtrace/settings/crashtracker.py index af574448f73..6637047193d 100644 --- a/ddtrace/settings/crashtracker.py +++ b/ddtrace/settings/crashtracker.py @@ -2,6 +2,8 @@ from envier import En +from ddtrace.internal.utils.formats import parse_tags_str + def _derive_stacktrace_resolver(config: "CrashtrackerConfig") -> t.Optional[str]: resolver = str(config._stacktrace_resolver or "") @@ -41,8 +43,8 @@ class CrashtrackerConfig(En): "debug_url", default=None, help_type="String", - help="Overrides the URL parameter set by the ddtrace library. This is for testing and debugging purposes" - " and is not generally useful for end-users.", + help="Overrides the URL parameter set by the ddtrace library. " + "This is generally useful only for dd-trace-py development.", ) stdout_filename = En.v( @@ -66,7 +68,8 @@ class CrashtrackerConfig(En): "alt_stack", default=False, help_type="Boolean", - help="Whether to use an alternate stack for the crashtracker. This is used for internal development.", + help="Whether to use an alternate stack for the crashtracker." + "This is generally useful only for dd-trace-py development.", ) _stacktrace_resolver = En.v( @@ -79,5 +82,15 @@ class CrashtrackerConfig(En): ) stacktrace_resolver = En.d(t.Optional[str], _derive_stacktrace_resolver) + tags = En.v( + dict, + "tags", + parser=parse_tags_str, + default={}, + help_type="Mapping", + help="Additional crashtracking tags. Must be a list in the ``key1:value,key2:value2`` format. " + "This is generally useful only for dd-trace-py development.", + ) + config = CrashtrackerConfig() diff --git a/tests/internal/crashtracker/test_crashtracker.py b/tests/internal/crashtracker/test_crashtracker.py index 4359c57e22a..8798da9301c 100644 --- a/tests/internal/crashtracker/test_crashtracker.py +++ b/tests/internal/crashtracker/test_crashtracker.py @@ -382,3 +382,131 @@ def test_crashtracker_auto_disabled(run_python_code_in_subprocess): # Wait for the connection, which should fail conn = utils.listen_get_conn(sock) assert not conn + + +@pytest.mark.skipif(not sys.platform.startswith("linux"), reason="Linux only") +def test_crashtracker_user_tags_envvar(run_python_code_in_subprocess): + # Setup the listening socket before we open ddtrace + port, sock = utils.crashtracker_receiver_bind() + assert sock + + # Call the program + env = os.environ.copy() + env["DD_TRACE_AGENT_URL"] = "http://localhost:%d" % port + + # Injecting tags, but since the way we validate them is with a raw-data string search, we make things unique + tag_prefix = "cryptocrystalline" + tags = { + tag_prefix + "_tag1": "quartz_flint", + tag_prefix + "_tag2": "quartz_chert", + } + env["DD_CRASHTRACKER_TAGS"] = ",".join(["%s:%s" % (k, v) for k, v in tags.items()]) + stdout, stderr, exitcode, _ = run_python_code_in_subprocess(auto_code, env=env) + + # Check for expected exit condition + assert not stdout + assert not stderr + assert exitcode == -11 + + # Wait for the connection + conn = utils.listen_get_conn(sock) + assert conn + data = utils.conn_to_bytes(conn) + assert data + + # Now check for the tags + for k, v in tags.items(): + assert k.encode() in data + assert v.encode() in data + + +@pytest.mark.skipif(not sys.platform.startswith("linux"), reason="Linux only") +@pytest.mark.subprocess() +def test_crashtracker_user_tags_profiling(): + # Tests tag ingestion in the backend API (which is currently out of profiling) + import ctypes + import os + + import ddtrace.internal.datadog.profiling.crashtracker as crashtracker + import tests.internal.crashtracker.utils as utils + + # Define some tags + tag_prefix = "manganese_oxides" + tags = { + tag_prefix + "_tag1": "pyrolusite", + tag_prefix + "_tag2": "birnessite", + } + + port, sock = utils.crashtracker_receiver_bind() + assert port + assert sock + + pid = os.fork() + if pid == 0: + # Set the tags before starting + for k, v in tags.items(): + crashtracker.set_tag(k, v) + assert utils.start_crashtracker(port) + stdout_msg, stderr_msg = utils.read_files(["stdout.log", "stderr.log"]) + assert not stdout_msg + assert not stderr_msg + + ctypes.string_at(0) + exit(-1) + + conn = utils.listen_get_conn(sock) + assert conn + data = utils.conn_to_bytes(conn) + conn.close() + assert b"string_at" in data + + # Now check for the tags + for k, v in tags.items(): + assert k.encode() in data + assert v.encode() in data + + +@pytest.mark.skipif(not sys.platform.startswith("linux"), reason="Linux only") +@pytest.mark.subprocess() +def test_crashtracker_user_tags_core(): + # Tests tag ingestion in the core API + import ctypes + import os + + from ddtrace.internal.core import crashtracking + import tests.internal.crashtracker.utils as utils + + # Define some tags + tag_prefix = "manganese_oxides" + tags = { + tag_prefix + "_tag1": "pyrolusite", + tag_prefix + "_tag2": "birnessite", + } + + port, sock = utils.crashtracker_receiver_bind() + assert port + assert sock + + pid = os.fork() + if pid == 0: + # Set the tags before starting + for k, v in tags.items(): + crashtracking.add_tag(k, v) + assert utils.start_crashtracker(port) + stdout_msg, stderr_msg = utils.read_files(["stdout.log", "stderr.log"]) + assert not stdout_msg + assert not stderr_msg + + ctypes.string_at(0) + exit(-1) + + conn = utils.listen_get_conn(sock) + assert conn + data = utils.conn_to_bytes(conn) + conn.close() + assert b"string_at" in data + + # Now check for the tags + for k, v in tags.items(): + assert k.encode() in data + assert v.encode() in data