-
Notifications
You must be signed in to change notification settings - Fork 130
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
✨[RUMF-737] migrate resource to v2 format (experimental) #584
Conversation
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.
Looks good!
import { LifeCycleEventType } from '../lifeCycle' | ||
import { setup, TestSetupBuilder } from '../../../../test/specHelper' | ||
import { RumPerformanceEntry } from '../../../browser/performanceCollection' | ||
import { RumEventCategory } from '../../../index' |
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.
We might want to introduce some TS path to avoid deep relative require paths
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.
Yes, it could be nice.
Let's see in which state we are at the end of the migration, we'll probably want to re-challenge the file organization.
It could be easier to define the right relative path at this point.
handleRequest(lifeCycle, session, handler, request) | ||
if (session.isTrackedWithResource()) { | ||
lifeCycle.notify(LifeCycleEventType.RAW_RUM_EVENT_COLLECTED, processRequest(request)) | ||
lifeCycle.notify(LifeCycleEventType.RESOURCE_ADDED_TO_BATCH) |
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.
NIT for another PR: RESOURCE_ADDED_TO_BATCH
could probably be replaced with a RAW_RUM_EVENT_COLLECTED
usage + test on evt.category
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.
yep, I've listed some potential cleanup here
packages/rum/src/domain/rumEventsCollection/resource/resourceCollection.spec.ts
Outdated
Show resolved
Hide resolved
Codecov Report
@@ Coverage Diff @@
## master #584 +/- ##
==========================================
- Coverage 88.13% 88.07% -0.06%
==========================================
Files 46 47 +1
Lines 2359 2382 +23
Branches 492 493 +1
==========================================
+ Hits 2079 2098 +19
- Misses 280 284 +4
Continue to review full report at Codecov.
|
Motivation
Following #570, migrating resource to v2 format
Changes
Testing
unit tests, staging
I have gone over the contributing documentation.