From 830e5d0b5bf8cb333b829f98ab8eb45970d9fc9a Mon Sep 17 00:00:00 2001 From: Tony Xiao Date: Thu, 6 Oct 2022 17:11:53 -0400 Subject: [PATCH 1/7] feat(profiling): Extract qualified name for each frame Currently, we use `code.co_name` for the frame name. This does not include the name of the class if it was a method. This tries to extract the qualified name for each frame where possible. - methods: *typically* have `self` as a positional argument and we can inspect it to extract the class name - class methods: *typically* have `cls` as a positional argument and we can inspect it to extract the class name - static methods: no obvious way of extract the class name --- sentry_sdk/profiler.py | 63 ++++++++++++++++++++++++++++++------------ 1 file changed, 45 insertions(+), 18 deletions(-) diff --git a/sentry_sdk/profiler.py b/sentry_sdk/profiler.py index 45ef706815..893e71c2e7 100644 --- a/sentry_sdk/profiler.py +++ b/sentry_sdk/profiler.py @@ -16,21 +16,20 @@ import platform import random import signal +import sys import threading import time -import sys import uuid - -from collections import deque +from collections import deque, namedtuple from contextlib import contextmanager import sentry_sdk from sentry_sdk._compat import PY33 - from sentry_sdk._types import MYPY from sentry_sdk.utils import nanosecond_time if MYPY: + from types import FrameType from typing import Any from typing import Deque from typing import Dict @@ -38,11 +37,10 @@ from typing import List from typing import Optional from typing import Sequence - from typing import Tuple import sentry_sdk.tracing - Frame = Any - FrameData = Tuple[str, str, int] + +FrameData = namedtuple("FrameData", ["name", "file", "line"]) _sample_buffer = None # type: Optional[_SampleBuffer] @@ -127,7 +125,7 @@ def _sample_stack(*args, **kwargs): def _extract_stack(frame): - # type: (Frame) -> Sequence[FrameData] + # type: (Optional[FrameType]) -> Sequence[FrameData] """ Extracts the stack starting the specified frame. The extracted stack assumes the specified frame is the top of the stack, and works back @@ -141,13 +139,10 @@ def _extract_stack(frame): while frame is not None: stack.append( - ( - # co_name only contains the frame name. - # If the frame was a class method, - # the class name will NOT be included. - frame.f_code.co_name, - frame.f_code.co_filename, - frame.f_code.co_firstlineno, + FrameData( + name=get_frame_name(frame), + file=frame.f_code.co_filename, + line=frame.f_lineno, ) ) frame = frame.f_back @@ -155,6 +150,38 @@ def _extract_stack(frame): return stack +def get_frame_name(frame): + # type: (FrameType) -> str + + # in 3.11+, there is a frame.f_code.co_qualname that + # we should consider using instead where possible + + # co_name only contains the frame name. If the frame was a method, + # the class name will NOT be included. + name = frame.f_code.co_name + + # if it was a method, we can get the class name by inspecting + # the f_locals for the `self` argument + try: + if "self" in frame.f_locals: + return "{}.{}".format(frame.f_locals["self"].__class__.__name__, name) + except AttributeError: + pass + + # if it was a class method, (decorated with `@classmethod`) + # we can get the class name by inspecting the f_locals for the `cls` argument + try: + if "cls" in frame.f_locals: + return "{}.{}".format(frame.f_locals["cls"].__name__, name) + except AttributeError: + pass + + # nothing we can do if it is a staticmethod (decorated with @staticmethod) + + # we've done all we can, time to give up and return what we have + return name + + class Profile(object): def __init__(self, transaction, hub=None): # type: (sentry_sdk.tracing.Transaction, Optional[sentry_sdk.Hub]) -> None @@ -289,9 +316,9 @@ def slice_profile(self, start_ns, stop_ns): frames[frame] = len(frames) frames_list.append( { - "name": frame[0], - "file": frame[1], - "line": frame[2], + "name": frame.name, + "file": frame.file, + "line": frame.line, } ) current_stack.append(frames[frame]) From 7c259deaf3587fe7e96f548d8d057f5109b22bc9 Mon Sep 17 00:00:00 2001 From: Tony Xiao Date: Fri, 7 Oct 2022 13:27:57 -0400 Subject: [PATCH 2/7] small optimization --- sentry_sdk/profiler.py | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/sentry_sdk/profiler.py b/sentry_sdk/profiler.py index 893e71c2e7..99bf30998d 100644 --- a/sentry_sdk/profiler.py +++ b/sentry_sdk/profiler.py @@ -138,16 +138,17 @@ def _extract_stack(frame): stack = deque(maxlen=MAX_STACK_DEPTH) # type: Deque[FrameData] while frame is not None: - stack.append( - FrameData( - name=get_frame_name(frame), - file=frame.f_code.co_filename, - line=frame.f_lineno, - ) - ) + stack.append(frame) frame = frame.f_back - return stack + return [ + FrameData( + name=get_frame_name(frame), + file=frame.f_code.co_filename, + line=frame.f_lineno, + ) + for frame in stack + ] def get_frame_name(frame): From 2f141c781c10226f44e1d4d2df9783d9e55d57bf Mon Sep 17 00:00:00 2001 From: Tony Xiao Date: Fri, 7 Oct 2022 13:30:33 -0400 Subject: [PATCH 3/7] fix type --- sentry_sdk/profiler.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sentry_sdk/profiler.py b/sentry_sdk/profiler.py index 99bf30998d..e595455123 100644 --- a/sentry_sdk/profiler.py +++ b/sentry_sdk/profiler.py @@ -135,7 +135,7 @@ def _extract_stack(frame): only the first `MAX_STACK_DEPTH` frames will be returned. """ - stack = deque(maxlen=MAX_STACK_DEPTH) # type: Deque[FrameData] + stack = deque(maxlen=MAX_STACK_DEPTH) # type: Deque[FrameType] while frame is not None: stack.append(frame) From 2289868fdf41f0f4890a3ccd3150cf529345dff6 Mon Sep 17 00:00:00 2001 From: Tony Xiao Date: Wed, 12 Oct 2022 15:10:09 -0400 Subject: [PATCH 4/7] add tests --- sentry_sdk/profiler.py | 6 +-- tests/test_profiler.py | 95 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 98 insertions(+), 3 deletions(-) create mode 100644 tests/test_profiler.py diff --git a/sentry_sdk/profiler.py b/sentry_sdk/profiler.py index e595455123..d4d3c25646 100644 --- a/sentry_sdk/profiler.py +++ b/sentry_sdk/profiler.py @@ -113,7 +113,7 @@ def _sample_stack(*args, **kwargs): ( nanosecond_time(), [ - (tid, _extract_stack(frame)) + (tid, extract_stack(frame)) for tid, frame in sys._current_frames().items() ], ) @@ -124,7 +124,7 @@ def _sample_stack(*args, **kwargs): MAX_STACK_DEPTH = 128 -def _extract_stack(frame): +def extract_stack(frame, max_stack_depth=MAX_STACK_DEPTH): # type: (Optional[FrameType]) -> Sequence[FrameData] """ Extracts the stack starting the specified frame. The extracted stack @@ -135,7 +135,7 @@ def _extract_stack(frame): only the first `MAX_STACK_DEPTH` frames will be returned. """ - stack = deque(maxlen=MAX_STACK_DEPTH) # type: Deque[FrameType] + stack = deque(maxlen=max_stack_depth) # type: Deque[FrameType] while frame is not None: stack.append(frame) diff --git a/tests/test_profiler.py b/tests/test_profiler.py new file mode 100644 index 0000000000..8d6d465bc4 --- /dev/null +++ b/tests/test_profiler.py @@ -0,0 +1,95 @@ +import inspect + +import pytest + +from sentry_sdk.profiler import extract_stack, get_frame_name + + +def get_frame(depth=1): + """ + This function is not exactly true to its name. Depending on + how it is called, the true depth of the stack can be deeper + than the argument implies. + """ + if depth <= 0: + raise ValueError("only positive integers allowed") + if depth > 1: + return get_frame(depth=depth - 1) + return inspect.currentframe() + + +class GetFrame: + def instance_method(self): + return inspect.currentframe() + + @classmethod + def class_method(cls): + return inspect.currentframe() + + @staticmethod + def static_method(): + return inspect.currentframe() + + +@pytest.mark.parametrize( + ("frame", "frame_name"), + [ + pytest.param( + get_frame(), + "get_frame", + id="function", + ), + pytest.param( + (lambda: inspect.currentframe())(), + "", + id="lambda", + ), + pytest.param( + GetFrame().instance_method(), + "GetFrame.instance_method", + id="instance_method", + ), + pytest.param( + GetFrame().class_method(), + "GetFrame.class_method", + id="class_method", + ), + pytest.param( + GetFrame().static_method(), + "GetFrame.static_method", + id="static_method", + marks=pytest.mark.skip(reason="unsupported"), + ), + ], +) +def test_get_frame_name(frame, frame_name): + assert get_frame_name(frame) == frame_name + + +@pytest.mark.parametrize( + ("depth", "max_stack_depth", "actual_depth"), + [ + pytest.param(1, 128, 1, id="less than"), + pytest.param(256, 128, 128, id="greater than"), + pytest.param(128, 128, 128, id="equals"), + ], +) +def test_extract_stack_with_max_depth(depth, max_stack_depth, actual_depth): + # introduce a lambda that we'll be looking for in the stack + frame = (lambda: get_frame(depth=depth))() + + # plus 1 because we introduced a lambda intentionally that we'll + # look for in the final stack to make sure its in the right position + base_stack_depth = len(inspect.stack()) + 1 + + # increase the max_depth by the `base_stack_depth` to account + # for the extra frames pytest will add + stack = extract_stack(frame, max_stack_depth + base_stack_depth) + assert len(stack) == base_stack_depth + actual_depth + + for i in range(actual_depth): + assert stack[i].name == "get_frame", i + + # index 0 contains the inner most frame on the stack, so the lamdba + # should be at index `actual_depth` + assert stack[actual_depth].name == "", actual_depth From 160b484a37a442a9b351e38e64f19b49bd46277a Mon Sep 17 00:00:00 2001 From: Tony Xiao Date: Wed, 12 Oct 2022 15:39:27 -0400 Subject: [PATCH 5/7] fix mypy errors --- sentry_sdk/profiler.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sentry_sdk/profiler.py b/sentry_sdk/profiler.py index d4d3c25646..05605e44b7 100644 --- a/sentry_sdk/profiler.py +++ b/sentry_sdk/profiler.py @@ -125,7 +125,7 @@ def _sample_stack(*args, **kwargs): def extract_stack(frame, max_stack_depth=MAX_STACK_DEPTH): - # type: (Optional[FrameType]) -> Sequence[FrameData] + # type: (Optional[FrameType], int) -> Sequence[FrameData] """ Extracts the stack starting the specified frame. The extracted stack assumes the specified frame is the top of the stack, and works back From a6a1f1bba7c54fd8440b77a5a186c01791f1ede2 Mon Sep 17 00:00:00 2001 From: Tony Xiao Date: Thu, 13 Oct 2022 10:43:52 -0400 Subject: [PATCH 6/7] move test --- tests/test_profiler.py | 58 +++++++++++++++++++++--------------------- 1 file changed, 29 insertions(+), 29 deletions(-) diff --git a/tests/test_profiler.py b/tests/test_profiler.py index 8c9edc156a..e1b668eeba 100644 --- a/tests/test_profiler.py +++ b/tests/test_profiler.py @@ -17,35 +17,6 @@ ) -@pytest.mark.parametrize( - ("depth", "max_stack_depth", "actual_depth"), - [ - pytest.param(1, 128, 1, id="less than"), - pytest.param(256, 128, 128, id="greater than"), - pytest.param(128, 128, 128, id="equals"), - ], -) -def test_extract_stack_with_max_depth(depth, max_stack_depth, actual_depth): - # introduce a lambda that we'll be looking for in the stack - frame = (lambda: get_frame(depth=depth))() - - # plus 1 because we introduced a lambda intentionally that we'll - # look for in the final stack to make sure its in the right position - base_stack_depth = len(inspect.stack()) + 1 - - # increase the max_depth by the `base_stack_depth` to account - # for the extra frames pytest will add - stack = extract_stack(frame, max_stack_depth + base_stack_depth) - assert len(stack) == base_stack_depth + actual_depth - - for i in range(actual_depth): - assert stack[i].name == "get_frame", i - - # index 0 contains the inner most frame on the stack, so the lamdba - # should be at index `actual_depth` - assert stack[actual_depth].name == "", actual_depth - - @minimum_python_33 def test_profiler_invalid_mode(teardown_profiling): with pytest.raises(ValueError): @@ -150,3 +121,32 @@ def static_method(): ) def test_get_frame_name(frame, frame_name): assert get_frame_name(frame) == frame_name + + +@pytest.mark.parametrize( + ("depth", "max_stack_depth", "actual_depth"), + [ + pytest.param(1, 128, 1, id="less than"), + pytest.param(256, 128, 128, id="greater than"), + pytest.param(128, 128, 128, id="equals"), + ], +) +def test_extract_stack_with_max_depth(depth, max_stack_depth, actual_depth): + # introduce a lambda that we'll be looking for in the stack + frame = (lambda: get_frame(depth=depth))() + + # plus 1 because we introduced a lambda intentionally that we'll + # look for in the final stack to make sure its in the right position + base_stack_depth = len(inspect.stack()) + 1 + + # increase the max_depth by the `base_stack_depth` to account + # for the extra frames pytest will add + stack = extract_stack(frame, max_stack_depth + base_stack_depth) + assert len(stack) == base_stack_depth + actual_depth + + for i in range(actual_depth): + assert stack[i].name == "get_frame", i + + # index 0 contains the inner most frame on the stack, so the lamdba + # should be at index `actual_depth` + assert stack[actual_depth].name == "", actual_depth From 4f2820b3a76e2824f189e7c79becc6cadb6d16a9 Mon Sep 17 00:00:00 2001 From: Tony Xiao Date: Thu, 13 Oct 2022 10:50:53 -0400 Subject: [PATCH 7/7] clean up tests --- tests/test_profiler.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/tests/test_profiler.py b/tests/test_profiler.py index e1b668eeba..5feae5cc11 100644 --- a/tests/test_profiler.py +++ b/tests/test_profiler.py @@ -21,7 +21,6 @@ def test_profiler_invalid_mode(teardown_profiling): with pytest.raises(ValueError): setup_profiler({"_experiments": {"profiler_mode": "magic"}}) - # make sure to clean up at the end of the test @unix_only @@ -53,9 +52,8 @@ def join(self, timeout=None): thread.start() thread.join() - # make sure to clean up at the end of the test - +@unix_only @pytest.mark.parametrize("mode", ["sleep", "event", "sigprof", "sigalrm"]) def test_profiler_valid_mode(mode, teardown_profiling): # should not raise any exceptions