✨ Add action and vital metadata to profiles#4148
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
Bundles Sizes Evolution
🚀 CPU Performance
🧠 Memory Performance
|
59aa5b7 to
f709051
Compare
|
✨ Fix all issues with BitsAI or with Cursor
|
5059362 to
ac62ddd
Compare
thomasbertet
left a comment
There was a problem hiding this comment.
Looks good to me!
I believe we could have a more generic history combining all of the profiler's tracked events in one place, it would be slightly less code.
| { | ||
| id: vitalStart.id, | ||
| startClocks: vitalStart.startClocks, | ||
| duration: 0 as Duration, |
There was a problem hiding this comment.
I wonder if we should have another value for "non-stopped" events 🤔
Relying on 0 seems like it could be error prone and the BE that process should be aware 0 means "not stopped".
Maybe just undefined would work instead ? WDYT ?
There was a problem hiding this comment.
Good question... could we maybe settle for something like -1, though, to not have to re-do a rum-events-format PR to allow undefined here?
There was a problem hiding this comment.
Yes, but we will need to adjust the logic, I kinda forgot about this when I did the BE logic : https://github.com/DataDog/profiling-backend/pull/7969/changes#diff-fe24f465d52fe19d3a52bfd86ac3eaa5e441e710eca6bb9c26a14cac5ce169e4R339-R346
You can see I'm just using the duration field without taking into account if the duration is 0 or undefined.
Let's have -1 as signal for unfinished events. I'll prepare the BE to support that value.
There was a problem hiding this comment.
Actually maybe that's cleaner if that's undefined, even if we have to do another PR for the format, WDYT ?
There was a problem hiding this comment.
Works for me, I just updated!
ade4eac to
65b9912
Compare
|
/to-staging |
|
View all feedbacks in Devflow UI.
Commit 65b99121e9 will soon be integrated into staging-09.
Commit 65b99121e9 has been merged into staging-09 in merge commit 848b6b79d7. Check out the triggered DDCI request. If you need to revert this integration, you can use the following command: |
65b9912 to
a68787a
Compare
|
/to-staging |
|
View all feedbacks in Devflow UI.
Commit a68787a74c will soon be integrated into staging-09.
Commit a68787a74c has been merged into staging-09 in merge commit 661ba0a622. Check out the triggered DDCI request. If you need to revert this integration, you can use the following command: |
a68787a to
5e88cac
Compare
|
/to-staging -c |
|
View all feedbacks in Devflow UI.
Cannot cancel integration of into staging-09: This merge request was already processed and can't be unqueued anymore. To get help about command usage, write If you need support, contact us on Slack #devflow with those details! |
|
|
||
| lifeCycle.subscribe(LifeCycleEventType.RAW_RUM_EVENT_COLLECTED, ({ rawRumEvent, startClocks, duration }) => { | ||
| if (rawRumEvent.type === 'action') { | ||
| const durationForEntry = duration ?? (0 as Duration) |
There was a problem hiding this comment.
You can use duration! like we do in longTaskHistory
There was a problem hiding this comment.
The thing is that the duration is not always defined here since we can have punctual actions (or actions that are still ongoing) that don't have a duration. In these cases, we do need to default to 0 for the close method of the history, but we keep the undefined duration for the profiling metadata (which is not perfectly reflected, I will update that)
| .add( | ||
| { | ||
| id: rawRumEvent.action.id, | ||
| label: '', |
There was a problem hiding this comment.
❓ question: Is it expected not to have a label? Shouldn't it be the action name?
There was a problem hiding this comment.
The issue here is that actions' names might be redacted by the customers in beforeSend, and we currently have no way of knowing this (or at least no easy way). After talking about this with @thomasbertet, we decided that we would be fine without the action names, but I preferred leaving the field as an empty string here instead of removing it because we will still try to add it later!
There was a problem hiding this comment.
I see. We usually remove unnecessary attributes to avoid confusion. Would it be too much work to remove it?
There was a problem hiding this comment.
Well I was planning to try a follow-up to see if can get the action name in some cases when it's not redacted. Is that okay if I remove it later, only if my follow-up doesn't work?
(Otherwise, I wouldn't say it's too much work, just that we need to do a PR on rum-events-format to remove it, and probably on profiling-backend as well since it expects a label I think)
There was a problem hiding this comment.
Removing it would also need a BE PR to make it optional. Let's keep it for now :)
There was a problem hiding this comment.
I’m not convinced by this decision, but at least add a comment explaining why it’s empty ;)
| .add( | ||
| { | ||
| id: rawRumEvent.action.id, | ||
| label: '', |
There was a problem hiding this comment.
I see. We usually remove unnecessary attributes to avoid confusion. Would it be too much work to remove it?
|
|
||
| lifeCycle.subscribe(LifeCycleEventType.RAW_RUM_EVENT_COLLECTED, ({ rawRumEvent, startClocks, duration }) => { | ||
| if (rawRumEvent.type === 'action') { | ||
| const durationForEntry = duration ?? (0 as Duration) |
430f58a to
2049f75
Compare
…id on start instead of stop
…allowed to be undefined
… histoies, and track click actions with duration
…cle from startDurationVital"
…a different param in addDurationVital
43c6247 to
65a4937
Compare
|
@amortemousque I've just rebased as I had merge conflicts, but you can look only at this commit following our discussion! Also, I've just created a task for the follow-up on preStart |
Motivation
To enable profiling aggregation on the backend for vitals and actions, we want to add vitals and actions data to the metadata of profiling events.
This PR adds these entries to the profiling events metadata.
Changes
vitalHistorynext to the profiler that catches vital events from the life cycle and stores them in the historyactionHistorystartmethod of the event tracker return the stored dataTest instructions
sandbox/react-app/main.tsx, add:to the SDK configuration, and add
before the
returnof theLayoutfunction to have a vital that will be displayed in the collected ones.yarn devand go tolocalhost:8080/react-apphttps://browser-intake-datadoghq.com/api/v2/profilein the network tab (profiles usually last one minute, so this might not come instantly, but you can also edit the constantcollectIntervalMsinprofiler.tsto make it faster)"action"and"vital", both with an object of the shape{ "id": string[], "label": string[] }"action"and"vitals"with the following shape:Checklist