From 1db0411afe0b4acb712fc489ea24cfc9490bb233 Mon Sep 17 00:00:00 2001 From: Tony Xiao Date: Tue, 4 Feb 2025 11:51:13 -0500 Subject: [PATCH 1/4] feat(profiling): Continuous profiling lifecycle This introduces auto lifecycle setting for continuous profiling to only profile while there is an active transaction. This replaces the experimental auto start setting. --- sentry_sdk/consts.py | 2 + sentry_sdk/profiler/continuous_profiler.py | 159 +++++++++++++++++---- sentry_sdk/scope.py | 14 +- sentry_sdk/tracing.py | 10 +- tests/profiler/test_continuous_profiler.py | 111 +++++++++++++- 5 files changed, 262 insertions(+), 34 deletions(-) diff --git a/sentry_sdk/consts.py b/sentry_sdk/consts.py index ce435de36b..d9ba23438c 100644 --- a/sentry_sdk/consts.py +++ b/sentry_sdk/consts.py @@ -38,6 +38,7 @@ class CompressionAlgo(Enum): from typing import Any from typing import Sequence from typing import Tuple + from typing_extensions import Literal from typing_extensions import TypedDict from sentry_sdk._types import ( @@ -528,6 +529,7 @@ def __init__( profiles_sample_rate=None, # type: Optional[float] profiles_sampler=None, # type: Optional[TracesSampler] profiler_mode=None, # type: Optional[ProfilerMode] + profile_lifecycle="manual", # type: Literal["manual", "auto"] profile_session_sample_rate=None, # type: Optional[float] auto_enabling_integrations=True, # type: bool disabled_integrations=None, # type: Optional[Sequence[sentry_sdk.integrations.Integration]] diff --git a/sentry_sdk/profiler/continuous_profiler.py b/sentry_sdk/profiler/continuous_profiler.py index b07fbec998..7abddfe2a1 100644 --- a/sentry_sdk/profiler/continuous_profiler.py +++ b/sentry_sdk/profiler/continuous_profiler.py @@ -5,6 +5,7 @@ import threading import time import uuid +from collections import deque from datetime import datetime, timezone from sentry_sdk.consts import VERSION @@ -27,6 +28,7 @@ if TYPE_CHECKING: from typing import Any from typing import Callable + from typing import Deque from typing import Dict from typing import List from typing import Optional @@ -120,6 +122,9 @@ def setup_continuous_profiler(options, sdk_info, capture_func): def try_autostart_continuous_profiler(): # type: () -> None + + # TODO: deprecate this as it'll be replaced by the auto lifecycle option + if _scheduler is None: return @@ -129,6 +134,22 @@ def try_autostart_continuous_profiler(): _scheduler.manual_start() +def try_profile_lifecycle_auto_start(): + # type: () -> bool + if _scheduler is None: + return False + + return _scheduler.auto_start() + + +def try_profile_lifecycle_auto_stop(): + # type: () -> None + if _scheduler is None: + return + + _scheduler.auto_stop() + + def start_profiler(): # type: () -> None if _scheduler is None: @@ -179,16 +200,22 @@ def __init__(self, frequency, options, sdk_info, capture_func): self.options = options self.sdk_info = sdk_info self.capture_func = capture_func + + self.lifecycle = self.options.get("profile_lifecycle") + profile_session_sample_rate = self.options.get("profile_session_sample_rate") + self.sampled = determine_profile_session_sampling_decision( + profile_session_sample_rate + ) + self.sampler = self.make_sampler() self.buffer = None # type: Optional[ProfileBuffer] self.pid = None # type: Optional[int] self.running = False - profile_session_sample_rate = self.options.get("profile_session_sample_rate") - self.sampled = determine_profile_session_sampling_decision( - profile_session_sample_rate - ) + self.active_spans = 0 + self.started_spans = deque(maxlen=128) # type: Deque[None] + self.finished_spans = deque(maxlen=128) # type: Deque[None] def is_auto_start_enabled(self): # type: () -> bool @@ -207,15 +234,45 @@ def is_auto_start_enabled(self): return experiments.get("continuous_profiling_auto_start") + def auto_start(self): + # type: () -> bool + if not self.sampled: + return False + + if self.lifecycle != "auto": + return False + + logger.debug("[Profiling] Auto starting profiler") + + self.started_spans.append(None) + self.ensure_running() + + return True + + def auto_stop(self): + # type: () -> None + if self.lifecycle != "auto": + return + + logger.debug("[Profiling] Auto stopping profiler") + + self.finished_spans.append(None) + def manual_start(self): # type: () -> None if not self.sampled: return + if self.lifecycle != "manual": + return + self.ensure_running() def manual_stop(self): # type: () -> None + if self.lifecycle != "manual": + return + self.teardown() def ensure_running(self): @@ -249,28 +306,77 @@ def make_sampler(self): cache = LRUCache(max_size=256) - def _sample_stack(*args, **kwargs): - # type: (*Any, **Any) -> None - """ - Take a sample of the stack on all the threads in the process. - This should be called at a regular interval to collect samples. - """ - - ts = now() - - try: - sample = [ - (str(tid), extract_stack(frame, cache, cwd)) - for tid, frame in sys._current_frames().items() - ] - except AttributeError: - # For some reason, the frame we get doesn't have certain attributes. - # When this happens, we abandon the current sample as it's bad. - capture_internal_exception(sys.exc_info()) - return - - if self.buffer is not None: - self.buffer.write(ts, sample) + if self.lifecycle == "auto": + + def _sample_stack(*args, **kwargs): + # type: (*Any, **Any) -> None + """ + Take a sample of the stack on all the threads in the process. + This should be called at a regular interval to collect samples. + """ + + if ( + not self.active_spans + and not self.started_spans + and not self.finished_spans + ): + self.running = False + return + + started_spans = len(self.started_spans) + finished_spans = len(self.finished_spans) + + ts = now() + + try: + sample = [ + (str(tid), extract_stack(frame, cache, cwd)) + for tid, frame in sys._current_frames().items() + ] + except AttributeError: + # For some reason, the frame we get doesn't have certain attributes. + # When this happens, we abandon the current sample as it's bad. + capture_internal_exception(sys.exc_info()) + return + + for _ in range(started_spans): + self.started_spans.popleft() + + for _ in range(finished_spans): + self.finished_spans.popleft() + + self.active_spans = self.active_spans + started_spans - finished_spans + + if self.buffer is None: + self.reset_buffer() + + if self.buffer is not None: + self.buffer.write(ts, sample) + + else: + + def _sample_stack(*args, **kwargs): + # type: (*Any, **Any) -> None + """ + Take a sample of the stack on all the threads in the process. + This should be called at a regular interval to collect samples. + """ + + ts = now() + + try: + sample = [ + (str(tid), extract_stack(frame, cache, cwd)) + for tid, frame in sys._current_frames().items() + ] + except AttributeError: + # For some reason, the frame we get doesn't have certain attributes. + # When this happens, we abandon the current sample as it's bad. + capture_internal_exception(sys.exc_info()) + return + + if self.buffer is not None: + self.buffer.write(ts, sample) return _sample_stack @@ -294,6 +400,7 @@ def run(self): if self.buffer is not None: self.buffer.flush() + self.buffer = None class ThreadContinuousScheduler(ContinuousScheduler): diff --git a/sentry_sdk/scope.py b/sentry_sdk/scope.py index c22cdfb030..9887ad0239 100644 --- a/sentry_sdk/scope.py +++ b/sentry_sdk/scope.py @@ -12,7 +12,11 @@ from sentry_sdk.attachments import Attachment from sentry_sdk.consts import DEFAULT_MAX_BREADCRUMBS, FALSE_VALUES, INSTRUMENTER from sentry_sdk.feature_flags import FlagBuffer, DEFAULT_FLAG_CAPACITY -from sentry_sdk.profiler.continuous_profiler import try_autostart_continuous_profiler +from sentry_sdk.profiler.continuous_profiler import ( + get_profiler_id, + try_autostart_continuous_profiler, + try_profile_lifecycle_auto_start, +) from sentry_sdk.profiler.transaction_profiler import Profile from sentry_sdk.session import Session from sentry_sdk.tracing_utils import ( @@ -1051,6 +1055,14 @@ def start_transaction( transaction._profile = profile + transaction._started_profile_lifecycle = try_profile_lifecycle_auto_start() + + # Typically, the profiler is set when the transaction is created. But when + # using the auto lifecycle, the profiler isn't running when the first + # transaction is started. So make sure we update the profiler id on it. + if transaction._started_profile_lifecycle: + transaction.set_profiler_id(get_profiler_id()) + # we don't bother to keep spans if we already know we're not going to # send the transaction max_spans = (client.options["_experiments"].get("max_spans")) or 1000 diff --git a/sentry_sdk/tracing.py b/sentry_sdk/tracing.py index 86456b8964..6073e7e98f 100644 --- a/sentry_sdk/tracing.py +++ b/sentry_sdk/tracing.py @@ -5,7 +5,10 @@ import sentry_sdk from sentry_sdk.consts import INSTRUMENTER, SPANSTATUS, SPANDATA -from sentry_sdk.profiler.continuous_profiler import get_profiler_id +from sentry_sdk.profiler.continuous_profiler import ( + get_profiler_id, + try_profile_lifecycle_auto_stop, +) from sentry_sdk.utils import ( get_current_thread_meta, is_valid_sample_rate, @@ -268,6 +271,7 @@ class Span: "scope", "origin", "name", + "_started_profile_lifecycle", ) def __init__( @@ -790,6 +794,7 @@ def __init__( # type: ignore[misc] self._profile = ( None ) # type: Optional[sentry_sdk.profiler.transaction_profiler.Profile] + self._started_profile_lifecycle = False # type: bool self._baggage = baggage def __repr__(self): @@ -842,6 +847,9 @@ def __exit__(self, ty, value, tb): if self._profile is not None: self._profile.__exit__(ty, value, tb) + if self._started_profile_lifecycle: + try_profile_lifecycle_auto_stop() + super().__exit__(ty, value, tb) @property diff --git a/tests/profiler/test_continuous_profiler.py b/tests/profiler/test_continuous_profiler.py index 6f4893e59d..e4aef29a2a 100644 --- a/tests/profiler/test_continuous_profiler.py +++ b/tests/profiler/test_continuous_profiler.py @@ -8,6 +8,7 @@ import sentry_sdk from sentry_sdk.consts import VERSION from sentry_sdk.profiler.continuous_profiler import ( + get_profiler_id, setup_continuous_profiler, start_profiler, stop_profiler, @@ -24,9 +25,12 @@ def get_client_options(use_top_level_profiler_mode): - def client_options(mode=None, auto_start=None, profile_session_sample_rate=1.0): + def client_options( + mode=None, auto_start=None, profile_session_sample_rate=1.0, lifecycle="manual" + ): if use_top_level_profiler_mode: return { + "profile_lifecycle": lifecycle, "profiler_mode": mode, "profile_session_sample_rate": profile_session_sample_rate, "_experiments": { @@ -34,6 +38,7 @@ def client_options(mode=None, auto_start=None, profile_session_sample_rate=1.0): }, } return { + "profile_lifecycle": lifecycle, "profile_session_sample_rate": profile_session_sample_rate, "_experiments": { "continuous_profiling_auto_start": auto_start, @@ -224,7 +229,7 @@ def test_continuous_profiler_auto_start_and_manual_stop( with sentry_sdk.start_transaction(name="profiling"): with sentry_sdk.start_span(op="op"): - time.sleep(0.1) + time.sleep(0.05) assert_single_transaction_with_profile_chunks(envelopes, thread) @@ -235,7 +240,7 @@ def test_continuous_profiler_auto_start_and_manual_stop( with sentry_sdk.start_transaction(name="profiling"): with sentry_sdk.start_span(op="op"): - time.sleep(0.1) + time.sleep(0.05) assert_single_transaction_without_profile_chunks(envelopes) @@ -245,7 +250,7 @@ def test_continuous_profiler_auto_start_and_manual_stop( with sentry_sdk.start_transaction(name="profiling"): with sentry_sdk.start_span(op="op"): - time.sleep(0.1) + time.sleep(0.05) assert_single_transaction_with_profile_chunks(envelopes, thread) @@ -272,7 +277,9 @@ def test_continuous_profiler_manual_start_and_stop_sampled( make_options, teardown_profiling, ): - options = make_options(mode=mode) + options = make_options( + mode=mode, profile_session_sample_rate=1.0, lifecycle="manual" + ) sentry_init( traces_sample_rate=1.0, **options, @@ -325,7 +332,9 @@ def test_continuous_profiler_manual_start_and_stop_unsampled( make_options, teardown_profiling, ): - options = make_options(mode=mode, profile_session_sample_rate=0.0) + options = make_options( + mode=mode, profile_session_sample_rate=0.0, lifecycle="manual" + ) sentry_init( traces_sample_rate=1.0, **options, @@ -342,3 +351,93 @@ def test_continuous_profiler_manual_start_and_stop_unsampled( assert_single_transaction_without_profile_chunks(envelopes) stop_profiler() + + +@pytest.mark.parametrize( + "mode", + [ + pytest.param("thread"), + pytest.param("gevent", marks=requires_gevent), + ], +) +@pytest.mark.parametrize( + "make_options", + [ + pytest.param(get_client_options(True), id="non-experiment"), + pytest.param(get_client_options(False), id="experiment"), + ], +) +@mock.patch("sentry_sdk.profiler.continuous_profiler.PROFILE_BUFFER_SECONDS", 0.01) +def test_continuous_profiler_auto_start_and_stop_sampled( + sentry_init, + capture_envelopes, + mode, + make_options, + teardown_profiling, +): + options = make_options(mode=mode, profile_session_sample_rate=1.0, lifecycle="auto") + sentry_init( + traces_sample_rate=1.0, + **options, + ) + + envelopes = capture_envelopes() + + thread = threading.current_thread() + + for _ in range(3): + envelopes.clear() + + with sentry_sdk.start_transaction(name="profiling"): + assert get_profiler_id() is not None, "profiler should be running" + with sentry_sdk.start_span(op="op"): + time.sleep(0.05) + assert get_profiler_id() is not None, "profiler should be running" + + # wait at least 1 cycle for the profiler to stop + time.sleep(0.05) + assert get_profiler_id() is None, "profiler should not be running" + assert_single_transaction_with_profile_chunks(envelopes, thread) + + +@pytest.mark.parametrize( + "mode", + [ + pytest.param("thread"), + pytest.param("gevent", marks=requires_gevent), + ], +) +@pytest.mark.parametrize( + "make_options", + [ + pytest.param(get_client_options(True), id="non-experiment"), + pytest.param(get_client_options(False), id="experiment"), + ], +) +@mock.patch("sentry_sdk.profiler.continuous_profiler.PROFILE_BUFFER_SECONDS", 0.01) +def test_continuous_profiler_auto_start_and_stop_unsampled( + sentry_init, + capture_envelopes, + mode, + make_options, + teardown_profiling, +): + options = make_options(mode=mode, profile_session_sample_rate=0.0, lifecycle="auto") + sentry_init( + traces_sample_rate=1.0, + **options, + ) + + envelopes = capture_envelopes() + + for _ in range(3): + envelopes.clear() + + with sentry_sdk.start_transaction(name="profiling"): + assert get_profiler_id() is None, "profiler should not be running" + with sentry_sdk.start_span(op="op"): + time.sleep(0.05) + assert get_profiler_id() is None, "profiler should not be running" + + assert get_profiler_id() is None, "profiler should not be running" + assert_single_transaction_without_profile_chunks(envelopes) From f21fb9350cddbc4b48a9cb8a8efc82b5afb4fc08 Mon Sep 17 00:00:00 2001 From: Tony Xiao Date: Tue, 11 Feb 2025 11:51:47 -0500 Subject: [PATCH 2/4] use old logic for tracking transactions --- sentry_sdk/consts.py | 2 +- sentry_sdk/profiler/continuous_profiler.py | 98 +++++++++++++-------- sentry_sdk/profiler/transaction_profiler.py | 2 +- sentry_sdk/scope.py | 6 +- sentry_sdk/tracing.py | 20 ++--- tests/profiler/test_continuous_profiler.py | 40 ++++++--- 6 files changed, 104 insertions(+), 64 deletions(-) diff --git a/sentry_sdk/consts.py b/sentry_sdk/consts.py index d9ba23438c..8660086feb 100644 --- a/sentry_sdk/consts.py +++ b/sentry_sdk/consts.py @@ -529,7 +529,7 @@ def __init__( profiles_sample_rate=None, # type: Optional[float] profiles_sampler=None, # type: Optional[TracesSampler] profiler_mode=None, # type: Optional[ProfilerMode] - profile_lifecycle="manual", # type: Literal["manual", "auto"] + profile_lifecycle="manual", # type: Literal["manual", "trace"] profile_session_sample_rate=None, # type: Optional[float] auto_enabling_integrations=True, # type: bool disabled_integrations=None, # type: Optional[Sequence[sentry_sdk.integrations.Integration]] diff --git a/sentry_sdk/profiler/continuous_profiler.py b/sentry_sdk/profiler/continuous_profiler.py index 7abddfe2a1..398d2e03a5 100644 --- a/sentry_sdk/profiler/continuous_profiler.py +++ b/sentry_sdk/profiler/continuous_profiler.py @@ -32,6 +32,7 @@ from typing import Dict from typing import List from typing import Optional + from typing import Set from typing import Type from typing import Union from typing_extensions import TypedDict @@ -134,15 +135,15 @@ def try_autostart_continuous_profiler(): _scheduler.manual_start() -def try_profile_lifecycle_auto_start(): - # type: () -> bool +def try_profile_lifecycle_trace_start(): + # type: () -> Union[ContinuousProfile, None] if _scheduler is None: - return False + return None return _scheduler.auto_start() -def try_profile_lifecycle_auto_stop(): +def try_profile_lifecycle_trace_stop(): # type: () -> None if _scheduler is None: return @@ -191,6 +192,14 @@ def determine_profile_session_sampling_decision(sample_rate): return random.random() < float(sample_rate) +class ContinuousProfile: + active: bool = True + + def stop(self): + # type: () -> None + self.active = False + + class ContinuousScheduler: mode = "unknown" # type: ContinuousProfilerMode @@ -213,9 +222,8 @@ def __init__(self, frequency, options, sdk_info, capture_func): self.running = False - self.active_spans = 0 - self.started_spans = deque(maxlen=128) # type: Deque[None] - self.finished_spans = deque(maxlen=128) # type: Deque[None] + self.new_profiles = deque(maxlen=128) # type: Deque[ContinuousProfile] + self.active_profiles = set() # type: Set[ContinuousProfile] def is_auto_start_enabled(self): # type: () -> bool @@ -235,28 +243,21 @@ def is_auto_start_enabled(self): return experiments.get("continuous_profiling_auto_start") def auto_start(self): - # type: () -> bool + # type: () -> Union[ContinuousProfile, None] if not self.sampled: - return False + return None - if self.lifecycle != "auto": - return False + if self.lifecycle != "trace": + return None logger.debug("[Profiling] Auto starting profiler") - self.started_spans.append(None) - self.ensure_running() - - return True + profile = ContinuousProfile() - def auto_stop(self): - # type: () -> None - if self.lifecycle != "auto": - return - - logger.debug("[Profiling] Auto stopping profiler") + self.new_profiles.append(profile) + self.ensure_running() - self.finished_spans.append(None) + return profile def manual_start(self): # type: () -> None @@ -306,7 +307,7 @@ def make_sampler(self): cache = LRUCache(max_size=256) - if self.lifecycle == "auto": + if self.lifecycle == "trace": def _sample_stack(*args, **kwargs): # type: (*Any, **Any) -> None @@ -315,16 +316,20 @@ def _sample_stack(*args, **kwargs): This should be called at a regular interval to collect samples. """ - if ( - not self.active_spans - and not self.started_spans - and not self.finished_spans - ): + # no profiles taking place, so we can stop early + if not self.new_profiles and not self.active_profiles: self.running = False return - started_spans = len(self.started_spans) - finished_spans = len(self.finished_spans) + # This is the number of profiles we want to pop off. + # It's possible another thread adds a new profile to + # the list and we spend longer than we want inside + # the loop below. + # + # Also make sure to set this value before extracting + # frames so we do not write to any new profiles that + # were started after this point. + new_profiles = len(self.new_profiles) ts = now() @@ -339,13 +344,32 @@ def _sample_stack(*args, **kwargs): capture_internal_exception(sys.exc_info()) return - for _ in range(started_spans): - self.started_spans.popleft() - - for _ in range(finished_spans): - self.finished_spans.popleft() - - self.active_spans = self.active_spans + started_spans - finished_spans + # Move the new profiles into the active_profiles set. + # + # We cannot directly add the to active_profiles set + # in `start_profiling` because it is called from other + # threads which can cause a RuntimeError when it the + # set sizes changes during iteration without a lock. + # + # We also want to avoid using a lock here so threads + # that are starting profiles are not blocked until it + # can acquire the lock. + for _ in range(new_profiles): + self.active_profiles.add(self.new_profiles.popleft()) + inactive_profiles = [] + + for profile in self.active_profiles: + if profile.active: + pass + else: + # If a profile is marked inactive, we buffer it + # to `inactive_profiles` so it can be removed. + # We cannot remove it here as it would result + # in a RuntimeError. + inactive_profiles.append(profile) + + for profile in inactive_profiles: + self.active_profiles.remove(profile) if self.buffer is None: self.reset_buffer() diff --git a/sentry_sdk/profiler/transaction_profiler.py b/sentry_sdk/profiler/transaction_profiler.py index f579c441fa..3743b7c905 100644 --- a/sentry_sdk/profiler/transaction_profiler.py +++ b/sentry_sdk/profiler/transaction_profiler.py @@ -644,7 +644,7 @@ def _sample_stack(*args, **kwargs): if profile.active: profile.write(now, sample) else: - # If a thread is marked inactive, we buffer it + # If a profile is marked inactive, we buffer it # to `inactive_profiles` so it can be removed. # We cannot remove it here as it would result # in a RuntimeError. diff --git a/sentry_sdk/scope.py b/sentry_sdk/scope.py index 9887ad0239..9af9769ee5 100644 --- a/sentry_sdk/scope.py +++ b/sentry_sdk/scope.py @@ -15,7 +15,7 @@ from sentry_sdk.profiler.continuous_profiler import ( get_profiler_id, try_autostart_continuous_profiler, - try_profile_lifecycle_auto_start, + try_profile_lifecycle_trace_start, ) from sentry_sdk.profiler.transaction_profiler import Profile from sentry_sdk.session import Session @@ -1055,12 +1055,12 @@ def start_transaction( transaction._profile = profile - transaction._started_profile_lifecycle = try_profile_lifecycle_auto_start() + transaction._continuous_profile = try_profile_lifecycle_trace_start() # Typically, the profiler is set when the transaction is created. But when # using the auto lifecycle, the profiler isn't running when the first # transaction is started. So make sure we update the profiler id on it. - if transaction._started_profile_lifecycle: + if transaction._continuous_profile is not None: transaction.set_profiler_id(get_profiler_id()) # we don't bother to keep spans if we already know we're not going to diff --git a/sentry_sdk/tracing.py b/sentry_sdk/tracing.py index 016a8f5746..b7dc437506 100644 --- a/sentry_sdk/tracing.py +++ b/sentry_sdk/tracing.py @@ -5,10 +5,7 @@ import sentry_sdk from sentry_sdk.consts import INSTRUMENTER, SPANSTATUS, SPANDATA -from sentry_sdk.profiler.continuous_profiler import ( - get_profiler_id, - try_profile_lifecycle_auto_stop, -) +from sentry_sdk.profiler.continuous_profiler import get_profiler_id from sentry_sdk.utils import ( get_current_thread_meta, is_valid_sample_rate, @@ -37,7 +34,8 @@ P = ParamSpec("P") R = TypeVar("R") - import sentry_sdk.profiler + from sentry_sdk.profiler.continuous_profiler import ContinuousProfile + from sentry_sdk.profiler.transaction_profiler import Profile from sentry_sdk._types import ( Event, MeasurementUnit, @@ -272,7 +270,6 @@ class Span: "scope", "origin", "name", - "_started_profile_lifecycle", ) def __init__( @@ -771,6 +768,7 @@ class Transaction(Span): "_measurements", "_contexts", "_profile", + "_continuous_profile", "_baggage", ) @@ -792,10 +790,8 @@ def __init__( # type: ignore[misc] self.parent_sampled = parent_sampled self._measurements = {} # type: Dict[str, MeasurementValue] self._contexts = {} # type: Dict[str, Any] - self._profile = ( - None - ) # type: Optional[sentry_sdk.profiler.transaction_profiler.Profile] - self._started_profile_lifecycle = False # type: bool + self._profile = None # type: Optional[Profile] + self._continuous_profile = None # type: Optional[ContinuousProfile] self._baggage = baggage def __repr__(self): @@ -848,8 +844,8 @@ def __exit__(self, ty, value, tb): if self._profile is not None: self._profile.__exit__(ty, value, tb) - if self._started_profile_lifecycle: - try_profile_lifecycle_auto_stop() + if self._continuous_profile is not None: + self._continuous_profile.stop() super().__exit__(ty, value, tb) diff --git a/tests/profiler/test_continuous_profiler.py b/tests/profiler/test_continuous_profiler.py index e4aef29a2a..1dabc080ff 100644 --- a/tests/profiler/test_continuous_profiler.py +++ b/tests/profiler/test_continuous_profiler.py @@ -126,14 +126,17 @@ def test_continuous_profiler_setup_twice(mode, make_options, teardown_profiling) ) -def assert_single_transaction_with_profile_chunks(envelopes, thread): +def assert_single_transaction_with_profile_chunks( + envelopes, thread, max_chunks, transactions=1 +): items = defaultdict(list) for envelope in envelopes: for item in envelope.items: items[item.type].append(item) - assert len(items["transaction"]) == 1 + assert len(items["transaction"]) == transactions assert len(items["profile_chunk"]) > 0 + assert len(items["profile_chunk"]) <= max_chunks transaction = items["transaction"][0].payload.json @@ -231,7 +234,7 @@ def test_continuous_profiler_auto_start_and_manual_stop( with sentry_sdk.start_span(op="op"): time.sleep(0.05) - assert_single_transaction_with_profile_chunks(envelopes, thread) + assert_single_transaction_with_profile_chunks(envelopes, thread, max_chunks=10) for _ in range(3): stop_profiler() @@ -252,7 +255,7 @@ def test_continuous_profiler_auto_start_and_manual_stop( with sentry_sdk.start_span(op="op"): time.sleep(0.05) - assert_single_transaction_with_profile_chunks(envelopes, thread) + assert_single_transaction_with_profile_chunks(envelopes, thread, max_chunks=10) @pytest.mark.parametrize( @@ -298,7 +301,7 @@ def test_continuous_profiler_manual_start_and_stop_sampled( with sentry_sdk.start_span(op="op"): time.sleep(0.05) - assert_single_transaction_with_profile_chunks(envelopes, thread) + assert_single_transaction_with_profile_chunks(envelopes, thread, max_chunks=10) stop_profiler() @@ -367,7 +370,7 @@ def test_continuous_profiler_manual_start_and_stop_unsampled( pytest.param(get_client_options(False), id="experiment"), ], ) -@mock.patch("sentry_sdk.profiler.continuous_profiler.PROFILE_BUFFER_SECONDS", 0.01) +@mock.patch("sentry_sdk.profiler.continuous_profiler.PROFILE_BUFFER_SECONDS", 0.1) def test_continuous_profiler_auto_start_and_stop_sampled( sentry_init, capture_envelopes, @@ -375,7 +378,9 @@ def test_continuous_profiler_auto_start_and_stop_sampled( make_options, teardown_profiling, ): - options = make_options(mode=mode, profile_session_sample_rate=1.0, lifecycle="auto") + options = make_options( + mode=mode, profile_session_sample_rate=1.0, lifecycle="trace" + ) sentry_init( traces_sample_rate=1.0, **options, @@ -391,13 +396,26 @@ def test_continuous_profiler_auto_start_and_stop_sampled( with sentry_sdk.start_transaction(name="profiling"): assert get_profiler_id() is not None, "profiler should be running" with sentry_sdk.start_span(op="op"): - time.sleep(0.05) + time.sleep(0.03) + assert get_profiler_id() is not None, "profiler should be running" + + # the profiler takes a while to stop so if we start a transaction + # immediately, it'll be part of the same chunk + assert get_profiler_id() is not None, "profiler should be running" + + with sentry_sdk.start_transaction(name="profiling"): + assert get_profiler_id() is not None, "profiler should be running" + with sentry_sdk.start_span(op="op"): + time.sleep(0.03) assert get_profiler_id() is not None, "profiler should be running" # wait at least 1 cycle for the profiler to stop time.sleep(0.05) assert get_profiler_id() is None, "profiler should not be running" - assert_single_transaction_with_profile_chunks(envelopes, thread) + + assert_single_transaction_with_profile_chunks( + envelopes, thread, max_chunks=1, transactions=2 + ) @pytest.mark.parametrize( @@ -422,7 +440,9 @@ def test_continuous_profiler_auto_start_and_stop_unsampled( make_options, teardown_profiling, ): - options = make_options(mode=mode, profile_session_sample_rate=0.0, lifecycle="auto") + options = make_options( + mode=mode, profile_session_sample_rate=0.0, lifecycle="trace" + ) sentry_init( traces_sample_rate=1.0, **options, From e0f72c03f3e8fe79d8dbcd3fee708d9545ecb7b1 Mon Sep 17 00:00:00 2001 From: Tony Xiao Date: Tue, 11 Feb 2025 12:38:59 -0500 Subject: [PATCH 3/4] remove unneeded code + more tests --- sentry_sdk/profiler/continuous_profiler.py | 11 ------ tests/profiler/test_continuous_profiler.py | 46 ++++++++++++++++++++++ 2 files changed, 46 insertions(+), 11 deletions(-) diff --git a/sentry_sdk/profiler/continuous_profiler.py b/sentry_sdk/profiler/continuous_profiler.py index 398d2e03a5..1619925bd2 100644 --- a/sentry_sdk/profiler/continuous_profiler.py +++ b/sentry_sdk/profiler/continuous_profiler.py @@ -143,14 +143,6 @@ def try_profile_lifecycle_trace_start(): return _scheduler.auto_start() -def try_profile_lifecycle_trace_stop(): - # type: () -> None - if _scheduler is None: - return - - _scheduler.auto_stop() - - def start_profiler(): # type: () -> None if _scheduler is None: @@ -371,9 +363,6 @@ def _sample_stack(*args, **kwargs): for profile in inactive_profiles: self.active_profiles.remove(profile) - if self.buffer is None: - self.reset_buffer() - if self.buffer is not None: self.buffer.write(ts, sample) diff --git a/tests/profiler/test_continuous_profiler.py b/tests/profiler/test_continuous_profiler.py index 1dabc080ff..902c7409a7 100644 --- a/tests/profiler/test_continuous_profiler.py +++ b/tests/profiler/test_continuous_profiler.py @@ -461,3 +461,49 @@ def test_continuous_profiler_auto_start_and_stop_unsampled( assert get_profiler_id() is None, "profiler should not be running" assert_single_transaction_without_profile_chunks(envelopes) + + +@pytest.mark.parametrize( + ["mode", "class_name"], + [ + pytest.param("thread", "ThreadContinuousScheduler"), + pytest.param( + "gevent", + "GeventContinuousScheduler", + marks=requires_gevent, + ), + ], +) +@pytest.mark.parametrize( + "make_options", + [ + pytest.param(get_client_options(True), id="non-experiment"), + pytest.param(get_client_options(False), id="experiment"), + ], +) +def test_continuous_profiler_manual_start_and_stop_noop_when_using_trace_lifecyle( + sentry_init, + mode, + class_name, + make_options, + teardown_profiling, +): + options = make_options( + mode=mode, profile_session_sample_rate=0.0, lifecycle="trace" + ) + sentry_init( + traces_sample_rate=1.0, + **options, + ) + + with mock.patch( + f"sentry_sdk.profiler.continuous_profiler.{class_name}.ensure_running" + ) as mock_ensure_running: + start_profiler() + mock_ensure_running.assert_not_called() + + with mock.patch( + f"sentry_sdk.profiler.continuous_profiler.{class_name}.teardown" + ) as mock_teardown: + stop_profiler() + mock_teardown.assert_not_called() From 5956a11c08efe2bf22467ac7a3e06fa3b1eab492 Mon Sep 17 00:00:00 2001 From: Tony Xiao Date: Tue, 11 Feb 2025 15:47:22 -0500 Subject: [PATCH 4/4] reduce test flake --- tests/profiler/test_continuous_profiler.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/tests/profiler/test_continuous_profiler.py b/tests/profiler/test_continuous_profiler.py index 902c7409a7..331080df83 100644 --- a/tests/profiler/test_continuous_profiler.py +++ b/tests/profiler/test_continuous_profiler.py @@ -171,6 +171,7 @@ def assert_single_transaction_with_profile_chunks( for profile_chunk_item in items["profile_chunk"]: profile_chunk = profile_chunk_item.payload.json + del profile_chunk["profile"] # make the diff easier to read assert profile_chunk == ApproxDict( { "client_sdk": { @@ -370,7 +371,7 @@ def test_continuous_profiler_manual_start_and_stop_unsampled( pytest.param(get_client_options(False), id="experiment"), ], ) -@mock.patch("sentry_sdk.profiler.continuous_profiler.PROFILE_BUFFER_SECONDS", 0.1) +@mock.patch("sentry_sdk.profiler.continuous_profiler.DEFAULT_SAMPLING_FREQUENCY", 21) def test_continuous_profiler_auto_start_and_stop_sampled( sentry_init, capture_envelopes, @@ -393,7 +394,7 @@ def test_continuous_profiler_auto_start_and_stop_sampled( for _ in range(3): envelopes.clear() - with sentry_sdk.start_transaction(name="profiling"): + with sentry_sdk.start_transaction(name="profiling 1"): assert get_profiler_id() is not None, "profiler should be running" with sentry_sdk.start_span(op="op"): time.sleep(0.03) @@ -403,14 +404,14 @@ def test_continuous_profiler_auto_start_and_stop_sampled( # immediately, it'll be part of the same chunk assert get_profiler_id() is not None, "profiler should be running" - with sentry_sdk.start_transaction(name="profiling"): + with sentry_sdk.start_transaction(name="profiling 2"): assert get_profiler_id() is not None, "profiler should be running" with sentry_sdk.start_span(op="op"): time.sleep(0.03) assert get_profiler_id() is not None, "profiler should be running" # wait at least 1 cycle for the profiler to stop - time.sleep(0.05) + time.sleep(0.2) assert get_profiler_id() is None, "profiler should not be running" assert_single_transaction_with_profile_chunks(