feat: add utc_offset_minutes to the analytics context - #10168
Conversation
Segment stores every timestamp in UTC and strips the offset from the ISO string, so local-day retention (D1/D7) cannot be computed from the Unity client's events. Send the system time zone offset in minutes as a common context trait, resolved once at session start. The field name matches the one the Godot client is adding (godot-explorer#2763). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
🚦 CI StatusWindows and Mac built successfully in Unity Cloud.
Warnings not reduced: 11846 => 11846 — remove at least 1 warning to merge. No warnings in files changed by this PR — showing general ones you can remove to unblock (50 of 11846)Lint run · took 33m 58s All Unity tests passed ✅
Tests time sums the test cases; Job time is the job's wall clock including checkout, licensing and asset import. Slowest tests
Full report: run summary · results + editor logs: editmode · playmode 🏁 Bare-metal benchmark finished — run #35892111812. Full reportPR #10168, run #35892111812 Overall: ✅ no significant changes Builds: Windows change, Windows baseline, macOS change, macOS baseline How to read this table
Apple M1
Intel Core i5
On demand — comment |
The Godot client has not shipped its field yet, so nothing is locked in downstream. The name spells out what the number is relative to. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
popuz
changed the title
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
Review — PR #10168: feat: add tz_offset_minutes to the analytics context
Step 2 — Root-cause check: PASS
The problem is clearly stated: the Segment pipeline loses timezone information because the Unity client sends nothing about the user's timezone, making D1/D7 retention metrics inaccurate when cut on UTC midnight. The fix adds utc_offset_minutes to every tracked event via the existing StaticCommonTraitsPlugin. This addresses the root cause directly — the missing data field.
Step 3 — Design & integration: PASS
Placement. StaticCommonTraitsPlugin is the correct home. The field follows the exact initialization pattern of its siblings (os, dclRendererType) — a readonly JToken resolved once via a field initializer and assigned per-event in Track() at the cost of one reference copy. No new type or lifecycle unit is introduced.
Static vs Dynamic. Evaluated whether the offset belongs in DynamicCommonTraitsPlugin given that a DST transition mid-session would stale the cached value. Downgraded: TimeZoneInfo.Local on Unity's Mono runtime is itself resolved once per process — calling GetUtcOffset per-event in DynamicCommonTraitsPlugin would return the same value. The author documented this trade-off and the one-line migration path. The retention use-case (calendar-day bucketing) is tolerant of a ±1 hour edge case occurring at most twice per year.
No new lifecycle unit. Nothing to trace through the mandatory owner search — the change adds one field to an existing plugin, no subscriptions, connections, or teardown needed.
Step 4 — Member audit: PASS
No new public surface. utcOffsetMinutes is private readonly, consumed only by Track(). No members to audit for single-use, absent-≠-false, or redundant-guard concerns.
Step 5 — Line-level review
R1 (alloc in hot paths): Clean — Track() assigns a cached JToken reference, no allocation.
R2 (LINQ): N/A.
R4 (ECS discipline): N/A — not an ECS system.
R5 (Entity by-ref): N/A.
R6 (acquire/release): N/A — no subscriptions, disposables, or pool rentals introduced.
R7 (nullability): Clean — non-nullable JToken field unconditionally assigned.
R8 (root-cause): PASS — see Step 2.
R9 (logging): N/A.
R11 (async/CT): N/A.
R12 (single-impl abstraction): N/A.
R13 (reuse/centralize): Clean — no duplicate constant or URL literal. TimeZoneInfo has no project wrapper.
R14 (contract honesty): Clean — see Step 3 rationale.
R15 (dead weight): N/A.
R16/R17 (naming): Clean — field utcOffsetMinutes follows camelCase; JSON key "utc_offset_minutes" follows the snake_case convention of all sibling keys.
R18 (magic numbers): N/A.
R19 (file hygiene): Clean.
R20 (idiom cluster): Clean — readonly on the new field; no concurrent collections.
R23 (AI comments): Clean — no comments added.
R24 (scope): Clean — one file, one feature, no unrelated changes.
Cast safety. (int)TimeZoneInfo.Local.GetUtcOffset(DateTime.UtcNow).TotalMinutes — real-world UTC offsets range from −720 to +840 minutes, always in whole-minute increments. TotalMinutes returns an integer-valued double for these inputs; the (int) truncation is lossless and well within int range.
using System; ordering. Correctly placed alphabetically: Newtonsoft.Json.Linq < System < UnityEngine.
Findings
| # | File | Line | Sev | Rule | Finding |
|---|---|---|---|---|---|
| 1 | StaticCommonTraitsPlugin.cs | 24 | P2 | — | Cross-client field name alignment. PR description says the name matches the Godot client's tz_offset_minutes (godot-explorer#2763), but the code now ships utc_offset_minutes after the rename commit (107137d). Confirm the Godot PR was also renamed, or update the PR description — mismatched names would break the joinability promise. |
| 2 | StaticCommonTraitsPlugin.cs | — | P2 | R22 | No test added. StaticCommonTraitsPlugin has zero pre-existing test coverage (confirmed via repo search). The change is pattern-identical to 6 other untested fields, so the gap is pre-existing — but R22 asks new behavior to ship with tests. A simple unit test constructing the plugin and asserting Track() populates "utc_offset_minutes" with a sane integer would close this. |
Step 6 — Complexity: SIMPLE
One file, 3 added lines, no ECS/async/plugin/DI/networking changes. Pure data-field addition following an established pattern.
Step 7 — QA assessment: YES
The change modifies runtime code under Explorer/ that ships in the build (analytics payload). Verifiable via --debug analytics logging as described in the PR.
Step 8 — Non-blocking warnings
None. Main scene not modified.
Merge gates
R25 — Open threads: 0 unresolved review threads (first review round).
R26 — QA gate: Awaiting QA. PR carries manual test instructions.
Lint pre-flight
scripts/lint/custom-rules.sh is not present on this branch. Machine-held rules (R1–R24 regex/analyzer IDs) were checked by hand against the diff — no violations found.
REVIEW_RESULT: PASS ✅
COMPLEXITY: SIMPLE
COMPLEXITY_REASON: Single data-field addition to an existing analytics plugin — no ECS, async, DI, or networking changes
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by decentraland-bot via GitHub
popuz
added
the
no QA needed
|
checked manually - new field is presented in the context |
Segment stores every timestamp in UTC and recomputes its own `timestamp`, so the offset in the client's ISO string never reaches the warehouse and retention can only be cut on UTC midnight. #2763 measured the artifact: D1 for the same cohort reads 24.75% for installs at 23:00 UTC and 4.20% for installs at 01:00. Adds one signed integer to the common explorer fields, so it rides into the `properties` of every event through the existing builder — including `Install Attribution`, which fires pre-login. `local = utc + offset`; minutes rather than hours so half- and quarter-hour zones round-trip exactly. Resolved once at startup, like `renderer_version`. The field name and the decision to send no zone name come from decentraland/unity-explorer#10168 and popuz's comment on #2763, and supersede the `tz_id` + `tz_offset_minutes` shape in the issue body. The value is NOT `Time.get_time_zone_from_system()["bias"]`. Its sign is right, but `OS_Unix::get_time_zone_info()` converts `strftime("%z")` with `bias % 100`, which is negative in C, and the `bias < 0` branch subtracts it — double-negating the minutes. Against the 4.6.2 binary we ship it reports -90 for Newfoundland (-150) and -510 for the Marquesas (-570), while Kolkata, Kathmandu and Chatham are fine. Newfoundland is one of the zones the minutes decision exists for, so the offset is derived from the OS wall clock instead: read local as if it were UTC, diff against real UTC. `chrono::Local` is also unusable here — it resolves the zone from /etc/localtime, which Android does not maintain.
3 participants
Problem
Segment stores every timestamp in UTC. The client-sent
timestampis relabeled tooriginalTimestamp, Segment recomputes its owntimestamp, and the offset in the ISO string never reaches the warehouse. The Unity client sends nothing about the user's time zone, so retention (D1/D7) can only be cut on UTC midnight. The Godot team measured the size of that artifact in godot-explorer#2763: D1 for the same cohort reads 24.75% for installs at 23:00 UTC and 4.20% for installs at 01:00 UTC.Change
StaticCommonTraitsPluginadds one common trait to the Segmentcontextof every event:utc_offset_minutes180(UTC+3),330(India),-210(Newfoundland)TimeZoneInfo.Local.GetUtcOffset(DateTime.UtcNow)local = utc + offset. Local calendar day in SQL:JToken, so the per-event cost is one reference assignment like the other static traits. A DST switch mid-session is a negligible error next to the current UTC-day cut; moving the trait toDynamicCommonTraitsPluginlater is a one-line change.context, so in the warehouse it lands ascontext_utc_offset_minutesontracksand every event table.Why a signed integer, not a zone name
A zone name string depends on both the engine and the OS: Unity on Windows returns Windows zone ids such as
Pacific Standard Time, Unity on macOS returnsAmerica/Los_Angeles; Godot gives IANA on Android/iOS/macOS and Windows ids on Windows. Emitting the same strings from every client would need a Windows→IANA mapping table and alias normalization. The offset is computed the same way by every engine on every OS, no mapping, and it is all a local-day cut needs.Why
utc_offset_minutes, nottz_offset_minutes/time_zone_offset_minutesGetUtcOffset, Rusttime::UtcOffset, Pythonutcoffset().timezone offsetis ambiguous because JavaScript'sgetTimezoneOffset()returns the inverted sign (-180for UTC+3).Cross-client alignment is proposed in godot-explorer#2763 (comment):
utc_offset_minutes(int) on launcher, unity-explorer and godot-explorer; the launcher's existingfp_timezonestays the only zone-name field.Out of scope
fp_timezoneon every event. Per the alignment proposal,fp_timezonestays the only zone-name field across clients.Test
Manual: run the client with
--debuganalytics logging (or theDebugAnalyticsService) and confirm every tracked event carriesutc_offset_minutesequal to the machine's current UTC offset in minutes.🤖 Generated with Claude Code