-
Notifications
You must be signed in to change notification settings - Fork 162
✨⚗️[RUM-11398] Add feature operation step vital APIs #3804
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
4001e3a
to
1fe1863
Compare
✅ Tests 🎉 All green!❄️ No new flaky tests detected 🎯 Code Coverage 🔗 Commit SHA: 481fce8 | Docs | Was this helpful? Give us feedback! |
Bundles Sizes Evolution
🚀 CPU Performance
🧠 Memory Performance
|
7cd5619
to
494418a
Compare
/to-staging |
View all feedbacks in Devflow UI.
Commit 494418a708 will soon be integrated into staging-35.
Commit 494418a708 has been merged into staging-35 in merge commit 2764d2b44d. Check out the triggered pipeline on Gitlab 🦊 If you need to revert this integration, you can use the following command: |
494418a
to
714e048
Compare
Integrated commit sha: 494418a Co-authored-by: cy-moi <[email protected]>
17cda02
to
acc2518
Compare
@cy-moi rebase and let's see those e2e errors are gone |
acc2518
to
8361589
Compare
export interface FeatureOperationOptions extends VitalOptions { | ||
operationKey?: string | ||
} | ||
export interface FullFeatureOperationOptions extends FeatureOperationOptions { |
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: Rename FullFeatureOperationOptions
to FailFeatureOperationOptions
succeedFeatureOperation: monitor((name, options) => { | ||
addTelemetryUsage({ feature: 'add-operation-step-vital', action_type: 'succeed' }) | ||
strategy.addOperationStepVital(name, 'end', options) | ||
}), | ||
|
||
failFeatureOperation: monitor((name, options) => { | ||
addTelemetryUsage({ feature: 'add-operation-step-vital', action_type: 'fail' }) | ||
strategy.addOperationStepVital(name, 'end', options) | ||
}), |
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: how do you distinguish between succeed
and fail
if the user is not providing a failureReason
?
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.
Indeed, failureReason
should be required.
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 made the type changes (mentioned in comments below) based on this as well.
a0de1e8
to
6c1022f
Compare
6c1022f
to
7723cca
Compare
vital: vitalData, | ||
type: RumEventType.VITAL, | ||
context, | ||
...(valueComputedBySdk && { _dd: { vital: { computed_value: 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.
nitpick: I'm not sure what valueComputedBySdk
is, but it looks like it's always true
, so maybe we can simplify?
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.
Ah wait, this one should be false when customers use addDurationVital
and compute their own values right? I think we can either remove this or we change how start
or stop
calling addDurationVital
under the hood to pass in valueComputedBySdk
.
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.
Per discussion on slack, we are removing this value because it was only relevant to the pre-GA vital and is no longer in use.
8560e14
to
e404bf9
Compare
/to-staging |
View all feedbacks in Devflow UI.
Commit 481fce8c3e will soon be integrated into staging-38.
Commit 481fce8c3e has been merged into staging-38 in merge commit a25fe54e56. Check out the triggered pipeline on Gitlab 🦊 If you need to revert this integration, you can use the following command: |
Integrated commit sha: 481fce8 Co-authored-by: cy-moi <[email protected]>
* Add feature operation step vital apis * update sanitzation * Update API signature * Update specific types and jsdocs * Make failureReason required * Update packages/rum-core/src/boot/rumPublicApi.ts Co-authored-by: Benoît <[email protected]> * fix-comment: remove computedValueBySdk * Update rum-event-format and remove unused ff * Leverage type from schema updates --------- Co-authored-by: Benoît <[email protected]>
Motivation
Expose feature operation step vitals public APIs
Changes
FeatureOperation
Apis inRumPublicApi
vitalCollection
Test instructions
Go on staging
Call apis from console
See events sent
Checklist