Conversation
|
I have read the CLA Document and I hereby sign the CLA You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot. |
Bundles Sizes Evolution
🚀 CPU Performance
🧠 Memory Performance
|
|
✅ Tests 🎉 All green!❄️ No new flaky tests detected 🎯 Code Coverage (details) 🔗 Commit SHA: 169989b | Docs | Datadog PR Page | Was this helpful? Give us feedback! |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f4e10304d9
ℹ️ 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".
| no_active_view: isResult && 'no_active_view' in result, | ||
| overwritten: isResult && 'overwritten' in result && result.overwritten !== false, |
There was a problem hiding this comment.
Default telemetry flags to false for buffered calls
When addViewLoadingTime() is called before init, strategy.addLoadingTime(...) returns undefined, so isResult && ... evaluates to undefined for both no_active_view and overwritten. Those fields are then omitted from the telemetry payload instead of being sent as explicit booleans, which contradicts the AddViewLoadingTime usage schema and makes pre-start telemetry inaccurate. Please coerce these expressions to false when no result is available.
Useful? React with 👍 / 👎.
Add a new public API `DD_RUM.addViewLoadingTime(options?)` that allows
developers to manually report when a view has finished loading.
- Computes loading time as elapsed time since view start
- Suppresses auto-detected loading time on first manual call
- First-call-wins by default; `{ overwrite: true }` to replace
- Pre-start buffering with preserved call timestamps
- Telemetry usage tracking
- Gracefully no-ops on ended/expired views
- Unit tests + E2E test
f4e1030 to
169989b
Compare
| }, | ||
| addLoadingTime(callTimestamp?: TimeStamp, overwrite = false) { | ||
| if (endClocks) { | ||
| return { no_active_view: true as const } |
There was a problem hiding this comment.
🔨 warning: These are not following the standard naming convention. we use camelCase and convert what need to be to pascal_case just before sending the event.
| const hadPreviousManual = isLoadingTimeSetManually | ||
| const hadAutoValue = !isLoadingTimeSetManually && commonViewMetrics.loadingTime !== undefined | ||
|
|
||
| // Stop auto-detection entirely on first manual call (CONTEXT decision) |
There was a problem hiding this comment.
❓ question: what does (CONTEXT decision) means?
| isLoadingTimeSetManually = true | ||
| commonViewMetrics.loadingTime = loadingTime | ||
|
|
||
| // Return overwrite source for Phase 2 telemetry |
There was a problem hiding this comment.
❓ question: what's Phase 2 telemetry?
| addTelemetryUsage({ | ||
| feature: 'addViewLoadingTime', | ||
| no_view: false, | ||
| no_active_view: isResult && 'no_active_view' in result, | ||
| overwritten: isResult && 'overwritten' in result && result.overwritten !== false, | ||
| }) |
There was a problem hiding this comment.
❓ question: TelemetryUsage is meant to understand which API the customer are using and with which option when available. I don't think we should return run-time analysis like this.
| addTelemetryUsage({ | |
| feature: 'addViewLoadingTime', | |
| no_view: false, | |
| no_active_view: isResult && 'no_active_view' in result, | |
| overwritten: isResult && 'overwritten' in result && result.overwritten !== false, | |
| }) | |
| addTelemetryUsage({ | |
| feature: 'addViewLoadingTime', | |
| overwrite | |
| }) |
| * elapsed time since the view started. By default, the first call sets the loading time and | ||
| * subsequent calls are no-ops. Use `{ overwrite: true }` to replace a previously set value. | ||
| * | ||
| * See [Override RUM View Loading Time](https://docs.datadoghq.com/real_user_monitoring/browser/advanced_configuration/) for further information. |
There was a problem hiding this comment.
🔨 warning: "Override RUM View Loading Time" does not exist at that link
| /** | ||
| * Schema of browser specific features usage | ||
| */ | ||
| // [MANUAL] AddViewLoadingTime added pending upstream rum-events-format schema sync |
There was a problem hiding this comment.
❓ question: I did not find any PR related to that in rum-event-format
| const result = strategy.addLoadingTime(callTimestamp, options?.overwrite ?? false) | ||
|
|
||
| // For pre-start buffered calls, result is undefined so we emit best-guess default values. | ||
| const isResult = result && typeof result === 'object' |
There was a problem hiding this comment.
💬 suggestion: use Types to prevent runtime assertion
Motivation
Allow developers to manually report when a view has finished loading, instead of relying solely on auto-detection. This is needed for complex async loading patterns (e.g., SPAs with deferred data fetching, skeleton screens) where the SDK's heuristic-based auto-detection cannot accurately determine when the view is truly ready.
This brings the Browser SDK to parity with the iOS and Android SDKs which already expose this API.
Changes
Adds a new public API
DD_RUM.addViewLoadingTime(options?)that:{ overwrite: true }to replace a previously set valueinit()are buffered and drained with preserved timestamps)addViewLoadingTimefeature)Files changed:
packages/core/src/domain/telemetry/telemetryEvent.types.tsAddViewLoadingTimeto browser telemetry unionpackages/rum-core/src/domain/view/viewMetrics/trackCommonViewMetrics.tssetManualLoadingTime()with auto-detection suppressionpackages/rum-core/src/domain/view/trackViews.tsaddLoadingTime()onnewViewwith overwrite policypackages/rum-core/src/boot/startRum.tsaddLoadingTimethroughstartRumEventCollectionpackages/rum-core/src/boot/rumPublicApi.tspackages/rum-core/src/boot/preStartRum.tsTest instructions
Running tests
Checklist