Skip to content

Commit 2904bbb

Browse files
authored
fix(profiling): ensure correct thread link [backport #6798 to 1.17] (#6816)
Backport of #6798 to 1.17 We use the C API to retrieve the thread ID of the current thread to create a link between threads and objects. This way we protect ourselves from frameworks such as gevent that might turn thread IDs into task IDs. ## Testing strategy Tested with a sample application that the endpoint information is attached to profiles with this change. ## 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.
1 parent 57fc217 commit 2904bbb

File tree

3 files changed

+29
-16
lines changed

3 files changed

+29
-16
lines changed

ddtrace/profiling/_threading.pyx

Lines changed: 21 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -10,9 +10,20 @@ from six.moves import _thread
1010
from ddtrace import _threading as ddtrace_threading
1111

1212

13-
IF UNAME_SYSNAME == "Linux":
14-
from cpython cimport PyLong_FromLong
13+
from cpython cimport PyLong_FromLong
14+
15+
16+
cdef extern from "<Python.h>":
17+
# This one is provided as an opaque struct from Cython's
18+
# cpython/pystate.pxd, but we need to access some of its fields so we
19+
# redefine it here.
20+
ctypedef struct PyThreadState:
21+
unsigned long thread_id
1522

23+
PyThreadState* PyThreadState_Get()
24+
25+
26+
IF UNAME_SYSNAME == "Linux":
1627
from ddtrace.internal.module import ModuleWatchdog
1728
from ddtrace.internal.wrapping import wrap
1829

@@ -108,28 +119,22 @@ class _ThreadLink(_thread_link_base):
108119
):
109120
# type: (...) -> None
110121
"""Link an object to the current running thread."""
111-
self._thread_id_to_object[_thread.get_ident()] = weakref.ref(obj)
122+
# Because threads might become tasks with some frameworks (e.g. gevent),
123+
# we retrieve the thread ID using the C API instead of the Python API.
124+
self._thread_id_to_object[<object>PyLong_FromLong(PyThreadState_Get().thread_id)] = weakref.ref(obj)
112125

113126
def clear_threads(self,
114127
existing_thread_ids, # type: typing.Set[int]
115128
):
116-
"""Clear the stored list of threads based on the list of existing thread ids.
129+
"""Clean up the thread linking map.
117130
118-
If any thread that is part of this list was stored, its data will be deleted.
131+
We remove all threads that are not in the existing thread IDs.
119132
120133
:param existing_thread_ids: A set of thread ids to keep.
121134
"""
122-
# This code clears the thread/object mapping by clearing a copy and swapping it in an atomic operation This is
123-
# needed to be able to have this whole class lock-free and avoid concurrency issues.
124-
# The fact that it is lock free means we might lose some accuracy, but it's worth the trade-off for speed and simplicity.
125-
new_thread_id_to_object_mapping = self._thread_id_to_object.copy()
126-
# Iterate over a copy of the list of keys since it's mutated during our iteration.
127-
for thread_id in list(new_thread_id_to_object_mapping):
128-
if thread_id not in existing_thread_ids:
129-
del new_thread_id_to_object_mapping[thread_id]
130-
131-
# Swap with the new list
132-
self._thread_id_to_object = new_thread_id_to_object_mapping
135+
self._thread_id_to_object = {
136+
k: v for k, v in self._thread_id_to_object.items() if k in existing_thread_ids
137+
}
133138

134139
def get_object(
135140
self,

docs/spelling_wordlist.txt

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -250,3 +250,5 @@ programmatically
250250
DES
251251
Blowfish
252252
Gitlab
253+
hotspot
254+
CMake
Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
---
2+
fixes:
3+
- |
4+
profiling: fixed a bug that prevented profiles from being correctly
5+
correlated to traces in gevent-based applications, thus causing code hotspot
6+
and end point data to be missing from the UI.

0 commit comments

Comments
 (0)