feat: loading benchmark analytic - #9494
Conversation
|
New build in progress, come back later! |
|
Warnings not reduced: 13999 => 13999 — remove at least 1 warning to merge. Warnings/errors in files changed by this PR (7) |
lorenzo-ranciaffi
changed the title
lorenzo-ranciaffi
added
the
no QA needed
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
Code Review — PR #9494: feat: loading benchmark analytic
STEP 2 — Root-cause check
PASS. This PR adds a new CI-only feature for benchmarking loading times — it is solving a genuine observability gap, not patching a symptom.
STEP 3 — Design & integration
Owner search for LoadingTimes (the new orchestrator):
- Entity managed: loading-stage measurement lifecycle.
- Existing owners:
LoadingStatus(drives stage transitions viaSetCurrentStage()),RealUserInAppInitializationFlow(callsSetCurrentStageduring init),ProfilingPlugin(home for profiling/CI concerns, already hostsAutoPilot). - Verdict:
ProfilingPluginis the correct home for this CI-only feature. The subscription toILoadingStatus.CurrentStageMut.OnUpdateis the right integration point — it fires on each stage transition without coupling to the flow that drives them.
However, LoadingTimeSampler is a second unit that exists only as a static delegate of LoadingTimes. It has no independent consumers and no polymorphism. Per CLAUDE.md §11 ("Extracting when you should merge"), it should be merged into LoadingTimes as instance state.
Teardown trace:
+= OnStageUpdated→LoadingTimesconstructor (line 23) ✅-= OnStageUpdated→LoadingTimes.Dispose()(line 28) ✅loadingTimes?.Dispose()→ProfilingPlugin.Dispose()✅⚠️ In practice,Application.Quit()fires inside the callback beforeDispose()is ever reached. The teardown is technically correct but effectively dead code in the CI happy path.
Comparison with AutoPilot (existing CI feature in the same plugin):
AutoPilot uses an async UniTask pattern — it awaits loading completion, runs its work, then quits in a finally block. This gives async operations (like network I/O) time to complete. The new LoadingTimes uses a synchronous event callback that calls Track() then Application.Quit() immediately — this is a critical difference that likely causes analytics data loss (see P1 finding below).
STEP 4 — Member audit
| Member | Consumers | Assessment |
|---|---|---|
LoadingTimeSampler.Sample() |
1 (LoadingTimes.OnStageUpdated) |
Single-use static → merge into LoadingTimes |
LoadingTimeSampler.ToJObject() |
1 (LoadingTimes.OnStageUpdated) |
Single-use static → merge into LoadingTimes |
StageMeasure (struct) |
1 (LoadingTimeSampler) |
Could be private nested type after merge |
STEP 5 — Line-level findings
See inline comments below. Summary of findings:
| # | Severity | File | Issue |
|---|---|---|---|
| 1 | P1 | LoadingTimeSampler.cs:19-36 |
Missing upper-bound check on array access → IndexOutOfRangeException |
| 2 | P1 | LoadingTimeSampler.cs:48-54 |
Iterates uninitialized array entries → ArgumentException on duplicate JObject keys |
| 3 | P1 | LoadingTimes.cs:39-43 |
Application.Quit() immediately after Track() without Flush() → analytics data likely lost |
| 4 | P2 | LoadingTimeSampler.cs:6 |
Entirely static mutable state — merge into LoadingTimes (CLAUDE.md §11) |
| 5 | P2 | LoadingTimes.cs:11 |
Namespace–type name collision (DCL.LoadingTimes.LoadingTimes) |
STEP 5B — Design smells
- Branch name mismatch (ADR-6): Branch is
fix/loading-benchmark-analyticbut PR title correctly usesfeat:. The branch prefix should match —feat/loading-benchmark-analytic.
STEP 6 — Complexity
COMPLEX — introduces new assembly reference, event subscription lifecycle in ProfilingPlugin, modifies plugin constructor signature (affects all callers), and changes the authentication/initialization flow.
STEP 7 — QA
YES — modifies runtime code under Explorer/ that ships in the build. The constructor signature change to ProfilingPlugin and the auth-flow change in RealUserInAppInitializationFlow run in all builds, not just when the flag is set.
STEP 8 — Non-blocking warnings
None.
Security review
No security issues found. The measure-loading-time flag requires command-line access (not a remote attack vector). Auth-screen skip mirrors the existing AUTOPILOT pattern. No secrets or PII exposed in the analytics payload.
REVIEW_RESULT: FAIL ❌
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Introduces new assembly reference, event subscription lifecycle in ProfilingPlugin, modifies plugin constructor signature, and changes auth/init flow.
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by decentraland-bot via GitHub
|
All Unity tests passed ✅
|
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
Code Review — PR #9494: feat: loading benchmark analytic (Re-review)
Previous review findings addressed: The author rewrote the static
LoadingTimeSampler+LoadingTimesinto a single instance-basedLoadingTimeBenchmark, addressing all five prior findings (static mutable state, index-out-of-bounds, uninitialized array iteration, analytics delivery, namespace collision, and single-consumer merge). Well done.
STEP 2 — Root-cause check
PASS. This PR adds a new CI-only feature — a loading-time benchmark activated by the measure-loading-time app arg. It measures startup stage durations, reports them via analytics, and quits. This is genuine new functionality, not a symptom fix.
STEP 3 — Design & integration
PASS.
Owner search for LoadingTimeBenchmark:
- Manages: observation of
ILoadingStatus.CurrentStageMutstage transitions for timing measurement. - Existing owners searched:
ILoadingStatusis created by the container infrastructure and consumed by the loading screen UI. Stage transitions are driven byRealUserInAppInitializationFlowand container initialization. No existing class measures and reports stage durations — this is a new cross-cutting CI instrumentation concern. - Conclusion: No existing owner can host this logic.
ProfilingPluginis the natural home for profiling/diagnostic tools, and conditional creation via the flag keeps it clean.
Teardown trace:
loadingStatus.CurrentStageMut.OnUpdate += OnStageUpdated(constructor, L50) →OnUpdate -= OnStageUpdated(Dispose(), L55) ✅ProfilingPlugin.Dispose()callsloadingTimeBenchmark?.Dispose()✅ReportAndQuitAsync()— detached via.Forget()with proper try/catch/finally per CLAUDE.md §9. However,UniTask.Delayhas no cancellation token — ifDispose()runs during the grace period, the delay continues detached. See P2-1 below.
STEP 4 — Member audit
LoadingTimeBenchmark(ILoadingStatus, IAnalyticsController, IScenesCache)— 1 consumer (ProfilingPluginconstructor). ✅Dispose()— 1 consumer (ProfilingPlugin.Dispose). ✅- All other members are
private. No single-use public accessors. ✅
STEP 5 — Line-level findings
See inline comments. Summary:
| # | Sev | Finding |
|---|---|---|
| 1 | P2 | UniTask.Delay in ReportAndQuitAsync has no CancellationToken — delay continues if Dispose() runs during grace period |
| 2 | P2 | Scene hash read across async boundary in ReportAndQuitAsync — capture synchronously before .Forget() |
| 3 | P2 | Removed AnalyticsEvents XML comment warning about "Refresh Events" button — confirm this step is no longer required |
Security: No issues found. The auth bypass mirrors the existing AUTOPILOT pattern with proper identity-expiry fallback (Application.Quit(1) if identity is null/expired). The analytics payload contains only timing floats and a scene identifier — no PII, no user-controlled input.
STEP 6 — Complexity
SIMPLE. New self-contained CI-only diagnostic class with event subscription and analytics reporting. No ECS systems, no complex async patterns, no cross-world access.
STEP 7 — QA
NO. The feature is gated behind a CI-only measure-loading-time app arg that is never passed in normal usage. The auth flow changes are purely additive flag checks that don't alter default behavior.
STEP 8 — Non-blocking warnings
None.
STEP 9 — Verdict
REVIEW_RESULT: PASS ✅
COMPLEXITY: SIMPLE
COMPLEXITY_REASON: New self-contained CI-only diagnostic class with event subscription and analytics reporting; no ECS, complex async, or architectural complexity.
QA_REQUIRED: NO
Reviewed by Jarvis 🤖 · Requested by lorenzo-ranciaffi via GitHub
| } | ||
|
|
||
| public void Dispose() | ||
| { |
There was a problem hiding this comment.
[P2] UniTask.Delay in ReportAndQuitAsync (L118) runs without a CancellationToken. If Dispose() is called during the 5-second analytics grace period, the delay and the Application.Quit() in finally continue running detached.
Add a CancellationTokenSource field, cancel it in Dispose(), and pass the token to UniTask.Delay. Also add using System.Threading; to imports.
| } | |
| public void Dispose() | |
| { | |
| public void Dispose() | |
| { | |
| loadingStatus.CurrentStageMut.OnUpdate -= OnStageUpdated; | |
| cts.Cancel(); | |
| cts.Dispose(); | |
| } |
Also add the field (after L35):
private readonly CancellationTokenSource cts = new ();And update L118:
await UniTask.Delay(ANALYTICS_DELIVERY_GRACE, cancellationToken: cts.Token);| if (stage != LoadingStatus.LoadingStage.Completed) return; | ||
|
|
||
| reported = true; | ||
| ReportAndQuitAsync().Forget(); |
There was a problem hiding this comment.
[P2] Scene hash read across an async boundary. scenesCache.CurrentScene.Value?.Info.Name is currently read inside ReportAndQuitAsync (L111), after the Completed event fires and across an async transition. The reactive property could theoretically change between the event and the read.
Capture the scene hash synchronously here, before the .Forget() call:
| if (stage != LoadingStatus.LoadingStage.Completed) return; | |
| reported = true; | |
| ReportAndQuitAsync().Forget(); | |
| if (stage != LoadingStatus.LoadingStage.Completed) return; | |
| reported = true; | |
| ReportAndQuitAsync(scenesCache.CurrentScene.Value?.Info.Name).Forget(); |
Then update the method signature and body:
private async UniTaskVoid ReportAndQuitAsync(string? sceneHash)
{
// ...
JObject payload = BuildPayload(sceneHash);This also removes the scenesCache field dependency from the async path.
| /// IMPORTANT!! | ||
| /// </summary> | ||
|
|
||
| public static class AnalyticsEvents |
There was a problem hiding this comment.
[P2] Removed XML comment — confirm this step is obsolete. The deleted comment warned:
After doing any change to the events here, we need to hit the "Refresh Events" button on the AnalyticsConfiguration Scriptable Object so the new events are recognized!!
If this step is still required for the new LOADING_TIMES event to be recognized, the warning should be preserved (or documented elsewhere). If the analytics system no longer uses the AnalyticsConfiguration scriptable object for event registration, the removal is fine.
| public static class AnalyticsEvents |
2 participants
Pull Request Description
What does this PR change?
This PR adds a
measure-loading-timeapp arg intended for CI usage only.The CI will start the client with that flag so that it will log the startup loading times and send them to matrix in order to track how much time (and which stage) it takes for the client to land in GP (the CI will send the user to a dedicated world with a copy of GP so performance doesn't fluctuate based on users, emotes, ... ).
When the client has loaded it shuts down.
Quality Checklist
Code Review Reference
Please review our Branch & PR Standards before submitting. It explains the automated review flow, QA/DEV approval requirements, and what each label does — especially useful for first-time contributors.