-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(gettingstarted): clean up new profiling snippets #81231
base: master
Are you sure you want to change the base?
fix(gettingstarted): clean up new profiling snippets #81231
Conversation
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## master #81231 +/- ##
=======================================
Coverage 80.34% 80.34%
=======================================
Files 7219 7220 +1
Lines 319536 319591 +55
Branches 20779 20787 +8
=======================================
+ Hits 256735 256783 +48
- Misses 62407 62414 +7
Partials 394 394 |
… in python and nodejs
Bundle ReportChanges will increase total bundle size by 18.28kB (0.06%) ⬆️. This is within the configured threshold ✅ Detailed changes
|
const steps = [ | ||
...doc.install(docParams), | ||
...doc.configure(docParams), | ||
...doc.verify(docParams), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very good point, I was under the impression that the existence of the profilingOnboarding
attribute in the Docs object indicates that it will always be used and separately created, but we fallback to the normal docs in case there is no separate profiling onboarding.
Sentry.profiler.stopProfiler(); | ||
} | ||
// Start a span | ||
await Sentry.startSpan({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Won't this complain that await is only valid in async functions? Or is the validate more for illustration purposes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The snippet definitely works - the createServer is setup in async, so if this is pasted into the server snippet, everything falls into place. Is there a nicer way to do the setTimeout that doesn't need us to do this async?
sentry_sdk.profiler.stop_profiler()` | ||
: '' | ||
}`; | ||
sentry_sdk.profiler.stop_profiler()`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to import sentry_sdk first?
@@ -138,7 +138,10 @@ const onboarding: OnboardingConfig = { | |||
description: t( | |||
'Raise an unhandled Python exception by inserting a divide by zero expression into your application:' | |||
), | |||
code: 'division_by_zero = 1 / 0', | |||
code: `${hasContinuousProfiling(params) ? `# Verify profiling functionality ${getProfilingVerifySnippet()}` : ''} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we first verify errors and then after it profiling?
configurations: [ | ||
{ | ||
language: 'python', | ||
code: getProfilingVerifySnippet(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc. @Zylphrex
This pull request has gone three weeks without activity. In another week, I will close it. But! If you comment or otherwise update it, I will reset the clock, and if you add the label "A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀 |
Before:
After: