From c174f365a555280be4a9610958dec0e275a1ec5e Mon Sep 17 00:00:00 2001 From: Tony Xiao Date: Tue, 28 Jan 2025 11:49:01 -0800 Subject: [PATCH 1/5] feat(profiling): Continuous profiling sample rate This introduces a new top level setting for the continuous profiling session sample rate. The sample rate is evaluated once at the beginning and is used to determine whether or not the profiler will be run for the remainder of the process. --- sentry_sdk/consts.py | 1 + sentry_sdk/profiler/continuous_profiler.py | 70 ++++++++++------ sentry_sdk/scope.py | 4 +- tests/profiler/test_continuous_profiler.py | 95 ++++++++++++++++++---- 4 files changed, 130 insertions(+), 40 deletions(-) diff --git a/sentry_sdk/consts.py b/sentry_sdk/consts.py index 23f79ebd63..ce435de36b 100644 --- a/sentry_sdk/consts.py +++ b/sentry_sdk/consts.py @@ -528,6 +528,7 @@ def __init__( profiles_sample_rate=None, # type: Optional[float] profiles_sampler=None, # type: Optional[TracesSampler] profiler_mode=None, # type: Optional[ProfilerMode] + profile_session_sample_rate=None, # type: Optional[float] auto_enabling_integrations=True, # type: bool disabled_integrations=None, # type: Optional[Sequence[sentry_sdk.integrations.Integration]] auto_session_tracking=True, # type: bool diff --git a/sentry_sdk/profiler/continuous_profiler.py b/sentry_sdk/profiler/continuous_profiler.py index 5d64896b93..e3395a90ae 100644 --- a/sentry_sdk/profiler/continuous_profiler.py +++ b/sentry_sdk/profiler/continuous_profiler.py @@ -1,5 +1,6 @@ import atexit import os +import random import sys import threading import time @@ -83,11 +84,15 @@ def setup_continuous_profiler(options, sdk_info, capture_func): else: default_profiler_mode = ThreadContinuousScheduler.mode - experiments = options.get("_experiments", {}) + if options.get("profiler_mode") is not None: + profiler_mode = options["profiler_mode"] + else: + # TODO: deprecate this and just use the existing `profiler_mode` + experiments = options.get("_experiments", {}) - profiler_mode = ( - experiments.get("continuous_profiling_mode") or default_profiler_mode - ) + profiler_mode = ( + experiments.get("continuous_profiling_mode") or default_profiler_mode + ) frequency = DEFAULT_SAMPLING_FREQUENCY @@ -113,21 +118,15 @@ def setup_continuous_profiler(options, sdk_info, capture_func): return True -def try_autostart_continuous_profiler(): +def try_continuous_profiling_auto_start(): # type: () -> None if _scheduler is None: return - # Ensure that the scheduler only autostarts once per process. - # This is necessary because many web servers use forks to spawn - # additional processes. And the profiler is only spawned on the - # master process, then it often only profiles the main process - # and not the ones where the requests are being handled. - # # Additionally, we only want this autostart behaviour once per # process. If the user explicitly calls `stop_profiler`, it should # be respected and not start the profiler again. - if not _scheduler.should_autostart(): + if not _scheduler.is_auto_start_enabled(): return _scheduler.ensure_running() @@ -164,6 +163,16 @@ def get_profiler_id(): return _scheduler.profiler_id +def determine_profile_session_sampling_decision(sample_rate): + # type: (Union[float, None]) -> bool + + # `None` is treated as `0.0` + if not sample_rate: + return False + + return random.random() < float(sample_rate) + + class ContinuousScheduler: mode = "unknown" # type: ContinuousProfilerMode @@ -175,15 +184,29 @@ def __init__(self, frequency, options, sdk_info, capture_func): self.capture_func = capture_func self.sampler = self.make_sampler() self.buffer = None # type: Optional[ProfileBuffer] + self.pid = None # type: Optional[int] self.running = False - def should_autostart(self): + profile_session_sample_rate = self.options.get("profile_session_sample_rate") + self.sampled = determine_profile_session_sampling_decision( + profile_session_sample_rate + ) + + def is_auto_start_enabled(self): # type: () -> bool experiments = self.options.get("_experiments") if not experiments: return False - return experiments.get("continuous_profiling_auto_start") + if not experiments.get("continuous_profiling_auto_start"): + return False + + # Ensure that the scheduler only autostarts once per process. + # This is necessary because many web servers use forks to spawn + # additional processes. And the profiler is only spawned on the + # master process, then it often only profiles the main process + # and not the ones where the requests are being handled. + return self.pid != os.getpid() def ensure_running(self): # type: () -> None @@ -277,15 +300,15 @@ def __init__(self, frequency, options, sdk_info, capture_func): super().__init__(frequency, options, sdk_info, capture_func) self.thread = None # type: Optional[threading.Thread] - self.pid = None # type: Optional[int] self.lock = threading.Lock() - def should_autostart(self): - # type: () -> bool - return super().should_autostart() and self.pid != os.getpid() - def ensure_running(self): # type: () -> None + + # if the current profile session is not sampled, ensure_running is noop + if not self.sampled: + return + pid = os.getpid() # is running on the right process @@ -356,17 +379,16 @@ def __init__(self, frequency, options, sdk_info, capture_func): super().__init__(frequency, options, sdk_info, capture_func) self.thread = None # type: Optional[_ThreadPool] - self.pid = None # type: Optional[int] self.lock = threading.Lock() - def should_autostart(self): - # type: () -> bool - return super().should_autostart() and self.pid != os.getpid() - def ensure_running(self): # type: () -> None pid = os.getpid() + # if the current profile session is not sampled, ensure_running is noop + if not self.sampled: + return + # is running on the right process if self.running and self.pid == pid: return diff --git a/sentry_sdk/scope.py b/sentry_sdk/scope.py index c22cdfb030..7ab1435d42 100644 --- a/sentry_sdk/scope.py +++ b/sentry_sdk/scope.py @@ -12,7 +12,7 @@ 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 try_continuous_profiling_auto_start from sentry_sdk.profiler.transaction_profiler import Profile from sentry_sdk.session import Session from sentry_sdk.tracing_utils import ( @@ -1022,7 +1022,7 @@ def start_transaction( if instrumenter != configuration_instrumenter: return NoOpSpan() - try_autostart_continuous_profiler() + try_continuous_profiling_auto_start() custom_sampling_context = custom_sampling_context or {} diff --git a/tests/profiler/test_continuous_profiler.py b/tests/profiler/test_continuous_profiler.py index 1b96f27036..cc867e6d78 100644 --- a/tests/profiler/test_continuous_profiler.py +++ b/tests/profiler/test_continuous_profiler.py @@ -23,13 +23,25 @@ requires_gevent = pytest.mark.skipif(gevent is None, reason="gevent not enabled") -def experimental_options(mode=None, auto_start=None): - return { - "_experiments": { - "continuous_profiling_auto_start": auto_start, - "continuous_profiling_mode": mode, +def get_client_options(use_top_level_profiler_mode): + def client_options(mode=None, auto_start=None, profile_session_sample_rate=1.0): + if use_top_level_profiler_mode: + return { + "profiler_mode": mode, + "profile_session_sample_rate": profile_session_sample_rate, + "_experiments": { + "continuous_profiling_auto_start": auto_start, + }, + } + return { + "profile_session_sample_rate": profile_session_sample_rate, + "_experiments": { + "continuous_profiling_auto_start": auto_start, + "continuous_profiling_mode": mode, + }, } - } + + return client_options mock_sdk_info = { @@ -42,7 +54,10 @@ def experimental_options(mode=None, auto_start=None): @pytest.mark.parametrize("mode", [pytest.param("foo")]) @pytest.mark.parametrize( "make_options", - [pytest.param(experimental_options, id="experiment")], + [ + pytest.param(get_client_options(True), id="non-experiment"), + pytest.param(get_client_options(False), id="experiment"), + ], ) def test_continuous_profiler_invalid_mode(mode, make_options, teardown_profiling): with pytest.raises(ValueError): @@ -62,7 +77,10 @@ def test_continuous_profiler_invalid_mode(mode, make_options, teardown_profiling ) @pytest.mark.parametrize( "make_options", - [pytest.param(experimental_options, id="experiment")], + [ + pytest.param(get_client_options(True), id="non-experiment"), + pytest.param(get_client_options(False), id="experiment"), + ], ) def test_continuous_profiler_valid_mode(mode, make_options, teardown_profiling): options = make_options(mode=mode) @@ -82,7 +100,10 @@ def test_continuous_profiler_valid_mode(mode, make_options, teardown_profiling): ) @pytest.mark.parametrize( "make_options", - [pytest.param(experimental_options, id="experiment")], + [ + pytest.param(get_client_options(True), id="non-experiment"), + pytest.param(get_client_options(False), id="experiment"), + ], ) def test_continuous_profiler_setup_twice(mode, make_options, teardown_profiling): options = make_options(mode=mode) @@ -178,7 +199,10 @@ def assert_single_transaction_without_profile_chunks(envelopes): ) @pytest.mark.parametrize( "make_options", - [pytest.param(experimental_options, id="experiment")], + [ + 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_manual_stop( @@ -191,7 +215,7 @@ def test_continuous_profiler_auto_start_and_manual_stop( options = make_options(mode=mode, auto_start=True) sentry_init( traces_sample_rate=1.0, - _experiments=options.get("_experiments", {}), + **options, ) envelopes = capture_envelopes() @@ -235,10 +259,13 @@ def test_continuous_profiler_auto_start_and_manual_stop( ) @pytest.mark.parametrize( "make_options", - [pytest.param(experimental_options, id="experiment")], + [ + 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_manual_start_and_stop( +def test_continuous_profiler_manual_start_and_stop_sampled( sentry_init, capture_envelopes, mode, @@ -248,7 +275,7 @@ def test_continuous_profiler_manual_start_and_stop( options = make_options(mode=mode) sentry_init( traces_sample_rate=1.0, - _experiments=options.get("_experiments", {}), + **options, ) envelopes = capture_envelopes() @@ -275,3 +302,43 @@ def test_continuous_profiler_manual_start_and_stop( time.sleep(0.05) assert_single_transaction_without_profile_chunks(envelopes) + + +@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"), + ], +) +def test_continuous_profiler_manual_start_and_stop_unsampled( + sentry_init, + capture_envelopes, + mode, + make_options, + teardown_profiling, +): + options = make_options(mode=mode, profile_session_sample_rate=0.0) + sentry_init( + traces_sample_rate=1.0, + **options, + ) + + envelopes = capture_envelopes() + + start_profiler() + + with sentry_sdk.start_transaction(name="profiling"): + with sentry_sdk.start_span(op="op"): + time.sleep(0.05) + + assert_single_transaction_without_profile_chunks(envelopes) + + stop_profiler() From f4f8601268e4519ac4cc5792b6dfbfcf4298a1a7 Mon Sep 17 00:00:00 2001 From: Tony Xiao Date: Wed, 29 Jan 2025 12:43:33 -0800 Subject: [PATCH 2/5] bump From ade644a9ca67fdd8c67c7061860a3d0ae6e0b713 Mon Sep 17 00:00:00 2001 From: Tony Xiao Date: Wed, 29 Jan 2025 13:21:38 -0800 Subject: [PATCH 3/5] some more cleanup for manual mode --- sentry_sdk/profiler/continuous_profiler.py | 29 ++++++++-------------- 1 file changed, 10 insertions(+), 19 deletions(-) diff --git a/sentry_sdk/profiler/continuous_profiler.py b/sentry_sdk/profiler/continuous_profiler.py index e3395a90ae..2f8dd048b8 100644 --- a/sentry_sdk/profiler/continuous_profiler.py +++ b/sentry_sdk/profiler/continuous_profiler.py @@ -129,7 +129,7 @@ def try_continuous_profiling_auto_start(): if not _scheduler.is_auto_start_enabled(): return - _scheduler.ensure_running() + _scheduler.manual_start() def start_profiler(): @@ -137,7 +137,7 @@ def start_profiler(): if _scheduler is None: return - _scheduler.ensure_running() + _scheduler.manual_start() def stop_profiler(): @@ -198,15 +198,15 @@ def is_auto_start_enabled(self): experiments = self.options.get("_experiments") if not experiments: return False - if not experiments.get("continuous_profiling_auto_start"): - return False - # Ensure that the scheduler only autostarts once per process. - # This is necessary because many web servers use forks to spawn - # additional processes. And the profiler is only spawned on the - # master process, then it often only profiles the main process - # and not the ones where the requests are being handled. - return self.pid != os.getpid() + return experiments.get("continuous_profiling_auto_start") + + def manual_start(self): + # type: () -> None + if not self.sampled: + return + + self.ensure_running() def ensure_running(self): # type: () -> None @@ -305,10 +305,6 @@ def __init__(self, frequency, options, sdk_info, capture_func): def ensure_running(self): # type: () -> None - # if the current profile session is not sampled, ensure_running is noop - if not self.sampled: - return - pid = os.getpid() # is running on the right process @@ -385,10 +381,6 @@ def ensure_running(self): # type: () -> None pid = os.getpid() - # if the current profile session is not sampled, ensure_running is noop - if not self.sampled: - return - # is running on the right process if self.running and self.pid == pid: return @@ -415,7 +407,6 @@ def ensure_running(self): # longer allows us to spawn a thread and we have to bail. self.running = False self.thread = None - return def teardown(self): # type: () -> None From 4f38a9a51ae8ee53b560d4082e1d62ab8cfc8dd3 Mon Sep 17 00:00:00 2001 From: Tony Xiao Date: Thu, 30 Jan 2025 16:36:28 -0800 Subject: [PATCH 4/5] fix tests --- sentry_sdk/profiler/continuous_profiler.py | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/sentry_sdk/profiler/continuous_profiler.py b/sentry_sdk/profiler/continuous_profiler.py index 2f8dd048b8..de561f6d43 100644 --- a/sentry_sdk/profiler/continuous_profiler.py +++ b/sentry_sdk/profiler/continuous_profiler.py @@ -123,9 +123,6 @@ def try_continuous_profiling_auto_start(): if _scheduler is None: return - # Additionally, we only want this autostart behaviour once per - # process. If the user explicitly calls `stop_profiler`, it should - # be respected and not start the profiler again. if not _scheduler.is_auto_start_enabled(): return @@ -145,7 +142,7 @@ def stop_profiler(): if _scheduler is None: return - _scheduler.teardown() + _scheduler.manual_stop() def teardown_continuous_profiler(): @@ -195,6 +192,15 @@ def __init__(self, frequency, options, sdk_info, capture_func): def is_auto_start_enabled(self): # type: () -> bool + + # Ensure that the scheduler only autostarts once per process. + # This is necessary because many web servers use forks to spawn + # additional processes. And the profiler is only spawned on the + # master process, then it often only profiles the main process + # and not the ones where the requests are being handled. + if self.pid == os.getpid(): + return False + experiments = self.options.get("_experiments") if not experiments: return False @@ -208,6 +214,10 @@ def manual_start(self): self.ensure_running() + def manual_stop(self): + # type: () -> None + self.teardown() + def ensure_running(self): # type: () -> None raise NotImplementedError From 79dc6c536bb004b437133ceff6480ab6ddcc03d3 Mon Sep 17 00:00:00 2001 From: Tony Xiao Date: Mon, 3 Feb 2025 18:08:29 -0500 Subject: [PATCH 5/5] undo name change --- sentry_sdk/profiler/continuous_profiler.py | 2 +- sentry_sdk/scope.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/sentry_sdk/profiler/continuous_profiler.py b/sentry_sdk/profiler/continuous_profiler.py index 3188365d77..b07fbec998 100644 --- a/sentry_sdk/profiler/continuous_profiler.py +++ b/sentry_sdk/profiler/continuous_profiler.py @@ -118,7 +118,7 @@ def setup_continuous_profiler(options, sdk_info, capture_func): return True -def try_continuous_profiling_auto_start(): +def try_autostart_continuous_profiler(): # type: () -> None if _scheduler is None: return diff --git a/sentry_sdk/scope.py b/sentry_sdk/scope.py index 7ab1435d42..c22cdfb030 100644 --- a/sentry_sdk/scope.py +++ b/sentry_sdk/scope.py @@ -12,7 +12,7 @@ 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_continuous_profiling_auto_start +from sentry_sdk.profiler.continuous_profiler import try_autostart_continuous_profiler from sentry_sdk.profiler.transaction_profiler import Profile from sentry_sdk.session import Session from sentry_sdk.tracing_utils import ( @@ -1022,7 +1022,7 @@ def start_transaction( if instrumenter != configuration_instrumenter: return NoOpSpan() - try_continuous_profiling_auto_start() + try_autostart_continuous_profiler() custom_sampling_context = custom_sampling_context or {}