-
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-4360] monitor more API usages #2745
Conversation
b63a18f
to
f480215
Compare
Bundles Sizes Evolution
🚀 CPU Performance
|
/to-staging |
🚂 Branch Integration: starting soon, merge in < 0s Commit f480215f9d will soon be integrated into staging-19. This build is going to start soon! (estimated merge in less than 0s) Use |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2745 +/- ##
=======================================
Coverage 93.27% 93.28%
=======================================
Files 241 241
Lines 7034 7044 +10
Branches 1554 1554
=======================================
+ Hits 6561 6571 +10
Misses 473 473 ☔ View full report in Codecov by Sentry. |
🚂 Branch Integration: This commit was successfully integrated Commit f480215f9d has been merged into staging-19 in merge commit 8255f230bf. Check out the triggered pipeline on Gitlab 🦊 |
f480215
to
bbbbef0
Compare
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.
LGTM
@@ -296,6 +308,7 @@ export function makeRumPublicApi(startRumImpl: StartRum, recorderApi: RecorderAp | |||
*/ | |||
addFeatureFlagEvaluation: monitor((key: string, value: any) => { | |||
strategy.addFeatureFlagEvaluation(sanitize(key)!, sanitize(value)) | |||
addTelemetryUsage({ feature: 'add-feature-flag-evaluation' }) |
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.
💭 thought: almost all usages of addTelemetryUsage
is only providing a feature name. It could be shorter to change the signature to something like:
addTelemetryUsage("feature", { optional_context: true })
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.
when introducing the API, I spent some time trying something like this but did not managed to have the typings working.
Don't hesitate to have a shot at it, happy to tweak that in a following PR.
Motivation
Provide more visibility on how some SDK APIs are used
Changes
Monitor usage of:
Testing
I have gone over the contributing documentation.