♻️ Replace longTaskRegistry by longTaskContexts#4013
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
✅ Tests 🎉 All green!❄️ No new flaky tests detected 🎯 Code Coverage 🔗 Commit SHA: 847e3b4 | Docs | Datadog PR Page | Was this helpful? Give us feedback! |
aef5b24 to
55c710a
Compare
Bundles Sizes Evolution
🚀 CPU Performance
🧠 Memory Performance
|
55c710a to
54300b7
Compare
|
|
||
| export const startProfilingContext = (hooks: Hooks): ProfilingContextManager => { | ||
| // Default status is `starting`. | ||
| export function startProfilingContext(hooks: Hooks): ProfilingContextManager { |
There was a problem hiding this comment.
Unrelated but use classic function declaration for consistency
361628c to
08d1b6b
Compare
643752a to
cfac54a
Compare
cfac54a to
847e3b4
Compare
thomasbertet
left a comment
There was a problem hiding this comment.
LGTM overall !
Added a nit on naming, but otherwise I believe that's a great refactoring!
| export interface LongTaskContext { | ||
| id: string | ||
| startClocks: ClocksState | ||
| duration: Duration | ||
| entryType: RumPerformanceEntryType.LONG_ANIMATION_FRAME | RumPerformanceEntryType.LONG_TASK | ||
| } | ||
|
|
||
| export interface LongTaskContexts { | ||
| findLongTasks: (startTime: RelativeTime) => LongTaskContext[] | ||
| } |
There was a problem hiding this comment.
Is this s plural to represent the API object a convention ? Otherwise I can suggest :
export interface LongTaskEntry {
id: string
startClocks: ClocksState
duration: Duration
entryType: RumPerformanceEntryType.LONG_ANIMATION_FRAME | RumPerformanceEntryType.LONG_TASK
}and
export interface LongTaskContext {
findLongTasks: (startTime: RelativeTime) => LongTaskEntry[]
}I'd find this a bit more clear - but it may not be following existing convention so overall I trust what you think is best !
|
/to-staging |
|
View all feedbacks in Devflow UI.
Commit 847e3b43d6 will soon be integrated into staging-50.
Commit 847e3b43d6 has been merged into staging-50 in merge commit 14ec0770a2. Check out the triggered pipeline on Gitlab 🦊 If you need to revert this integration, you can use the following command: |
Integrated commit sha: 847e3b4 Co-authored-by: amortemousque <aymeric.mortemousque@datadoghq.com>
Motivation
Remove longTaskRegistry and use a new longTaskContext exposed by longTaskCollection. This keeps the codebase consistent and preserves the bundle size.
Changes
longAnimationFrameCollectionintolongTaskCollection, since long tasks act as the fallback for LOAF.longTaskContextsfromlongTaskCollection.longTaskRegistrywithlongTaskContextsin profiling.longTaskRegistryandprofilingCorrelationmodules.Checklist