feat(node): add continuous profiling mode - #12124

Merged
JonasBa merged 54 commits into
developfrom
jb/profiling/continuous-profiling
Jun 6, 2024
Merged

feat(node): add continuous profiling mode#12124
JonasBa merged 54 commits into
developfrom
jb/profiling/continuous-profiling

Conversation

@JonasBa

@JonasBaJonasBa commented May 20, 2024•
edited
Loading

Copy link
Copy Markdown
Contributor

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

@github-actions

github-actionsBot commented May 21, 2024•
edited
Loading

Copy link
Copy Markdown
Contributor

size-limit report 📦

PathSize
@sentry/browser21.74 KB (+0.04% 🔺)
@sentry/browser (incl. Tracing)32.78 KB (+0.03% 🔺)
@sentry/browser (incl. Tracing, Replay)68.35 KB (+0.02% 🔺)
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags61.66 KB (+0.02% 🔺)
@sentry/browser (incl. Tracing, Replay with Canvas)72.41 KB (+0.02% 🔺)
@sentry/browser (incl. Tracing, Replay, Feedback)84.51 KB (+0.01% 🔺)
@sentry/browser (incl. Tracing, Replay, Feedback, metrics)86.37 KB (+0.01% 🔺)
@sentry/browser (incl. metrics)25.93 KB (+0.04% 🔺)
@sentry/browser (incl. Feedback)37.9 KB (+0.03% 🔺)
@sentry/browser (incl. sendFeedback)26.33 KB (+0.04% 🔺)
@sentry/browser (incl. FeedbackAsync)30.87 KB (+0.03% 🔺)
@sentry/react24.52 KB (+0.04% 🔺)
@sentry/react (incl. Tracing)35.82 KB (+0.02% 🔺)
@sentry/vue25.74 KB (+0.04% 🔺)
@sentry/vue (incl. Tracing)34.61 KB (+0.02% 🔺)
@sentry/svelte21.87 KB (+0.04% 🔺)
CDN Bundle23.12 KB (+0.04% 🔺)
CDN Bundle (incl. Tracing)34.51 KB (+0.02% 🔺)
CDN Bundle (incl. Tracing, Replay)68.44 KB (+0.01% 🔺)
CDN Bundle (incl. Tracing, Replay, Feedback)73.61 KB (+0.02% 🔺)
CDN Bundle - uncompressed68.02 KB (+0.04% 🔺)
CDN Bundle (incl. Tracing) - uncompressed102.2 KB (+0.03% 🔺)
CDN Bundle (incl. Tracing, Replay) - uncompressed212.09 KB (+0.02% 🔺)
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed224.56 KB (+0.02% 🔺)
@sentry/nextjs (client)35.17 KB (+0.03% 🔺)
@sentry/sveltekit (client)33.41 KB (+0.03% 🔺)
@sentry/node115.25 KB (+0.01% 🔺)
@sentry/node - without tracing94.56 KB (+0.01% 🔺)
@sentry/aws-serverless103.73 KB (+0.01% 🔺)

@JonasBa
JonasBa marked this pull request as ready for review May 21, 2024 20:32
@lforst

lforst commented May 22, 2024

Copy link
Copy Markdown
Contributor

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.

@JonasBa

JonasBa commented May 22, 2024

Copy link
Copy Markdown
ContributorAuthor

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
AbhiPrasad self-requested a review June 3, 2024 14:40
@JonasBaJonasBa mentioned this pull request Jun 3, 2024
Closed
5 tasks
AbhiPrasad
AbhiPrasad reviewed Jun 4, 2024
View reviewed changes

@AbhiPrasadAbhiPrasad left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After this merges in we def also need an e2e test.

Comment threadpackages/profiling-node/src/cpu_profiler.ts Outdated
Comment threadpackages/profiling-node/src/integration.ts Outdated
Comment threadpackages/profiling-node/src/integration.ts
Comment threadpackages/profiling-node/src/integration.ts
Comment threadpackages/profiling-node/src/integration.ts Outdated
Comment threadpackages/profiling-node/src/utils.ts
Comment threadpackages/profiling-node/src/utils.ts
Comment threadpackages/profiling-node/bindings/cpu_profiler.cc Outdated
Comment threadpackages/profiling-node/bindings/cpu_profiler.cc
Comment threadpackages/profiling-node/bindings/cpu_profiler.cc
JonasBa added 16 commits June 4, 2024 11:47
@JonasBa
ref(profiling): use an enum and overload the signature
3529f86
@JonasBa
ref(profiling): use enum to stop profile
cab74a8
@JonasBa
ref(profiling): dont pass undefined to maybeProfileSpan
23c154d
@JonasBa
ref(profiling): single init point for chunk
e77d8ae
@JonasBa
ref(profiling): clearer timestamp namings
69ecbb8
@JonasBa
ref(profiling): clear timeout if present
a1de1c7
@JonasBa
ref(profiling): use ProfilingIntegration as generic for getIntegratio…
b860f3a
…nByName
@JonasBa
ref(profiling): fixup usage of profiler
9b490f3
@JonasBa
ref(profiling): ignore conforming to the signature in binding return …
d09aeab
…type
@JonasBa
ref(profiling): test exclusive modes
17a4c57
@JonasBa
ref(profiling): use LRU map to store profiles before their parent txn…
f4ab018
… or span terminates
@JonasBa
ref(profiling): use LRU map to store profiles before their parent txn…
d24ad25
… or span terminates
@JonasBa
Merge branch 'develop' into jb/profiling/continuous-profiling
440b1bf
@JonasBa
ref(profiling): format
14ebd51
@JonasBa
ref(profiling): format
9f9f462
@JonasBa
ref(profiling): format lint and rename enum
fd47fad
AbhiPrasad
AbhiPrasad reviewed Jun 4, 2024
View reviewed changes

@AbhiPrasadAbhiPrasad left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment threadpackages/profiling-node/src/integration.ts Outdated
AbhiPrasad
AbhiPrasad approved these changes Jun 6, 2024
View reviewed changes

@AbhiPrasadAbhiPrasad left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we've given everyone time to review if need be, let's not wait longer.

LGTM!

@JonasBa

JonasBa commented Jun 6, 2024

Copy link
Copy Markdown
ContributorAuthor

Thanks @AbhiPrasad. I'm happy to make changes down the line if we need to as well

