-
Notifications
You must be signed in to change notification settings - Fork 142
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
✨ [RUM-5778] Custom Vitals Collection V3 #2929
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2929 +/- ##
==========================================
- Coverage 93.47% 93.43% -0.04%
==========================================
Files 271 271
Lines 7616 7636 +20
Branches 1696 1704 +8
==========================================
+ Hits 7119 7135 +16
- Misses 497 501 +4 ☔ View full report in Codecov by Sentry. |
Bundles Sizes Evolution
🚀 CPU Performance
🧠 Memory Performance
|
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: If i understand correctly, custom vitals are used in web-ui already, and this PR introduce a minor breaking change (renaming details
to description
).
How do we plan to release this?
Co-authored-by: Aymeric <[email protected]>
* Stop a custom duration vital | ||
* stored in @vital.custom.<name> | ||
* | ||
* @param name name of the custom vital | ||
* @param nameOrRef name of the custom vital or the reference to it |
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: We could give more detail on when to use the ref or the name and the limitations. Maybe say to favour refs to avoid any concurrent vital issues.
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.
Done
}) | ||
|
||
it('should not create multiple duration vitals by calling "stop" on the same vital instance multiple times', () => { | ||
it('should not create multiple duration vitals by calling "stopDurationVital" on the same vital multiple times', () => { |
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: It would be clearer if you split this test. One when using refs and another when using names
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.
*split 😉
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.
Done
@@ -30,78 +32,112 @@ describe('vitalCollection', () => { | |||
}) | |||
|
|||
describe('custom duration', () => { | |||
describe('createVitalInstance', () => { | |||
it('should create duration vital from a vital instance', () => { | |||
describe('startDurationVital', () => { |
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: You could add a test explaining what happens when creating multiple vitals with the same name and using stop with the name.
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.
Done
const vital = vitalCollection.startDurationVital({ | ||
name: 'foo', | ||
it('should create a vital from start API using name', () => { | ||
vitalCollection.startDurationVital('foo', { |
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: group vitalCollection tests together with a describe('startVitalCollection'
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.
Done
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.
Mmh what confused me is that we have tests for both startDurationVital
and vitalCollection.startDurationVital
at the same level. I would expect the module-level startDurationVital
tests to be independent from tests needing startVitalCollection
.
Motivation
We would like to be able to report concurrent vitals with the same name and allowing better visualisations. And for better flexibility, we also want to be able to use global vitals.
Changes
Update custom vital structure:
The custom vital value is now in
duration
field instead of incustom
object.Update custom vital API:
startDurationVital
/stopDurationVital
can be used to:To track performance accros components.
To track performance of multiple instance the same components.
Also introducing
addDurationVital
to create a vital with a custom duration and startVital values.All those methods now have a
description
attribute that can be used to differentiate vitals using the same name.Testing
I have gone over the contributing documentation.