Skip to content

Commit abd4ad1

Browse files
github-actions[bot]brettlangdonchristophe-papazian
authored
perf(rc): improve performance of subprocess service sharing [backport 3.9] (#13685)
Backport b686305 from #13651 to 3.9. This change will only write service names to the shared file queue only if we have not shared that service name yet. This improvement avoids locking and writing to disk every time we call Pin.onto which can happen a lot when database connections are cycled/recreated frequently. It also helps ensure that the same process doesn't continually write the same service name to the same file over and over again. I also updated the tests to use subprocesses/forking as is expected with it's behavior for RC. ## 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: Brett Langdon <[email protected]> Co-authored-by: Christophe Papazian <[email protected]>
1 parent 297032c commit abd4ad1

File tree

2 files changed

+80
-30
lines changed

2 files changed

+80
-30
lines changed

ddtrace/settings/_config.py

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -504,6 +504,7 @@ def __init__(self):
504504
self.version = _get_config("DD_VERSION", self.tags.get("version"))
505505
self._http_server = self._HTTPServerConfig()
506506

507+
self._extra_services_sent = set() # type: set[str]
507508
self._extra_services_queue = None
508509
if self._remote_config_enabled and not in_aws_lambda():
509510
# lazy load slow import
@@ -667,8 +668,12 @@ def __getattr__(self, name) -> Any:
667668
def _add_extra_service(self, service_name: str) -> None:
668669
if self._extra_services_queue is None:
669670
return
670-
if service_name != self.service:
671-
self._extra_services_queue.put(service_name)
671+
672+
if service_name == self.service or service_name in self._extra_services_sent:
673+
return
674+
675+
self._extra_services_queue.put(service_name)
676+
self._extra_services_sent.add(service_name)
672677

673678
def _get_extra_services(self):
674679
# type: () -> set[str]
Lines changed: 73 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -1,42 +1,87 @@
1-
import random
2-
import re
3-
import threading
4-
import time
1+
import os
2+
import sys
53

64
import pytest
75

6+
7+
@pytest.mark.skipif(sys.platform in ("win32", "cygwin"), reason="Fork not supported on Windows")
8+
def test_config_extra_service_names_fork(run_python_code_in_subprocess):
9+
code = """
10+
import ddtrace.auto
811
import ddtrace
912
13+
import re
14+
import os
15+
import sys
16+
import time
17+
18+
children = []
19+
for i in range(10):
20+
pid = os.fork()
21+
if pid == 0:
22+
# Child process
23+
ddtrace.config._add_extra_service(f"extra_service_{i}")
24+
time.sleep(0.1) # Ensure the child has time to save the service
25+
sys.exit(0)
26+
else:
27+
# Parent process
28+
children.append(pid)
29+
30+
for pid in children:
31+
os.waitpid(pid, 0)
32+
33+
extra_services = ddtrace.config._get_extra_services()
34+
extra_services.discard("sqlite") # coverage
35+
assert len(extra_services) == 10, extra_services
36+
assert all(re.match(r"extra_service_\\d+", service) for service in extra_services), extra_services
37+
"""
1038

11-
MAX_NAMES = 64
39+
env = os.environ.copy()
40+
env["DD_REMOTE_CONFIGURATION_ENABLED"] = "true"
41+
stdout, stderr, status, _ = run_python_code_in_subprocess(code, env=env)
42+
assert status == 0, (stdout, stderr, status)
1243

1344

14-
@pytest.mark.parametrize("nb_service", [2, 16, 64, 256])
15-
def test_service_name(nb_service):
16-
ddtrace.config._extra_services = set()
45+
def test_config_extra_service_names_duplicates(run_python_code_in_subprocess):
46+
code = """
47+
import ddtrace.auto
48+
import ddtrace
49+
import re
50+
import os
51+
import sys
52+
import time
53+
54+
for _ in range(10):
55+
ddtrace.config._add_extra_service("extra_service_1")
1756
18-
def write_in_subprocess(id_nb):
19-
time.sleep(random.random())
20-
ddtrace.config._add_extra_service(f"extra_service_{id_nb}")
57+
extra_services = ddtrace.config._get_extra_services()
58+
extra_services.discard("sqlite") # coverage
59+
assert extra_services == {"extra_service_1"}
60+
"""
2161

22-
default_remote_config_enabled = ddtrace.config._remote_config_enabled
23-
ddtrace.config._remote_config_enabled = True
24-
if ddtrace.config._extra_services_queue is None:
25-
import ddtrace.internal._file_queue as file_queue
62+
env = os.environ.copy()
63+
env["DD_REMOTE_CONFIGURATION_ENABLED"] = "true"
64+
stdout, stderr, status, _ = run_python_code_in_subprocess(code, env=env)
65+
assert status == 0, (stdout, stderr, status)
2666

27-
ddtrace.config._extra_services_queue = file_queue.File_Queue()
2867

29-
threads = [threading.Thread(target=write_in_subprocess, args=(i,)) for i in range(nb_service)]
30-
for thread in threads:
31-
thread.start()
32-
for thread in threads:
33-
thread.join()
68+
def test_config_extra_service_names_rc_disabled(run_python_code_in_subprocess):
69+
code = """
70+
import ddtrace.auto
71+
import ddtrace
72+
import re
73+
import os
74+
import sys
75+
import time
76+
77+
for _ in range(10):
78+
ddtrace.config._add_extra_service("extra_service_1")
3479
35-
extra_services = ddtrace.config._get_extra_services()
36-
assert len(extra_services) == min(nb_service, MAX_NAMES)
37-
assert all(re.match(r"extra_service_\d+", service) for service in extra_services)
80+
extra_services = ddtrace.config._get_extra_services()
81+
assert len(extra_services) == 0
82+
"""
3883

39-
ddtrace.config._remote_config_enabled = default_remote_config_enabled
40-
if not default_remote_config_enabled:
41-
ddtrace.config._extra_services_queue = None
42-
ddtrace.config._extra_services = set()
84+
env = os.environ.copy()
85+
env["DD_REMOTE_CONFIGURATION_ENABLED"] = "false"
86+
stdout, stderr, status, _ = run_python_code_in_subprocess(code, env=env)
87+
assert status == 0, (stdout, stderr, status)

0 commit comments

Comments
 (0)