@JonasBa
JonasBa merged commit cecb0d7 into developJun 6, 2024
@JonasBa
JonasBa deleted the jb/profiling/continuous-profiling branch June 6, 2024 18:06
billyvg pushed a commit that referenced this pull request Jun 10, 2024
@JonasBa@billyvg
feat(node): add continuous profiling mode (#12124)
cc3be00
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
@AbhiPrasadAbhiPrasad mentioned this pull request Jun 11, 2024
Closed
3 tasks
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@AbhiPrasadAbhiPrasadAbhiPrasad approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@JonasBa@lforst@AbhiPrasad
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
feat(node): add continuous profiling mode - #12124
Merged
JonasBa merged 54 commits into
developfrom 
jb/profiling/continuous-profiling
Jun 6, 2024
Merged
feat(node): add continuous profiling mode#12124
JonasBa merged 54 commits into
developfrom 
jb/profiling/continuous-profiling
Conversation
@JonasBa
@JonasBaJonasBa
commented
May 20, 2024•

edited

Loading

Copy link


Copy Markdown

Contributor
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
@github-actions
github-actionsBot
commented
May 21, 2024•

edited

Loading

Copy link


Copy Markdown

Contributor
size-limit report 📦
PathSize
@sentry/browser21.74 KB (+0.04% 🔺)
@sentry/browser (incl. Tracing)32.78 KB (+0.03% 🔺)
@sentry/browser (incl. Tracing, Replay)68.35 KB (+0.02% 🔺)
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags61.66 KB (+0.02% 🔺)
@sentry/browser (incl. Tracing, Replay with Canvas)72.41 KB (+0.02% 🔺)
@sentry/browser (incl. Tracing, Replay, Feedback)84.51 KB (+0.01% 🔺)
@sentry/browser (incl. Tracing, Replay, Feedback, metrics)86.37 KB (+0.01% 🔺)
@sentry/browser (incl. metrics)25.93 KB (+0.04% 🔺)
@sentry/browser (incl. Feedback)37.9 KB (+0.03% 🔺)
@sentry/browser (incl. sendFeedback)26.33 KB (+0.04% 🔺)
@sentry/browser (incl. FeedbackAsync)30.87 KB (+0.03% 🔺)
@sentry/react24.52 KB (+0.04% 🔺)
@sentry/react (incl. Tracing)35.82 KB (+0.02% 🔺)
@sentry/vue25.74 KB (+0.04% 🔺)
@sentry/vue (incl. Tracing)34.61 KB (+0.02% 🔺)
@sentry/svelte21.87 KB (+0.04% 🔺)
CDN Bundle23.12 KB (+0.04% 🔺)
CDN Bundle (incl. Tracing)34.51 KB (+0.02% 🔺)
CDN Bundle (incl. Tracing, Replay)68.44 KB (+0.01% 🔺)
CDN Bundle (incl. Tracing, Replay, Feedback)73.61 KB (+0.02% 🔺)
CDN Bundle - uncompressed68.02 KB (+0.04% 🔺)
CDN Bundle (incl. Tracing) - uncompressed102.2 KB (+0.03% 🔺)
CDN Bundle (incl. Tracing, Replay) - uncompressed212.09 KB (+0.02% 🔺)
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed224.56 KB (+0.02% 🔺)
@sentry/nextjs (client)35.17 KB (+0.03% 🔺)
@sentry/sveltekit (client)33.41 KB (+0.03% 🔺)
@sentry/node115.25 KB (+0.01% 🔺)
@sentry/node - without tracing94.56 KB (+0.01% 🔺)
@sentry/aws-serverless103.73 KB (+0.01% 🔺)
@JonasBa
JonasBa
marked this pull request as ready for review
May 21, 2024 20:32
@lforst
lforst
commented
May 22, 2024

Copy link


Copy Markdown

Contributor
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.
@JonasBa
JonasBa
commented
May 22, 2024

Copy link


Copy Markdown

ContributorAuthor
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
AbhiPrasad
self-requested a review
June 3, 2024 14:40
@JonasBaJonasBa
mentioned this pull request
Jun 3, 2024

Closed

5 tasks
AbhiPrasad
AbhiPrasad
reviewed
Jun 4, 2024
View reviewed changes
@AbhiPrasadAbhiPrasad
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After this merges in we def also need an e2e test.
Comment threadpackages/profiling-node/src/cpu_profiler.ts
Outdated

Comment threadpackages/profiling-node/src/integration.ts
Outdated

Comment threadpackages/profiling-node/src/integration.ts
Comment threadpackages/profiling-node/src/integration.ts
Comment threadpackages/profiling-node/src/integration.ts
Outdated

Comment threadpackages/profiling-node/src/utils.ts
Comment threadpackages/profiling-node/src/utils.ts
Comment threadpackages/profiling-node/bindings/cpu_profiler.cc
Outdated

Comment threadpackages/profiling-node/bindings/cpu_profiler.cc
Comment threadpackages/profiling-node/bindings/cpu_profiler.cc
JonasBa
added 16 commits
June 4, 2024 11:47
@JonasBa
ref(profiling): use an enum and overload the signature
3529f86
@JonasBa
ref(profiling): use enum to stop profile
cab74a8
@JonasBa
ref(profiling): dont pass undefined to maybeProfileSpan
23c154d
@JonasBa
ref(profiling): single init point for chunk
e77d8ae
@JonasBa
ref(profiling): clearer timestamp namings
69ecbb8
@JonasBa
ref(profiling): clear timeout if present
a1de1c7
@JonasBa
ref(profiling): use ProfilingIntegration as generic for getIntegratio…
b860f3a
…nByName
@JonasBa
ref(profiling): fixup usage of profiler
9b490f3
@JonasBa
ref(profiling): ignore conforming to the signature in binding return …
d09aeab
…type
@JonasBa
ref(profiling): test exclusive modes
17a4c57
@JonasBa
ref(profiling): use LRU map to store profiles before their parent txn…
f4ab018
… or span terminates
@JonasBa
ref(profiling): use LRU map to store profiles before their parent txn…
d24ad25
… or span terminates
@JonasBa
Merge branch 'develop' into jb/profiling/continuous-profiling
440b1bf
@JonasBa
ref(profiling): format
14ebd51
@JonasBa
ref(profiling): format
9f9f462
@JonasBa
ref(profiling): format lint and rename enum
fd47fad
AbhiPrasad
AbhiPrasad
reviewed
Jun 4, 2024
View reviewed changes
@AbhiPrasadAbhiPrasad
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Comment threadpackages/profiling-node/src/integration.ts
Outdated

AbhiPrasad
AbhiPrasad
approved these changes
Jun 6, 2024
View reviewed changes
@AbhiPrasadAbhiPrasad
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we've given everyone time to review if need be, let's not wait longer.
LGTM!
@JonasBa
JonasBa
commented
Jun 6, 2024

Copy link


Copy Markdown

ContributorAuthor
Thanks @AbhiPrasad. I'm happy to make changes down the line if we need to as well
@JonasBa
JonasBa
merged commit cecb0d7
into
developJun 6, 2024
@JonasBa
JonasBa
deleted the

jb/profiling/continuous-profiling

branch
June 6, 2024 18:06
billyvg pushed a commit
that referenced
this pull request
Jun 10, 2024
@JonasBa@billyvg
feat(node): add continuous profiling mode (#12124)
cc3be00
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
@AbhiPrasadAbhiPrasad
mentioned this pull request
Jun 11, 2024

Closed

3 tasks
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers


1 more reviewer

@AbhiPrasadAbhiPrasadAbhiPrasad approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@JonasBa@lforst@AbhiPrasad

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
feat(node): add continuous profiling mode - #12124
Merged
JonasBa merged 54 commits into
developfrom 
jb/profiling/continuous-profiling
Jun 6, 2024
Merged
feat(node): add continuous profiling mode#12124
JonasBa merged 54 commits into
developfrom 
jb/profiling/continuous-profiling
Conversation
@JonasBa
@JonasBaJonasBa
commented
May 20, 2024•

edited

Loading

Copy link


Copy Markdown

Contributor
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
@github-actions
github-actionsBot
commented
May 21, 2024•

edited

Loading

Copy link


Copy Markdown

Contributor
size-limit report 📦
PathSize
@sentry/browser21.74 KB (+0.04% 🔺)
@sentry/browser (incl. Tracing)32.78 KB (+0.03% 🔺)
@sentry/browser (incl. Tracing, Replay)68.35 KB (+0.02% 🔺)
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags61.66 KB (+0.02% 🔺)
@sentry/browser (incl. Tracing, Replay with Canvas)72.41 KB (+0.02% 🔺)
@sentry/browser (incl. Tracing, Replay, Feedback)84.51 KB (+0.01% 🔺)
@sentry/browser (incl. Tracing, Replay, Feedback, metrics)86.37 KB (+0.01% 🔺)
@sentry/browser (incl. metrics)25.93 KB (+0.04% 🔺)
@sentry/browser (incl. Feedback)37.9 KB (+0.03% 🔺)
@sentry/browser (incl. sendFeedback)26.33 KB (+0.04% 🔺)
@sentry/browser (incl. FeedbackAsync)30.87 KB (+0.03% 🔺)
@sentry/react24.52 KB (+0.04% 🔺)
@sentry/react (incl. Tracing)35.82 KB (+0.02% 🔺)
@sentry/vue25.74 KB (+0.04% 🔺)
@sentry/vue (incl. Tracing)34.61 KB (+0.02% 🔺)
@sentry/svelte21.87 KB (+0.04% 🔺)
CDN Bundle23.12 KB (+0.04% 🔺)
CDN Bundle (incl. Tracing)34.51 KB (+0.02% 🔺)
CDN Bundle (incl. Tracing, Replay)68.44 KB (+0.01% 🔺)
CDN Bundle (incl. Tracing, Replay, Feedback)73.61 KB (+0.02% 🔺)
CDN Bundle - uncompressed68.02 KB (+0.04% 🔺)
CDN Bundle (incl. Tracing) - uncompressed102.2 KB (+0.03% 🔺)
CDN Bundle (incl. Tracing, Replay) - uncompressed212.09 KB (+0.02% 🔺)
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed224.56 KB (+0.02% 🔺)
@sentry/nextjs (client)35.17 KB (+0.03% 🔺)
@sentry/sveltekit (client)33.41 KB (+0.03% 🔺)
@sentry/node115.25 KB (+0.01% 🔺)
@sentry/node - without tracing94.56 KB (+0.01% 🔺)
@sentry/aws-serverless103.73 KB (+0.01% 🔺)
@JonasBa
JonasBa
marked this pull request as ready for review
May 21, 2024 20:32
@lforst
lforst
commented
May 22, 2024

Copy link


Copy Markdown

Contributor
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.
@JonasBa
JonasBa
commented
May 22, 2024

Copy link


Copy Markdown

ContributorAuthor
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
AbhiPrasad
self-requested a review
June 3, 2024 14:40
@JonasBaJonasBa
mentioned this pull request
Jun 3, 2024

Closed

5 tasks
AbhiPrasad
AbhiPrasad
reviewed
Jun 4, 2024
View reviewed changes
@AbhiPrasadAbhiPrasad
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After this merges in we def also need an e2e test.
Comment threadpackages/profiling-node/src/cpu_profiler.ts
Outdated

Comment threadpackages/profiling-node/src/integration.ts
Outdated

Comment threadpackages/profiling-node/src/integration.ts
Comment threadpackages/profiling-node/src/integration.ts
Comment threadpackages/profiling-node/src/integration.ts
Outdated

Comment threadpackages/profiling-node/src/utils.ts
Comment threadpackages/profiling-node/src/utils.ts
Comment threadpackages/profiling-node/bindings/cpu_profiler.cc
Outdated

Comment threadpackages/profiling-node/bindings/cpu_profiler.cc
Comment threadpackages/profiling-node/bindings/cpu_profiler.cc
JonasBa
added 16 commits
June 4, 2024 11:47
@JonasBa
ref(profiling): use an enum and overload the signature
3529f86
@JonasBa
ref(profiling): use enum to stop profile
cab74a8
@JonasBa
ref(profiling): dont pass undefined to maybeProfileSpan
23c154d
@JonasBa
ref(profiling): single init point for chunk
e77d8ae
@JonasBa
ref(profiling): clearer timestamp namings
69ecbb8
@JonasBa
ref(profiling): clear timeout if present
a1de1c7
@JonasBa
ref(profiling): use ProfilingIntegration as generic for getIntegratio…
b860f3a
…nByName
@JonasBa
ref(profiling): fixup usage of profiler
9b490f3
@JonasBa
ref(profiling): ignore conforming to the signature in binding return …
d09aeab
…type
@JonasBa
ref(profiling): test exclusive modes
17a4c57
@JonasBa
ref(profiling): use LRU map to store profiles before their parent txn…
f4ab018
… or span terminates
@JonasBa
ref(profiling): use LRU map to store profiles before their parent txn…
d24ad25
… or span terminates
@JonasBa
Merge branch 'develop' into jb/profiling/continuous-profiling
440b1bf
@JonasBa
ref(profiling): format
14ebd51
@JonasBa
ref(profiling): format
9f9f462
@JonasBa
ref(profiling): format lint and rename enum
fd47fad
AbhiPrasad
AbhiPrasad
reviewed
Jun 4, 2024
View reviewed changes
@AbhiPrasadAbhiPrasad
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Comment threadpackages/profiling-node/src/integration.ts
Outdated

AbhiPrasad
AbhiPrasad
approved these changes
Jun 6, 2024
View reviewed changes
@AbhiPrasadAbhiPrasad
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we've given everyone time to review if need be, let's not wait longer.
LGTM!
@JonasBa
JonasBa
commented
Jun 6, 2024

Copy link


Copy Markdown

ContributorAuthor
Thanks @AbhiPrasad. I'm happy to make changes down the line if we need to as well
@JonasBa
JonasBa
merged commit cecb0d7
into
developJun 6, 2024
@JonasBa
JonasBa
deleted the

jb/profiling/continuous-profiling

branch
June 6, 2024 18:06
billyvg pushed a commit
that referenced
this pull request
Jun 10, 2024
@JonasBa@billyvg
feat(node): add continuous profiling mode (#12124)
cc3be00
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
@AbhiPrasadAbhiPrasad
mentioned this pull request
Jun 11, 2024

Closed

3 tasks
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers


1 more reviewer

@AbhiPrasadAbhiPrasadAbhiPrasad approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@JonasBa@lforst@AbhiPrasad

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
feat(node): add continuous profiling mode - #12124
Merged
JonasBa merged 54 commits into
developfrom 
jb/profiling/continuous-profiling
Jun 6, 2024
Merged
feat(node): add continuous profiling mode#12124
JonasBa merged 54 commits into
developfrom 
jb/profiling/continuous-profiling
Conversation
@JonasBa
@JonasBaJonasBa
commented
May 20, 2024•

edited

Loading

Copy link


Copy Markdown

Contributor
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
@github-actions
github-actionsBot
commented
May 21, 2024•

edited

Loading

Copy link


Copy Markdown

Contributor
size-limit report 📦
PathSize
@sentry/browser21.74 KB (+0.04% 🔺)
@sentry/browser (incl. Tracing)32.78 KB (+0.03% 🔺)
@sentry/browser (incl. Tracing, Replay)68.35 KB (+0.02% 🔺)
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags61.66 KB (+0.02% 🔺)
@sentry/browser (incl. Tracing, Replay with Canvas)72.41 KB (+0.02% 🔺)
@sentry/browser (incl. Tracing, Replay, Feedback)84.51 KB (+0.01% 🔺)
@sentry/browser (incl. Tracing, Replay, Feedback, metrics)86.37 KB (+0.01% 🔺)
@sentry/browser (incl. metrics)25.93 KB (+0.04% 🔺)
@sentry/browser (incl. Feedback)37.9 KB (+0.03% 🔺)
@sentry/browser (incl. sendFeedback)26.33 KB (+0.04% 🔺)
@sentry/browser (incl. FeedbackAsync)30.87 KB (+0.03% 🔺)
@sentry/react24.52 KB (+0.04% 🔺)
@sentry/react (incl. Tracing)35.82 KB (+0.02% 🔺)
@sentry/vue25.74 KB (+0.04% 🔺)
@sentry/vue (incl. Tracing)34.61 KB (+0.02% 🔺)
@sentry/svelte21.87 KB (+0.04% 🔺)
CDN Bundle23.12 KB (+0.04% 🔺)
CDN Bundle (incl. Tracing)34.51 KB (+0.02% 🔺)
CDN Bundle (incl. Tracing, Replay)68.44 KB (+0.01% 🔺)
CDN Bundle (incl. Tracing, Replay, Feedback)73.61 KB (+0.02% 🔺)
CDN Bundle - uncompressed68.02 KB (+0.04% 🔺)
CDN Bundle (incl. Tracing) - uncompressed102.2 KB (+0.03% 🔺)
CDN Bundle (incl. Tracing, Replay) - uncompressed212.09 KB (+0.02% 🔺)
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed224.56 KB (+0.02% 🔺)
@sentry/nextjs (client)35.17 KB (+0.03% 🔺)
@sentry/sveltekit (client)33.41 KB (+0.03% 🔺)
@sentry/node115.25 KB (+0.01% 🔺)
@sentry/node - without tracing94.56 KB (+0.01% 🔺)
@sentry/aws-serverless103.73 KB (+0.01% 🔺)
@JonasBa
JonasBa
marked this pull request as ready for review
May 21, 2024 20:32
@lforst
lforst
commented
May 22, 2024

Copy link


Copy Markdown

Contributor
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.
@JonasBa
JonasBa
commented
May 22, 2024

Copy link


Copy Markdown

ContributorAuthor
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
AbhiPrasad
self-requested a review
June 3, 2024 14:40
@JonasBaJonasBa
mentioned this pull request
Jun 3, 2024

Closed

5 tasks
AbhiPrasad
AbhiPrasad
reviewed
Jun 4, 2024
View reviewed changes
@AbhiPrasadAbhiPrasad
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After this merges in we def also need an e2e test.
Comment threadpackages/profiling-node/src/cpu_profiler.ts
Outdated

Comment threadpackages/profiling-node/src/integration.ts
Outdated

Comment threadpackages/profiling-node/src/integration.ts
Comment threadpackages/profiling-node/src/integration.ts
Comment threadpackages/profiling-node/src/integration.ts
Outdated

Comment threadpackages/profiling-node/src/utils.ts
Comment threadpackages/profiling-node/src/utils.ts
Comment threadpackages/profiling-node/bindings/cpu_profiler.cc
Outdated

Comment threadpackages/profiling-node/bindings/cpu_profiler.cc
Comment threadpackages/profiling-node/bindings/cpu_profiler.cc
JonasBa
added 16 commits
June 4, 2024 11:47
@JonasBa
ref(profiling): use an enum and overload the signature
3529f86
@JonasBa
ref(profiling): use enum to stop profile
cab74a8
@JonasBa
ref(profiling): dont pass undefined to maybeProfileSpan
23c154d
@JonasBa
ref(profiling): single init point for chunk
e77d8ae
@JonasBa
ref(profiling): clearer timestamp namings
69ecbb8
@JonasBa
ref(profiling): clear timeout if present
a1de1c7
@JonasBa
ref(profiling): use ProfilingIntegration as generic for getIntegratio…
b860f3a
…nByName
@JonasBa
ref(profiling): fixup usage of profiler
9b490f3
@JonasBa
ref(profiling): ignore conforming to the signature in binding return …
d09aeab
…type
@JonasBa
ref(profiling): test exclusive modes
17a4c57
@JonasBa
ref(profiling): use LRU map to store profiles before their parent txn…
f4ab018
… or span terminates
@JonasBa
ref(profiling): use LRU map to store profiles before their parent txn…
d24ad25
… or span terminates
@JonasBa
Merge branch 'develop' into jb/profiling/continuous-profiling
440b1bf
@JonasBa
ref(profiling): format
14ebd51
@JonasBa
ref(profiling): format
9f9f462
@JonasBa
ref(profiling): format lint and rename enum
fd47fad
AbhiPrasad
AbhiPrasad
reviewed
Jun 4, 2024
View reviewed changes
@AbhiPrasadAbhiPrasad
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Comment threadpackages/profiling-node/src/integration.ts
Outdated

AbhiPrasad
AbhiPrasad
approved these changes
Jun 6, 2024
View reviewed changes
@AbhiPrasadAbhiPrasad
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we've given everyone time to review if need be, let's not wait longer.
LGTM!
@JonasBa
JonasBa
commented
Jun 6, 2024

Copy link


Copy Markdown

ContributorAuthor
Thanks @AbhiPrasad. I'm happy to make changes down the line if we need to as well
@JonasBa
JonasBa
merged commit cecb0d7
into
developJun 6, 2024
@JonasBa
JonasBa
deleted the

jb/profiling/continuous-profiling

branch
June 6, 2024 18:06
billyvg pushed a commit
that referenced
this pull request
Jun 10, 2024
@JonasBa@billyvg
feat(node): add continuous profiling mode (#12124)
cc3be00
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
@AbhiPrasadAbhiPrasad
mentioned this pull request
Jun 11, 2024

Closed

3 tasks
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers


1 more reviewer

@AbhiPrasadAbhiPrasadAbhiPrasad approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@JonasBa@lforst@AbhiPrasad

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
feat(node): add continuous profiling mode - #12124
Merged
JonasBa merged 54 commits into
developfrom 
jb/profiling/continuous-profiling
Jun 6, 2024
Merged
feat(node): add continuous profiling mode#12124
JonasBa merged 54 commits into
developfrom 
jb/profiling/continuous-profiling
Conversation
@JonasBa
@JonasBaJonasBa
commented
May 20, 2024•

edited

Loading

Copy link


Copy Markdown

Contributor
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
@github-actions
github-actionsBot
commented
May 21, 2024•

edited

Loading

Copy link


Copy Markdown

Contributor
size-limit report 📦
PathSize
@sentry/browser21.74 KB (+0.04% 🔺)
@sentry/browser (incl. Tracing)32.78 KB (+0.03% 🔺)
@sentry/browser (incl. Tracing, Replay)68.35 KB (+0.02% 🔺)
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags61.66 KB (+0.02% 🔺)
@sentry/browser (incl. Tracing, Replay with Canvas)72.41 KB (+0.02% 🔺)
@sentry/browser (incl. Tracing, Replay, Feedback)84.51 KB (+0.01% 🔺)
@sentry/browser (incl. Tracing, Replay, Feedback, metrics)86.37 KB (+0.01% 🔺)
@sentry/browser (incl. metrics)25.93 KB (+0.04% 🔺)
@sentry/browser (incl. Feedback)37.9 KB (+0.03% 🔺)
@sentry/browser (incl. sendFeedback)26.33 KB (+0.04% 🔺)
@sentry/browser (incl. FeedbackAsync)30.87 KB (+0.03% 🔺)
@sentry/react24.52 KB (+0.04% 🔺)
@sentry/react (incl. Tracing)35.82 KB (+0.02% 🔺)
@sentry/vue25.74 KB (+0.04% 🔺)
@sentry/vue (incl. Tracing)34.61 KB (+0.02% 🔺)
@sentry/svelte21.87 KB (+0.04% 🔺)
CDN Bundle23.12 KB (+0.04% 🔺)
CDN Bundle (incl. Tracing)34.51 KB (+0.02% 🔺)
CDN Bundle (incl. Tracing, Replay)68.44 KB (+0.01% 🔺)
CDN Bundle (incl. Tracing, Replay, Feedback)73.61 KB (+0.02% 🔺)
CDN Bundle - uncompressed68.02 KB (+0.04% 🔺)
CDN Bundle (incl. Tracing) - uncompressed102.2 KB (+0.03% 🔺)
CDN Bundle (incl. Tracing, Replay) - uncompressed212.09 KB (+0.02% 🔺)
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed224.56 KB (+0.02% 🔺)
@sentry/nextjs (client)35.17 KB (+0.03% 🔺)
@sentry/sveltekit (client)33.41 KB (+0.03% 🔺)
@sentry/node115.25 KB (+0.01% 🔺)
@sentry/node - without tracing94.56 KB (+0.01% 🔺)
@sentry/aws-serverless103.73 KB (+0.01% 🔺)
@JonasBa
JonasBa
marked this pull request as ready for review
May 21, 2024 20:32
@lforst
lforst
commented
May 22, 2024

Copy link


Copy Markdown

Contributor
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.
@JonasBa
JonasBa
commented
May 22, 2024

Copy link


Copy Markdown

ContributorAuthor
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
AbhiPrasad
self-requested a review
June 3, 2024 14:40
@JonasBaJonasBa
mentioned this pull request
Jun 3, 2024

Closed

5 tasks
AbhiPrasad
AbhiPrasad
reviewed
Jun 4, 2024
View reviewed changes
@AbhiPrasadAbhiPrasad
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After this merges in we def also need an e2e test.
Comment threadpackages/profiling-node/src/cpu_profiler.ts
Outdated

Comment threadpackages/profiling-node/src/integration.ts
Outdated

Comment threadpackages/profiling-node/src/integration.ts
Comment threadpackages/profiling-node/src/integration.ts
Comment threadpackages/profiling-node/src/integration.ts
Outdated

Comment threadpackages/profiling-node/src/utils.ts
Comment threadpackages/profiling-node/src/utils.ts
Comment threadpackages/profiling-node/bindings/cpu_profiler.cc
Outdated

Comment threadpackages/profiling-node/bindings/cpu_profiler.cc
Comment threadpackages/profiling-node/bindings/cpu_profiler.cc
JonasBa
added 16 commits
June 4, 2024 11:47
@JonasBa
ref(profiling): use an enum and overload the signature
3529f86
@JonasBa
ref(profiling): use enum to stop profile
cab74a8
@JonasBa
ref(profiling): dont pass undefined to maybeProfileSpan
23c154d
@JonasBa
ref(profiling): single init point for chunk
e77d8ae
@JonasBa
ref(profiling): clearer timestamp namings
69ecbb8
@JonasBa
ref(profiling): clear timeout if present
a1de1c7
@JonasBa
ref(profiling): use ProfilingIntegration as generic for getIntegratio…
b860f3a
…nByName
@JonasBa
ref(profiling): fixup usage of profiler
9b490f3
@JonasBa
ref(profiling): ignore conforming to the signature in binding return …
d09aeab
…type
@JonasBa
ref(profiling): test exclusive modes
17a4c57
@JonasBa
ref(profiling): use LRU map to store profiles before their parent txn…
f4ab018
… or span terminates
@JonasBa
ref(profiling): use LRU map to store profiles before their parent txn…
d24ad25
… or span terminates
@JonasBa
Merge branch 'develop' into jb/profiling/continuous-profiling
440b1bf
@JonasBa
ref(profiling): format
14ebd51
@JonasBa
ref(profiling): format
9f9f462
@JonasBa
ref(profiling): format lint and rename enum
fd47fad
AbhiPrasad
AbhiPrasad
reviewed
Jun 4, 2024
View reviewed changes
@AbhiPrasadAbhiPrasad
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Comment threadpackages/profiling-node/src/integration.ts
Outdated

AbhiPrasad
AbhiPrasad
approved these changes
Jun 6, 2024
View reviewed changes
@AbhiPrasadAbhiPrasad
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we've given everyone time to review if need be, let's not wait longer.
LGTM!
@JonasBa
JonasBa
commented
Jun 6, 2024

Copy link


Copy Markdown

ContributorAuthor
Thanks @AbhiPrasad. I'm happy to make changes down the line if we need to as well
@JonasBa
JonasBa
merged commit cecb0d7
into
developJun 6, 2024
@JonasBa
JonasBa
deleted the

jb/profiling/continuous-profiling

branch
June 6, 2024 18:06
billyvg pushed a commit
that referenced
this pull request
Jun 10, 2024
@JonasBa@billyvg
feat(node): add continuous profiling mode (#12124)
cc3be00
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
@AbhiPrasadAbhiPrasad
mentioned this pull request
Jun 11, 2024

Closed

3 tasks
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers


1 more reviewer

@AbhiPrasadAbhiPrasadAbhiPrasad approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@JonasBa@lforst@AbhiPrasad

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
feat(node): add continuous profiling mode - #12124
Merged
JonasBa merged 54 commits into
developfrom 
jb/profiling/continuous-profiling
Jun 6, 2024
Merged
feat(node): add continuous profiling mode#12124
JonasBa merged 54 commits into
developfrom 
jb/profiling/continuous-profiling
Conversation
@JonasBa
@JonasBaJonasBa
commented
May 20, 2024•

edited

Loading

Copy link


Copy Markdown

Contributor
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
@github-actions
github-actionsBot
commented
May 21, 2024•

edited

Loading

Copy link


Copy Markdown

Contributor
size-limit report 📦
PathSize
@sentry/browser21.74 KB (+0.04% 🔺)
@sentry/browser (incl. Tracing)32.78 KB (+0.03% 🔺)
@sentry/browser (incl. Tracing, Replay)68.35 KB (+0.02% 🔺)
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags61.66 KB (+0.02% 🔺)
@sentry/browser (incl. Tracing, Replay with Canvas)72.41 KB (+0.02% 🔺)
@sentry/browser (incl. Tracing, Replay, Feedback)84.51 KB (+0.01% 🔺)
@sentry/browser (incl. Tracing, Replay, Feedback, metrics)86.37 KB (+0.01% 🔺)
@sentry/browser (incl. metrics)25.93 KB (+0.04% 🔺)
@sentry/browser (incl. Feedback)37.9 KB (+0.03% 🔺)
@sentry/browser (incl. sendFeedback)26.33 KB (+0.04% 🔺)
@sentry/browser (incl. FeedbackAsync)30.87 KB (+0.03% 🔺)
@sentry/react24.52 KB (+0.04% 🔺)
@sentry/react (incl. Tracing)35.82 KB (+0.02% 🔺)
@sentry/vue25.74 KB (+0.04% 🔺)
@sentry/vue (incl. Tracing)34.61 KB (+0.02% 🔺)
@sentry/svelte21.87 KB (+0.04% 🔺)
CDN Bundle23.12 KB (+0.04% 🔺)
CDN Bundle (incl. Tracing)34.51 KB (+0.02% 🔺)
CDN Bundle (incl. Tracing, Replay)68.44 KB (+0.01% 🔺)
CDN Bundle (incl. Tracing, Replay, Feedback)73.61 KB (+0.02% 🔺)
CDN Bundle - uncompressed68.02 KB (+0.04% 🔺)
CDN Bundle (incl. Tracing) - uncompressed102.2 KB (+0.03% 🔺)
CDN Bundle (incl. Tracing, Replay) - uncompressed212.09 KB (+0.02% 🔺)
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed224.56 KB (+0.02% 🔺)
@sentry/nextjs (client)35.17 KB (+0.03% 🔺)
@sentry/sveltekit (client)33.41 KB (+0.03% 🔺)
@sentry/node115.25 KB (+0.01% 🔺)
@sentry/node - without tracing94.56 KB (+0.01% 🔺)
@sentry/aws-serverless103.73 KB (+0.01% 🔺)
@JonasBa
JonasBa
marked this pull request as ready for review
May 21, 2024 20:32
@lforst
lforst
commented
May 22, 2024

Copy link


Copy Markdown

Contributor
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.
@JonasBa
JonasBa
commented
May 22, 2024

Copy link


Copy Markdown

ContributorAuthor
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
AbhiPrasad
self-requested a review
June 3, 2024 14:40
@JonasBaJonasBa
mentioned this pull request
Jun 3, 2024

Closed

5 tasks
AbhiPrasad
AbhiPrasad
reviewed
Jun 4, 2024
View reviewed changes
@AbhiPrasadAbhiPrasad
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After this merges in we def also need an e2e test.
Comment threadpackages/profiling-node/src/cpu_profiler.ts
Outdated

Comment threadpackages/profiling-node/src/integration.ts
Outdated

Comment threadpackages/profiling-node/src/integration.ts
Comment threadpackages/profiling-node/src/integration.ts
Comment threadpackages/profiling-node/src/integration.ts
Outdated

Comment threadpackages/profiling-node/src/utils.ts
Comment threadpackages/profiling-node/src/utils.ts
Comment threadpackages/profiling-node/bindings/cpu_profiler.cc
Outdated

Comment threadpackages/profiling-node/bindings/cpu_profiler.cc
Comment threadpackages/profiling-node/bindings/cpu_profiler.cc
JonasBa
added 16 commits
June 4, 2024 11:47
@JonasBa
ref(profiling): use an enum and overload the signature
3529f86
@JonasBa
ref(profiling): use enum to stop profile
cab74a8
@JonasBa
ref(profiling): dont pass undefined to maybeProfileSpan
23c154d
@JonasBa
ref(profiling): single init point for chunk
e77d8ae
@JonasBa
ref(profiling): clearer timestamp namings
69ecbb8
@JonasBa
ref(profiling): clear timeout if present
a1de1c7
@JonasBa
ref(profiling): use ProfilingIntegration as generic for getIntegratio…
b860f3a
…nByName
@JonasBa
ref(profiling): fixup usage of profiler
9b490f3
@JonasBa
ref(profiling): ignore conforming to the signature in binding return …
d09aeab
…type
@JonasBa
ref(profiling): test exclusive modes
17a4c57
@JonasBa
ref(profiling): use LRU map to store profiles before their parent txn…
f4ab018
… or span terminates
@JonasBa
ref(profiling): use LRU map to store profiles before their parent txn…
d24ad25
… or span terminates
@JonasBa
Merge branch 'develop' into jb/profiling/continuous-profiling
440b1bf
@JonasBa
ref(profiling): format
14ebd51
@JonasBa
ref(profiling): format
9f9f462
@JonasBa
ref(profiling): format lint and rename enum
fd47fad
AbhiPrasad
AbhiPrasad
reviewed
Jun 4, 2024
View reviewed changes
@AbhiPrasadAbhiPrasad
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Comment threadpackages/profiling-node/src/integration.ts
Outdated

AbhiPrasad
AbhiPrasad
approved these changes
Jun 6, 2024
View reviewed changes
@AbhiPrasadAbhiPrasad
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we've given everyone time to review if need be, let's not wait longer.
LGTM!
@JonasBa
JonasBa
commented
Jun 6, 2024

Copy link


Copy Markdown

ContributorAuthor
Thanks @AbhiPrasad. I'm happy to make changes down the line if we need to as well
@JonasBa
JonasBa
merged commit cecb0d7
into
developJun 6, 2024
@JonasBa
JonasBa
deleted the

jb/profiling/continuous-profiling

branch
June 6, 2024 18:06
billyvg pushed a commit
that referenced
this pull request
Jun 10, 2024
@JonasBa@billyvg
feat(node): add continuous profiling mode (#12124)
cc3be00
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
@AbhiPrasadAbhiPrasad
mentioned this pull request
Jun 11, 2024

Closed

3 tasks
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers


1 more reviewer

@AbhiPrasadAbhiPrasadAbhiPrasad approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@JonasBa@lforst@AbhiPrasad

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
feat(node): add continuous profiling mode - #12124
Merged
JonasBa merged 54 commits into
developfrom 
jb/profiling/continuous-profiling
Jun 6, 2024
Merged
feat(node): add continuous profiling mode#12124
JonasBa merged 54 commits into
developfrom 
jb/profiling/continuous-profiling
Conversation
@JonasBa
@JonasBaJonasBa
commented
May 20, 2024•

edited

Loading

Copy link


Copy Markdown

Contributor
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
@github-actions
github-actionsBot
commented
May 21, 2024•

edited

Loading

Copy link


Copy Markdown

Contributor
size-limit report 📦
PathSize
@sentry/browser21.74 KB (+0.04% 🔺)
@sentry/browser (incl. Tracing)32.78 KB (+0.03% 🔺)
@sentry/browser (incl. Tracing, Replay)68.35 KB (+0.02% 🔺)
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags61.66 KB (+0.02% 🔺)
@sentry/browser (incl. Tracing, Replay with Canvas)72.41 KB (+0.02% 🔺)
@sentry/browser (incl. Tracing, Replay, Feedback)84.51 KB (+0.01% 🔺)
@sentry/browser (incl. Tracing, Replay, Feedback, metrics)86.37 KB (+0.01% 🔺)
@sentry/browser (incl. metrics)25.93 KB (+0.04% 🔺)
@sentry/browser (incl. Feedback)37.9 KB (+0.03% 🔺)
@sentry/browser (incl. sendFeedback)26.33 KB (+0.04% 🔺)
@sentry/browser (incl. FeedbackAsync)30.87 KB (+0.03% 🔺)
@sentry/react24.52 KB (+0.04% 🔺)
@sentry/react (incl. Tracing)35.82 KB (+0.02% 🔺)
@sentry/vue25.74 KB (+0.04% 🔺)
@sentry/vue (incl. Tracing)34.61 KB (+0.02% 🔺)
@sentry/svelte21.87 KB (+0.04% 🔺)
CDN Bundle23.12 KB (+0.04% 🔺)
CDN Bundle (incl. Tracing)34.51 KB (+0.02% 🔺)
CDN Bundle (incl. Tracing, Replay)68.44 KB (+0.01% 🔺)
CDN Bundle (incl. Tracing, Replay, Feedback)73.61 KB (+0.02% 🔺)
CDN Bundle - uncompressed68.02 KB (+0.04% 🔺)
CDN Bundle (incl. Tracing) - uncompressed102.2 KB (+0.03% 🔺)
CDN Bundle (incl. Tracing, Replay) - uncompressed212.09 KB (+0.02% 🔺)
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed224.56 KB (+0.02% 🔺)
@sentry/nextjs (client)35.17 KB (+0.03% 🔺)
@sentry/sveltekit (client)33.41 KB (+0.03% 🔺)
@sentry/node115.25 KB (+0.01% 🔺)
@sentry/node - without tracing94.56 KB (+0.01% 🔺)
@sentry/aws-serverless103.73 KB (+0.01% 🔺)
@JonasBa
JonasBa
marked this pull request as ready for review
May 21, 2024 20:32
@lforst
lforst
commented
May 22, 2024

Copy link


Copy Markdown

Contributor
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.
@JonasBa
JonasBa
commented
May 22, 2024

Copy link


Copy Markdown

ContributorAuthor
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
AbhiPrasad
self-requested a review
June 3, 2024 14:40
@JonasBaJonasBa
mentioned this pull request
Jun 3, 2024

Closed

5 tasks
AbhiPrasad
AbhiPrasad
reviewed
Jun 4, 2024
View reviewed changes
@AbhiPrasadAbhiPrasad
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After this merges in we def also need an e2e test.
Comment threadpackages/profiling-node/src/cpu_profiler.ts
Outdated

Comment threadpackages/profiling-node/src/integration.ts
Outdated

Comment threadpackages/profiling-node/src/integration.ts
Comment threadpackages/profiling-node/src/integration.ts
Comment threadpackages/profiling-node/src/integration.ts
Outdated

Comment threadpackages/profiling-node/src/utils.ts
Comment threadpackages/profiling-node/src/utils.ts
Comment threadpackages/profiling-node/bindings/cpu_profiler.cc
Outdated

Comment threadpackages/profiling-node/bindings/cpu_profiler.cc
Comment threadpackages/profiling-node/bindings/cpu_profiler.cc
JonasBa
added 16 commits
June 4, 2024 11:47
@JonasBa
ref(profiling): use an enum and overload the signature
3529f86
@JonasBa
ref(profiling): use enum to stop profile
cab74a8
@JonasBa
ref(profiling): dont pass undefined to maybeProfileSpan
23c154d
@JonasBa
ref(profiling): single init point for chunk
e77d8ae
@JonasBa
ref(profiling): clearer timestamp namings
69ecbb8
@JonasBa
ref(profiling): clear timeout if present
a1de1c7
@JonasBa
ref(profiling): use ProfilingIntegration as generic for getIntegratio…
b860f3a
…nByName
@JonasBa
ref(profiling): fixup usage of profiler
9b490f3
@JonasBa
ref(profiling): ignore conforming to the signature in binding return …
d09aeab
…type
@JonasBa
ref(profiling): test exclusive modes
17a4c57
@JonasBa
ref(profiling): use LRU map to store profiles before their parent txn…
f4ab018
… or span terminates
@JonasBa
ref(profiling): use LRU map to store profiles before their parent txn…
d24ad25
… or span terminates
@JonasBa
Merge branch 'develop' into jb/profiling/continuous-profiling
440b1bf
@JonasBa
ref(profiling): format
14ebd51
@JonasBa
ref(profiling): format
9f9f462
@JonasBa
ref(profiling): format lint and rename enum
fd47fad
AbhiPrasad
AbhiPrasad
reviewed
Jun 4, 2024
View reviewed changes
@AbhiPrasadAbhiPrasad
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Comment threadpackages/profiling-node/src/integration.ts
Outdated

AbhiPrasad
AbhiPrasad
approved these changes
Jun 6, 2024
View reviewed changes
@AbhiPrasadAbhiPrasad
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we've given everyone time to review if need be, let's not wait longer.
LGTM!
@JonasBa
JonasBa
commented
Jun 6, 2024

Copy link


Copy Markdown

ContributorAuthor
Thanks @AbhiPrasad. I'm happy to make changes down the line if we need to as well
@JonasBa
JonasBa
merged commit cecb0d7
into
developJun 6, 2024
@JonasBa
JonasBa
deleted the

jb/profiling/continuous-profiling

branch
June 6, 2024 18:06
billyvg pushed a commit
that referenced
this pull request
Jun 10, 2024
@JonasBa@billyvg
feat(node): add continuous profiling mode (#12124)
cc3be00
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
@AbhiPrasadAbhiPrasad
mentioned this pull request
Jun 11, 2024

Closed

3 tasks
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers


1 more reviewer

@AbhiPrasadAbhiPrasadAbhiPrasad approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@JonasBa@lforst@AbhiPrasad

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
feat(node): add continuous profiling mode - #12124
Merged
JonasBa merged 54 commits into
developfrom 
jb/profiling/continuous-profiling
Jun 6, 2024
Merged
feat(node): add continuous profiling mode#12124
JonasBa merged 54 commits into
developfrom 
jb/profiling/continuous-profiling
Conversation
@JonasBa
@JonasBaJonasBa
commented
May 20, 2024•

edited

Loading

Copy link


Copy Markdown

Contributor
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
@github-actions
github-actionsBot
commented
May 21, 2024•

edited

Loading

Copy link


Copy Markdown

Contributor
size-limit report 📦
PathSize
@sentry/browser21.74 KB (+0.04% 🔺)
@sentry/browser (incl. Tracing)32.78 KB (+0.03% 🔺)
@sentry/browser (incl. Tracing, Replay)68.35 KB (+0.02% 🔺)
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags61.66 KB (+0.02% 🔺)
@sentry/browser (incl. Tracing, Replay with Canvas)72.41 KB (+0.02% 🔺)
@sentry/browser (incl. Tracing, Replay, Feedback)84.51 KB (+0.01% 🔺)
@sentry/browser (incl. Tracing, Replay, Feedback, metrics)86.37 KB (+0.01% 🔺)
@sentry/browser (incl. metrics)25.93 KB (+0.04% 🔺)
@sentry/browser (incl. Feedback)37.9 KB (+0.03% 🔺)
@sentry/browser (incl. sendFeedback)26.33 KB (+0.04% 🔺)
@sentry/browser (incl. FeedbackAsync)30.87 KB (+0.03% 🔺)
@sentry/react24.52 KB (+0.04% 🔺)
@sentry/react (incl. Tracing)35.82 KB (+0.02% 🔺)
@sentry/vue25.74 KB (+0.04% 🔺)
@sentry/vue (incl. Tracing)34.61 KB (+0.02% 🔺)
@sentry/svelte21.87 KB (+0.04% 🔺)
CDN Bundle23.12 KB (+0.04% 🔺)
CDN Bundle (incl. Tracing)34.51 KB (+0.02% 🔺)
CDN Bundle (incl. Tracing, Replay)68.44 KB (+0.01% 🔺)
CDN Bundle (incl. Tracing, Replay, Feedback)73.61 KB (+0.02% 🔺)
CDN Bundle - uncompressed68.02 KB (+0.04% 🔺)
CDN Bundle (incl. Tracing) - uncompressed102.2 KB (+0.03% 🔺)
CDN Bundle (incl. Tracing, Replay) - uncompressed212.09 KB (+0.02% 🔺)
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed224.56 KB (+0.02% 🔺)
@sentry/nextjs (client)35.17 KB (+0.03% 🔺)
@sentry/sveltekit (client)33.41 KB (+0.03% 🔺)
@sentry/node115.25 KB (+0.01% 🔺)
@sentry/node - without tracing94.56 KB (+0.01% 🔺)
@sentry/aws-serverless103.73 KB (+0.01% 🔺)
@JonasBa
JonasBa
marked this pull request as ready for review
May 21, 2024 20:32
@lforst
lforst
commented
May 22, 2024

Copy link


Copy Markdown

Contributor
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.
@JonasBa
JonasBa
commented
May 22, 2024

Copy link


Copy Markdown

ContributorAuthor
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
AbhiPrasad
self-requested a review
June 3, 2024 14:40
@JonasBaJonasBa
mentioned this pull request
Jun 3, 2024

Closed

5 tasks
AbhiPrasad
AbhiPrasad
reviewed
Jun 4, 2024
View reviewed changes
@AbhiPrasadAbhiPrasad
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After this merges in we def also need an e2e test.
Comment threadpackages/profiling-node/src/cpu_profiler.ts
Outdated

Comment threadpackages/profiling-node/src/integration.ts
Outdated

Comment threadpackages/profiling-node/src/integration.ts
Comment threadpackages/profiling-node/src/integration.ts
Comment threadpackages/profiling-node/src/integration.ts
Outdated

Comment threadpackages/profiling-node/src/utils.ts
Comment threadpackages/profiling-node/src/utils.ts
Comment threadpackages/profiling-node/bindings/cpu_profiler.cc
Outdated

Comment threadpackages/profiling-node/bindings/cpu_profiler.cc
Comment threadpackages/profiling-node/bindings/cpu_profiler.cc
JonasBa
added 16 commits
June 4, 2024 11:47
@JonasBa
ref(profiling): use an enum and overload the signature
3529f86
@JonasBa
ref(profiling): use enum to stop profile
cab74a8
@JonasBa
ref(profiling): dont pass undefined to maybeProfileSpan
23c154d
@JonasBa
ref(profiling): single init point for chunk
e77d8ae
@JonasBa
ref(profiling): clearer timestamp namings
69ecbb8
@JonasBa
ref(profiling): clear timeout if present
a1de1c7
@JonasBa
ref(profiling): use ProfilingIntegration as generic for getIntegratio…
b860f3a
…nByName
@JonasBa
ref(profiling): fixup usage of profiler
9b490f3
@JonasBa
ref(profiling): ignore conforming to the signature in binding return …
d09aeab
…type
@JonasBa
ref(profiling): test exclusive modes
17a4c57
@JonasBa
ref(profiling): use LRU map to store profiles before their parent txn…
f4ab018
… or span terminates
@JonasBa
ref(profiling): use LRU map to store profiles before their parent txn…
d24ad25
… or span terminates
@JonasBa
Merge branch 'develop' into jb/profiling/continuous-profiling
440b1bf
@JonasBa
ref(profiling): format
14ebd51
@JonasBa
ref(profiling): format
9f9f462
@JonasBa
ref(profiling): format lint and rename enum
fd47fad
AbhiPrasad
AbhiPrasad
reviewed
Jun 4, 2024
View reviewed changes
@AbhiPrasadAbhiPrasad
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Comment threadpackages/profiling-node/src/integration.ts
Outdated

AbhiPrasad
AbhiPrasad
approved these changes
Jun 6, 2024
View reviewed changes
@AbhiPrasadAbhiPrasad
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we've given everyone time to review if need be, let's not wait longer.
LGTM!
@JonasBa
JonasBa
commented
Jun 6, 2024

Copy link


Copy Markdown

ContributorAuthor
Thanks @AbhiPrasad. I'm happy to make changes down the line if we need to as well
@JonasBa
JonasBa
merged commit cecb0d7
into
developJun 6, 2024
@JonasBa
JonasBa
deleted the

jb/profiling/continuous-profiling

branch
June 6, 2024 18:06
billyvg pushed a commit
that referenced
this pull request
Jun 10, 2024
@JonasBa@billyvg
feat(node): add continuous profiling mode (#12124)
cc3be00
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
@AbhiPrasadAbhiPrasad
mentioned this pull request
Jun 11, 2024

Closed

3 tasks
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers


1 more reviewer

@AbhiPrasadAbhiPrasadAbhiPrasad approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@JonasBa@lforst@AbhiPrasad