fix(dsm): only register exit signal handler if data streams monitoring is active [backport 2.2] (#7624)

Backport fd1c9e1 from #7578 to 2.2.

This pull request fixes
#7273 by only registering
exit signal handlers when Datastreams Monitoring is active. This works
because DSM is the only code path that currently uses these handlers.

The test verifies that the Flask app does not hang when it receives two
`SIGINT`s, which is the closest behavior to the original issue report
that I was able to replicate in the test suite.

There's also a change to a test that verifies the behavior of
`register_on_exit_signal`. Using `override_global_config` to activate
DSM causes the assertions in the test to change. I decided that changing
the assertions was more likely to cause errors than directly calling
`register_on_exit_signal` in the test.

Fixes #7273
Fixes #7014

## 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] Risk is outlined (performance impact, potential for breakage,
maintainability, etc).
- [x] Change is maintainable (easy to change, telemetry, documentation).
- [x] [Library release note
guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html)
are followed. If no release note is required, add label
`changelog/no-changelog`.
- [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))

## Reviewer Checklist

- [x] Title is accurate.
- [x] No unnecessary changes are introduced.
- [x] Description motivates each change.
- [x] Avoids breaking
[API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces)
changes unless absolutely necessary.
- [x] Testing strategy adequately addresses listed risk(s).
- [x] Change is maintainable (easy to change, telemetry, documentation).
- [x] Release note makes sense to a user of the library.
- [x] Reviewer has explicitly 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)
- [x] If this PR 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`.
- [x] This PR doesn't touch any of that.

Co-authored-by: Emmett Butler <[email protected]>
Co-authored-by: Munir Abdinur <[email protected]>

Author: 3 people

ddtrace/tracer.py

@@ -263,11 +263,11 @@ def __init__(
             from .internal.datastreams.processor import DataStreamsProcessor
 
             self.data_streams_processor = DataStreamsProcessor(self._agent_url)
+            register_on_exit_signal(self._atexit)
 
         self._hooks = _hooks.Hooks()
         atexit.register(self._atexit)
         forksafe.register(self._child_after_fork)
-        register_on_exit_signal(self._atexit)
 
         self._shutdown_lock = RLock()
 

releasenotes/notes/on-exit-conditional-7bc86389271f3847.yaml

@@ -0,0 +1,5 @@
+---
+fixes:
+  - |
+    tracing: This fix resolves an issue where ddtrace's signal handlers could cause Flask apps not to respond
+    correctly to SIGINT.

tests/contrib/flask/test_request.py

@@ -1,6 +1,10 @@
 # -*- coding: utf-8 -*-
 import json
 import os
+import signal
+import subprocess
+import sys
+import time
 
 import flask
 from flask import abort
@@ -23,6 +27,7 @@
 
 
 REMOVED_SPANS_2_2_0 = 1 if flask_version >= (2, 2, 0) else 0
+SIGTERM_EXIT_CODE = -15
 
 
 base_exception_name = "builtins.Exception"
@@ -1166,3 +1171,30 @@ def index():
     out, err, status, pid = ddtrace_run_python_code_in_subprocess(code, env=env)
     assert status == 0, (out, err)
     assert err == b"", (out, err)
+
+
+def test_sigint(tmpdir):
+    code = """
+from flask import Flask
+app = Flask(__name__)
+
+@app.route('/')
+def hello_world():
+    return 'Hello, World!'
+
+if __name__ == '__main__':
+    app.run(port=8082)
+    """
+    pyfile = tmpdir.join("test.py")
+    pyfile.write(code)
+    subp = subprocess.Popen(
+        ["ddtrace-run", sys.executable, str(pyfile)],
+        stdout=subprocess.PIPE,
+        stderr=subprocess.PIPE,
+        close_fds=sys.platform != "win32",
+    )
+    time.sleep(0.5)
+    # send two terminate signals to forcibly kill the process
+    subp.terminate()
+    subp.terminate()
+    assert subp.wait() == SIGTERM_EXIT_CODE, "An instrumented Flask app should respond to SIGINT by exiting"

tests/integration/test_integration.py

@@ -9,6 +9,7 @@
 import six
 
 from ddtrace import Tracer
+from ddtrace.internal.atexit import register_on_exit_signal
 from ddtrace.internal.writer import AgentWriter
 from tests.integration.utils import AGENT_VERSION
 from tests.integration.utils import BadEncoder
@@ -39,6 +40,7 @@ def test_configure_keeps_api_hostname_and_port():
 def test_shutdown_on_exit_signal(mock_get_signal, mock_signal):
     mock_get_signal.return_value = None
     tracer = Tracer()
+    register_on_exit_signal(tracer._atexit)
     assert mock_signal.call_count == 2
     assert mock_signal.call_args_list[0][0][0] == signal.SIGTERM
     assert mock_signal.call_args_list[1][0][0] == signal.SIGINT