-
Notifications
You must be signed in to change notification settings - Fork 157
✨ [RUM-10145] collect errors on module evaluation #3622
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
Conversation
8a8b156
to
fbe5f7b
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3622 +/- ##
==========================================
- Coverage 92.21% 92.21% -0.01%
==========================================
Files 329 331 +2
Lines 8209 8257 +48
Branches 1856 1864 +8
==========================================
+ Hits 7570 7614 +44
- Misses 639 643 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
b25932e
to
80e46f3
Compare
} | ||
const { stop: stopInstrumentingOnError } = instrumentOnError(handleRuntimeError) | ||
const { stop: stopInstrumentingOnUnhandledRejection } = instrumentUnhandledRejection(handleRuntimeError) | ||
export function trackRuntimeError() { |
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.
💬 suggestion: Why not using BufferedObservable directly in here.
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.
I tried, but:
- It makes a circular dependency between
trackRuntimeError
andbufferedData
- The "Buffered Data" concept leaks everywhere in
trackRuntimeError
. It makes it more complex as it's not scoped torawError
anymore.
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.
Hum, fine by me! Let’s move forward and maybe refine the design later as this pattern is used more widely :)
80e46f3
to
8db9e24
Compare
Bundles Sizes Evolution
🚀 CPU Performance
🧠 Memory Performance
|
8db9e24
to
bd3bfb1
Compare
bd3bfb1
to
a4f4d32
Compare
/to-staging |
View all feedbacks in Devflow UI.
Commit a4f4d328cc will soon be integrated into staging-27.
Commit a4f4d328cc has been merged into staging-27 in merge commit 2da91af4e6. Check out the triggered pipeline on Gitlab 🦊 If you need to revert this integration, you can use the following command: |
Integrated commit sha: a4f4d32 Co-authored-by: BenoitZugmeyer <[email protected]>
import type { RawError } from './error/error.types' | ||
import { trackRuntimeError } from './error/trackRuntimeError' | ||
|
||
const BUFFER_LIMIT = 500 |
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.
💭 suggestion : Could we put a single const for the buffer limit or is it expected since the other is deprecated?
(see boundedBuffer.ts)
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.
I'd like to keep BufferedObservable
as generic as possible to encourage using it in more places. But I could add a default value in the BufferedObservable
constructor, to keep it configurable while not having to define a limit every single time when it doesn't matter much. WDYT?
* work even if this method isn't called, but still useful to clarify our intent and lowering our | ||
* memory impact. | ||
*/ | ||
unbuffer() { |
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.
❓ question: could the unbuffer
be done without an explicit call after the first subscribe
?
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.
It could for now, as it is subscribed only once. But in the future I expect this observable to be used in multiple places where the buffer is leveraged (ex: we could imagine using the observable to generate resources, actions... not just errors).
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.
Great work!
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
/to-staging |
View all feedbacks in Devflow UI.
Commit cafe16b8e6 will soon be integrated into staging-29.
Commit cafe16b8e6 had already been merged into staging-29 If you need to revert this integration, you can use the following command: |
/merge |
View all feedbacks in Devflow UI.
The expected merge time in
|
Motivation
Currently, the RUM and Logs Browser SDK are starting to collect errors when the following conditions are met:
init()
was calledtrackingConsent
init parameter or whensetTrackingConsent(‘granted’)
is called)trackViewsManually
is used)To improve observability coverage, we want to collect errors earlier, and the easiest way to do so is to start collecting errors on module evaluation, buffer them, and send them when RUM is ready.
This is a first step. We chose to start with runtime errors first as it was deemed to be the most valuable without much drawback, but our approach needs to be easily extendable to collect other kind of values.
Also, in the future this mechanism could be separated from the main SDK to allow customer inline this code in their HTML to cover even more ground, mitigating the performance impact of inlining the whole SDK entirely.
Changes
Please review commit by commit.
We start by introducing a
BufferedObserver
, which is intended to be used for other purposes and eventually replaceBoundedBuffer
.After a bit of refactoring in tests, we implement early runtime error collection in RUM and Logs by tracking runtime errors and putting them in a
BufferedObserver
instance.Test instructions
In the sandbox (which uses a sync CDN setup), throw an exception before calling init, ex:
The error should be collected.
Checklist