From 69cd42098a6e7f7d08549bf23b7a669dc2483130 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Tue, 10 Dec 2024 12:54:53 +0100 Subject: [PATCH] fix: don't skip heartbeats for forked processes [backport 2.17] (#11637) Backport 63a439aa34d049a8a68397f9e25c39eb7d706a83 from #11575 to 2.17. ## Description We were skipping app-heartbeat messages for forked processes. The problem with this is that if a process doesn't sent a heartbeat in 60 minutes, the backend will forget its dependencies, which was causing the list to only be the updated ones for some clients, specifically with gunicorn. This makes forked processes also sent heartbeats with solved the issue in our tests. Will stay as draft PR until I can check with @brettlangdon the reason heartbeats were disabled for forks. ## 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) Co-authored-by: Juanjo Alvarez Martinez --- ddtrace/internal/telemetry/writer.py | 8 -------- ...nt-skip-fork-heartbeats-23040d3ddc072298.yaml | 4 ++++ tests/telemetry/test_telemetry.py | 16 +++++----------- 3 files changed, 9 insertions(+), 19 deletions(-) create mode 100644 releasenotes/notes/dont-skip-fork-heartbeats-23040d3ddc072298.yaml diff --git a/ddtrace/internal/telemetry/writer.py b/ddtrace/internal/telemetry/writer.py index 7fbbecf56ae..08171ce3b1a 100644 --- a/ddtrace/internal/telemetry/writer.py +++ b/ddtrace/internal/telemetry/writer.py @@ -364,14 +364,6 @@ def _app_started(self, register_app_shutdown=True): def _app_heartbeat_event(self): # type: () -> None - if self._forked: - # TODO: Enable app-heartbeat on forks - # Since we only send app-started events in the main process - # any forked processes won't be able to access the list of - # dependencies for this app, and therefore app-heartbeat won't - # add much value today. - return - self.add_event({}, "app-heartbeat") def _app_closing_event(self): diff --git a/releasenotes/notes/dont-skip-fork-heartbeats-23040d3ddc072298.yaml b/releasenotes/notes/dont-skip-fork-heartbeats-23040d3ddc072298.yaml new file mode 100644 index 00000000000..f69a367a98f --- /dev/null +++ b/releasenotes/notes/dont-skip-fork-heartbeats-23040d3ddc072298.yaml @@ -0,0 +1,4 @@ +--- +fixes: + - | + Ensure that Telemetry heartbeats are not skipped for forked processes, as doing so could result in the dependency list being lost over time. diff --git a/tests/telemetry/test_telemetry.py b/tests/telemetry/test_telemetry.py index 24dbaae45d0..d767090f6d2 100644 --- a/tests/telemetry/test_telemetry.py +++ b/tests/telemetry/test_telemetry.py @@ -66,7 +66,10 @@ def test_enable_fork(test_agent_session, run_python_code_in_subprocess): def test_enable_fork_heartbeat(test_agent_session, run_python_code_in_subprocess): - """assert app-heartbeat events are only sent in parent process when no other events are queued""" + """ + assert app-heartbeat events are also sent in forked processes since otherwise the dependency collection + would be lost in pre-fork models after one hour. + """ code = """ import warnings # This test logs the following warning in py3.12: @@ -76,11 +79,6 @@ def test_enable_fork_heartbeat(test_agent_session, run_python_code_in_subprocess import os import ddtrace # enables telemetry -from ddtrace.internal.runtime import get_runtime_id - -if os.fork() > 0: - # Print the parent process runtime id for validation - print(get_runtime_id()) # Heartbeat events are only sent if no other events are queued from ddtrace.internal.telemetry import telemetry_writer @@ -94,13 +92,9 @@ def test_enable_fork_heartbeat(test_agent_session, run_python_code_in_subprocess assert status == 0, stderr assert stderr == b"", stderr - runtime_id = stdout.strip().decode("utf-8") - # Allow test agent session to capture all heartbeat events app_heartbeats = test_agent_session.get_events("app-heartbeat", filter_heartbeats=False, subprocess=True) - assert len(app_heartbeats) > 0 - for hb in app_heartbeats: - assert hb["runtime_id"] == runtime_id + assert len(app_heartbeats) > 1 def test_heartbeat_interval_configuration(run_python_code_in_subprocess):