feat(node): add continuous profiling mode#12124
Conversation
size-limit report 📦
|
|
I feel like we could do a better job explaining this new mode. It is not clear to me from reading this PR description or the JSDoc when exactly the profiling starts and when it stops. |
|
Agree @lforst. We dont plan to expose this API yet and I'm going to write a doc on how exactly we want to do this so we can standardize it cross sdks. I'll share the doc with you so you can give us some input as well. The plan here is to just add the underlying functionality without exposing it to the users as the product doesnt even support it yet. |
AbhiPrasad
left a comment
There was a problem hiding this comment.
After this merges in we def also need an e2e test.
… or span terminates
… or span terminates
AbhiPrasad
left a comment
There was a problem hiding this comment.
Thanks for addressing all my changes, I think we are good to go! Tests make me more confident here. We can add an e2e test after we formalize the API as well given the surface area will change here.
I would like one more person from my team to 👍 so not going to give this an approve just yet - @getsentry/team-web-sdk-frontend please take a look.
AbhiPrasad
left a comment
There was a problem hiding this comment.
I think we've given everyone time to review if need be, let's not wait longer.
LGTM!
|
Thanks @AbhiPrasad. I'm happy to make changes down the line if we need to as well |
This PR introduce a new continuous profiling mode. This mode is
exclusive from the current mode which considers starting and stopping
profiles on a per span basis.
I've picked the interval duration of 5s as somewhat arbitrarily. The
idea is that we dont want profiles to grow too large, because that might
become a performance issue in the event that we have a lot of deep stack
samples to process.
Since profiling mode is exclusive, we will require users to add a
profilerMode (subject to change) as the SDK option (this is subject to
change as we align the APIs cross sdks). In terms of convenience, we are
likely also going to add a Sentry.profiler.start/stop methods so that
users can have access as to when they can start and stop the profiler
(not implemented as we havent standardized on the approach yet) -
currently this relies on
getIntegrationByName("ProfilingIntegration").profiler.stop
Since the UI does not support this mode yet, I will hide the
profilerMode hidden and only allow the current automated instrumentation
This PR introduce a new continuous profiling mode. This mode is exclusive from the current mode which considers starting and stopping profiles on a per span basis.
I've picked the interval duration of 5s as somewhat arbitrarily. The idea is that we dont want profiles to grow too large, because that might become a performance issue in the event that we have a lot of deep stack samples to process.
Since profiling mode is exclusive, we will require users to add a profilerMode (subject to change) as the SDK option (this is subject to change as we align the APIs cross sdks). In terms of convenience, we are likely also going to add a Sentry.profiler.start/stop methods so that users can have access as to when they can start and stop the profiler (not implemented as we havent standardized on the approach yet) - currently this relies on getIntegrationByName("ProfilingIntegration").profiler.stop
Since the UI does not support this mode yet, I will hide the profilerMode hidden and only allow the current automated instrumentation