feat(profiling): Introduce continuous profiling mode #2830
Conversation
This is a new profiling mode that is mutually exclusive from the existing profiling modes. In the current profiling modes, a profile is always directly attached to a transaction. This new mode will continuously emit chunks of profiling data that will be connected to the span data.
As per getsentry/rfc#75, this adds the thread data to the spans. This will be needed for the continuous profiling mode in #2830.
As per getsentry/rfc#75, this adds the thread data to the spans. This will be needed for the continuous profiling mode in #2830.
…o/feat/introduce-continuous-profiling-mode
| @@ -0,0 +1,543 @@ | |||
| import atexit | |||
There was a problem hiding this comment.
Can we put this file in a new profiler folder together with the original profiler.py just to have it a bit more organized? We can export the user-facing API like stop_profiler in profiler/__init__.py.
| else: | ||
| try: | ||
| setup_continuous_profiler( | ||
| self.options, | ||
| capture_func=_capture_envelope, | ||
| ) | ||
| except Exception as e: | ||
| logger.debug("Can not set up continuous profiler. (%s)", e) |
There was a problem hiding this comment.
So the continuous profiler is always enabled unless the user wants to use the old profiler, and there is no way to turn all profilers off if I read this correctly -- does this have any consequences for the user (e.g. quota)?
There was a problem hiding this comment.
Ah ok, the user has to explicitly provide _experiments["continuous_profiling_auto_start"] for the profiler to kick off right? Then disregard.
| ProfileContext = TypedDict( | ||
| "ProfileContext", | ||
| {"profile_id": str}, | ||
| {"profiler_id": str}, | ||
| ) |
There was a problem hiding this comment.
Is ProfileContext even used anywhere? Can't find any occurrences
| import threading | ||
| import time | ||
| from collections import defaultdict | ||
| from unittest import mock # python 3.3 and above |
There was a problem hiding this comment.
We only support 3.5+ so no need for the comment
| from unittest import mock # python 3.3 and above | |
| from unittest import mock |
| experiments.get("continuous_profiling_mode") or default_profiler_mode | ||
| ) | ||
|
|
||
| frequency = DEFAULT_SAMPLING_FREQUENCY |
There was a problem hiding this comment.
Would it make sense to make this configurable?
| self.thread = None | ||
| return | ||
|
|
||
| def run(self): |
There was a problem hiding this comment.
Unless I missed anything, run seems to be the same for both the thread- and gevent-based profiler, is there anything speaking against putting it in the parent ContinuousProfiler class?
| if self.buffer is not None: | ||
| self.buffer.flush() | ||
|
|
||
| def teardown(self): |
There was a problem hiding this comment.
Same comment as above for run
| @@ -0,0 +1,237 @@ | |||
| import threading | |||
There was a problem hiding this comment.
If you decide to go with the new profiler folder in the sdk that I suggested above, it'd make sense to do the same with test_continuous_profiler.py and test_profiler.py in the tests.
This is a new profiling mode that is mutually exclusive from the existing profiling modes. In the current profiling modes, a profile is always directly attached to a transaction. This new mode will continuously emit chunks of profiling data that will be connected to the span data.