Skip to content

Commit

Permalink
fix: don't skip heartbeats for forked processes [backport 2.17] (#11637)
Browse files Browse the repository at this point in the history
Backport 63a439a 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 <[email protected]>
  • Loading branch information
github-actions[bot] and juanjux authored Dec 10, 2024
1 parent 72efe18 commit 69cd420
Show file tree
Hide file tree
Showing 3 changed files with 9 additions and 19 deletions.
8 changes: 0 additions & 8 deletions ddtrace/internal/telemetry/writer.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down
Original file line number Diff line number Diff line change
@@ -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.
16 changes: 5 additions & 11 deletions tests/telemetry/test_telemetry.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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
Expand All @@ -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):
Expand Down

0 comments on commit 69cd420

Please sign in to comment.