-
Notifications
You must be signed in to change notification settings - Fork 76
Introduce vitalAppLaunchEventMapper #2987
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
base: aleksandr-gringauz/feature/app-launch-vitals
Are you sure you want to change the base?
Introduce vitalAppLaunchEventMapper #2987
Conversation
…launch-vitals-new-rum-schema Include new RUM Schema changes to app launch feature branch
|
🎯 Code Coverage 🔗 Commit SHA: 58ca36d | Docs | Datadog PR Page | Was this helpful? Give us feedback! |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## aleksandr-gringauz/feature/app-launch-vitals #2987 +/- ##
================================================================================
+ Coverage 70.99% 71.03% +0.05%
================================================================================
Files 831 831
Lines 30659 30666 +7
Branches 5216 5217 +1
================================================================================
+ Hits 21764 21783 +19
- Misses 7399 7400 +1
+ Partials 1496 1483 -13
🚀 New features to boost your workflow:
|
| fun setVitalOperationStepEventMapper(com.datadog.android.event.EventMapper<com.datadog.android.rum.model.RumVitalOperationStepEvent>): Builder | ||
| fun setVitalAppLaunchEventMapper(com.datadog.android.event.EventMapper<com.datadog.android.rum.model.RumVitalAppLaunchEvent>): Builder |
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'm wondering if we want to expose them in such shape. Maybe we can group them and introduce a single method here withVitalEventMappers(operationsMapper = ..., appLaunchMapper = ..., ....).
With current layout these methods seems to be very verbose among the other methods in this Builder.
Maybe we need to ask iOS SDK what will be their approach regarding this as well.
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.
Or maybe we should revert to setVitaEventMapper and use them for the all the vital events 🙂
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.
@satween What arguments should setVitaEventMapper have? Same as @0xnm suggested?
I was thinking about something like this: https://pl.kotl.in/nNeN0bmfx
Note that currently VitalEvent from this example doesn't exist in our code. OperationStepVital and AppLaunchVital don't have a common parent type. We could change it though.
But I still lean towards the solution with a separate method per vital type.
We started discussing it with @simaoseica-dd, but not finished yet.
84212fe to
58ca36d
Compare
1c884cb to
39cc533
Compare
What does this PR do?
Adding
vitalAppLaunchEventMappersimilar tovitalOperationStepEventMappernow that app launch vital events have a separate type.Motivation
What inspired you to submit this pull request?
Additional Notes
Anything else we should know when reviewing?
Review checklist (to be filled by reviewers)