feat: loading benchmark analytic

[lorenzo-ranciaffi]

Pull Request Description

What does this PR change?

This PR adds a measure-loading-time app 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

• Changes have been tested locally
• Documentation has been updated (if required)
• Performance impact has been considered
• For SDK features: Test scene is included

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.

[github-actions]

New build in progress, come back later!

[github-actions]

Lint did not finish (failure) — the warning ratchet could not be evaluated. See logs.

[decentraland-bot]

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 via SetCurrentStage()), RealUserInAppInitializationFlow (calls SetCurrentStage during init), ProfilingPlugin (home for profiling/CI concerns, already hosts AutoPilot).
• Verdict: ProfilingPlugin is the correct home for this CI-only feature. The subscription to ILoadingStatus.CurrentStageMut.OnUpdate is 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 → LoadingTimes constructor (line 23) ✅
• -= OnStageUpdated → LoadingTimes.Dispose() (line 28) ✅
• loadingTimes?.Dispose() → ProfilingPlugin.Dispose() ✅
• ⚠️ In practice, Application.Quit() fires inside the callback before Dispose() 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-analytic but PR title correctly uses feat:. 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

[github-actions]

⚠️ EditMode produced no results — the run likely crashed or timed out before finishing. Check the Unity Test / Test (editmode) job.

⚠️ PlayMode produced no results — the run likely crashed or timed out before finishing. Check the Unity Test / Test (playmode) job.

TESTS SUITE	Result	Passed	Failed	Skipped
EditMode	⚠️ No results	—	—	—
PlayMode	⚠️ No results	—	—	—

[decentraland-bot]

Code Review — PR #9494: feat: loading benchmark analytic (Re-review)

Previous review findings addressed: The author rewrote the static LoadingTimeSampler + LoadingTimes into a single instance-based LoadingTimeBenchmark, 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.CurrentStageMut stage transitions for timing measurement.
• Existing owners searched: ILoadingStatus is created by the container infrastructure and consumed by the loading screen UI. Stage transitions are driven by RealUserInAppInitializationFlow and 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. ProfilingPlugin is 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() calls loadingTimeBenchmark?.Dispose() ✅
• ReportAndQuitAsync() — detached via .Forget() with proper try/catch/finally per CLAUDE.md §9. However, UniTask.Delay has no cancellation token — if Dispose() runs during the grace period, the delay continues detached. See P2-1 below.

STEP 4 — Member audit

• LoadingTimeBenchmark(ILoadingStatus, IAnalyticsController, IScenesCache) — 1 consumer (ProfilingPlugin constructor). ✅
• 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