chore: Adapt RUM model generation to new Vital schema structure#2548
Conversation
maxep
left a comment
maxep
left a comment
There was a problem hiding this comment.
Looks great! I wonder if the priority ordering is necessary.. I personally don't mind large diff in generated code and the alphabetical ordering is easier to navigate IMO.
It’s doable 👍 no problem, but it would mean merging a single, very large PR to apply this change. |
DataDog/rum-events-format@fe242fe adds: RUMVitalAppLaunchEvent + RUMVitalDurationEvent + RUMVitalOperationStepEvent sorts alphabetically
simaoseica-dd
left a comment
simaoseica-dd
left a comment
There was a problem hiding this comment.
Great work on these two schema PRs! 🎖️
Regarding the priority, it’s subjective. The DataModels are bulky auto-generated files, so using Cmd+F is usually the most helpful approach.
6cd4e9d to
d500e47
Compare
|
@maxep @simaoseica-dd 👌 I removed priority list. Now models are simply sorted alphabetically - this way we will avoid running into same problem in the future. |
mariedm
left a comment
mariedm
left a comment
There was a problem hiding this comment.
Just a small renaming to avoid confusing if you don't mind, otherwise LGMT!
Since I need to update the RUM schema to make the view ID optional for operation steps, I can take care of it when updating the RUM models on iOS too. 👍 |
6633fc4
5 participants
What and why?
📦 This PR updates RUM model generation following the schema changes introduced in rum-events-format#315, where:
RUMVitalEventschema was split into three distinct events:RUMVitalAppLaunchEventRUMVitalDurationEventRUMVitalOperationStepEventRUMTransitionEventwas removed from the mobile schema.💡 The change to the RUM Vital event model impacted the Features Operations code, which required updates to align with the new model. Please pay special attention to this part during the review.
How
Because the new schema lists elements in a different order, regenerating models resulted in an extremely noisy diff. To avoid this problems in the future, the
rum-models-generatornow enforces alphabetical order of models in the file. This partially matches the previous order, reducing the size of diff.Even with this mitigation, the diff remains large — GitHub struggles to compute minimal changes for large generated files and overstates the extent of the modifications.
To make the review easier:
RUMVitalEventandRUMTransitionEvent(done manually). GitHub’s diff view is messy here — I removed consecutive model definitions, but it renders them as scattered changes.Actual diff summary (consecutive lines changed):
I explored a few ways to improve diff readability (custom diff drivers in
.gitattributes, alternative algorithms), but they only work locally. Marking files as linguist-generated would hide them on GitHub, which isn’t ideal since generated code still needs review.Review checklist
make api-surface)