Skip to content

Telemetry baseline: session tracking and UI-framework marker#987

Draft
johnml1135 wants to merge 1 commit into
mainfrom
telemetry-migration-baseline
Draft

Telemetry baseline: session tracking and UI-framework marker#987
johnml1135 wants to merge 1 commit into
mainfrom
telemetry-migration-baseline

Conversation

@johnml1135

@johnml1135 johnml1135 commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Summary

Establishes a WinForms telemetry baseline (openspec change
telemetry-migration-baseline) ahead of the Avalonia UI migration, so there is
a legacy usage/crash/session baseline to compare against later. All events go
through DesktopAnalytics.Analytics directly.

  • Session baselineSessionStart/SessionEnd with a per-launch
    SessionId and clean-vs-crashed classification, guarded so exactly one
    terminal event fires per session (gives session duration and crash-free rate).
  • UiFramework markerSetApplicationProperty("UiFramework", "WinForms")
    once at startup; a forward-compatible constant a future Avalonia surface flips
    per-surface, with no change to how events are tracked.
  • SwitchToTool dwell time — the existing throttled event now carries the
    time spent in the previous tool, plus a final record at shutdown.

Offline durability lives in the library, not here

Making analytics durable when Mixpanel is unreachable (on-disk spool, retry, a
real per-event delivery ack, $insert_id dedup, consent purge) is owned by the
SIL.DesktopAnalytics library's durable MixpanelClient
(sillsdev/DesktopAnalytics.net#43), so FieldWorks carries no queueing code and
inherits durability from a version bump. Until that release, offline-generated
events are delivered best-effort exactly as today — an accepted, temporary gap.
This PR also adds local-checkout iteration tooling (Build/Manage-LocalLibraries.ps1 -DesktopAnalytics) and two smoke tests that validate that library's spool directly.

Testing

  • .\build.ps1 green (0 warnings, 0 errors). .\test.ps1 on the affected
    projects with no regressions vs. main: FwUtilsTests 375/375,
    LexTextDllTests 3/3, FieldWorksTests 34/34 (1 pre-existing skip).
  • Two PowerShell smoke tests validate the upstream durable spool directly, no
    FieldWorks launch (see Docs/architecture/local-library-debugging.md):
    Test-AnalyticsOfflineDrain.ps1 (per-process firewall block —
    accumulate-then-drain across processes) and Test-AnalyticsOfflineSandbox.ps1
    (Windows Sandbox, no NIC — born-offline, never falsely acked). Both pass as of
    2026-07-09.
  • Not yet done: live-app manual pass confirming session/dwell events reach
    Mixpanel with UiFramework=WinForms and that the privacy checkbox still gates
    telemetry (tasks.md §4.3); session and dwell logic have no automated test
    (tasks.md §2.6/§3.4).

External prerequisite

Release the durable SIL.DesktopAnalytics (sillsdev/DesktopAnalytics.net#43),
then bump SilDesktopAnalyticsVersion in Build/SilVersions.props
(tasks.md §5). No other FieldWorks change is needed for offline durability;
the baseline instrumentation itself does not depend on that release.


This change is Reviewable

@github-actions

This comment has been minimized.

@github-actions

github-actions Bot commented Jul 2, 2026

Copy link
Copy Markdown

NUnit Tests

    1 files  ±0      1 suites  ±0   10m 29s ⏱️ +6s
4 302 tests ±0  4 229 ✅ ±0  73 💤 ±0  0 ❌ ±0 
4 311 runs  ±0  4 238 ✅ ±0  73 💤 ±0  0 ❌ ±0 

Results for commit b5f17ad. ± Comparison against base commit 6a73c08.

♻️ This comment has been updated with latest results.

@johnml1135 johnml1135 force-pushed the telemetry-migration-baseline branch 2 times, most recently from c2b97ca to f5b9386 Compare July 7, 2026 13:06
@johnml1135

Copy link
Copy Markdown
Contributor Author

Code review (verified against the code at the PR head)

This is a large, unusually well-documented change (design.md walks through several real bugs found and fixed during implementation — D13/D14 in particular are genuinely subtle Windows File.Move semantics worth keeping as institutional knowledge). One gap survived that documentation process:

HIGH — orphan-recovery path skips the network-availability check, so a stale .inflight file recovered while offline is silently dropped instead of requeued

AnalyticsOutbox.cs, ProcessOutboxFile (the normal, not-yet-claimed path) checks NetworkAvailable before attempting a claim:

if (!NetworkAvailable)
    return; // known gap (design.md D12): this only proves *a* network exists, not that Mixpanel is reachable
if (!TryClaimExclusive(path, InflightSuffix, out var claimedPath, out var claimedStream))
    return;
DeliverClaimedFile(claimedStream, claimedPath, path);

ProcessInflightFile (the orphan-recovery path, reached when a sweep finds an .inflight file older than OrphanClaimStaleness) has no equivalent check — it goes straight from the staleness check to TryClaimExclusive/DeliverClaimedFile:

if (DateTime.UtcNow - claimedAtUtc < OrphanClaimStaleness)
    return;
var originalPath = path.Substring(0, path.Length - InflightSuffix.Length);
if (!TryClaimExclusive(path, RecoveringSuffix, out var claimedPath, out var claimedStream))
    return;
DeliverClaimedFile(claimedStream, claimedPath, originalPath);

Per D12, Analytics.Track's internal call is fire-and-forget and essentially never throws regardless of connectivity — that's exactly why the normal path added the NetworkAvailable pre-check as a (documented, imperfect) guard against claiming-then-deleting a file that was never actually sent. Without that same guard here, a genuinely offline sweep that finds an old orphaned .inflight file (left behind by a crashed prior claimant) will claim it, "deliver" it (a no-op fire-and-forget that succeeds locally with no network), and delete it — permanently losing exactly the kind of event (crash-then-offline) this durability mechanism exists to protect. AnalyticsOutboxTests.Flush_DeliversOrphanedInflightFile_LeftBehindByACrashedPriorClaim only exercises this path with NetworkAvailableOverride = () => true, so the gap isn't caught by the current suite.

Suggest adding the same if (!NetworkAvailable) return; check to ProcessInflightFile before the claim attempt (or better yet, fold the offline check into the top of DeliverClaimedFile/TryClaimExclusive so both callers can't diverge again).

Minor — FlushSync's timeout is only checked between files, not within one

Flush's loop checks the deadline once per file (if (deadlineUtc.HasValue && DateTime.UtcNow >= deadlineUtc.Value) break;) but ProcessOutboxFile itself has no per-call timeout. Given Analytics.Track is fire-and-forget (per D12) this is low-risk in practice, but a single slow file op (e.g. AV-scanner-held file, RobustFile's retry path) could still make the shutdown sweep's "≤2s" guarantee (D9) soft rather than hard. Not blocking — just worth knowing it's a soft bound.

Reviewed with Claude Code; findings verified against the code at the PR head. No code changed.

@johnml1135

Copy link
Copy Markdown
Contributor Author

Fixed the HIGH issue in b98e841: added the same NetworkAvailable check to ProcessInflightFile that ProcessOutboxFile already had, so an orphaned .inflight file recovered while offline is left queued instead of being claimed, "delivered" via a no-op fire-and-forget Track, and deleted as if sent.

While verifying that fix, stress-testing (10 runs) turned up a second, real bug in the same file: Flush_ConcurrentFlushCalls_DeliverEachEventExactlyOnce is genuinely flaky on unmodified code (~2/3 failure rate in my testing, not a fluke — I reproduced it on a stash of the original code before making any changes). Root cause: DeliverClaimedFile released the FileShare.None exclusive lock immediately after reading the file, before delivery and delete. A racer whose RobustFile.Move spuriously "succeeds" against the same file (the .NET File.Move quirk your own design.md D14 documents) can win its own exclusive open in that now-unlocked window and deliver the same file a second time. Fixed by holding the lock open through delivery and releasing it only immediately before the terminal delete/restore. 10/10 green after the fix; full AnalyticsOutboxTests suite (16/16) also green.

Added Flush_LeavesOrphanedInflightFileQueued_WhenNetworkUnavailable to cover the first fix. Didn't add a dedicated regression test for the second (the existing concurrent-flush test already covers it once it's reliably green - a flaky test only proves the bug intermittently, so the real signal is that it now passes consistently rather than ~33% of the time).

Also verified the minor point re: FlushSync's per-file (not per-call) deadline check — leaving that as-is per the original note, since it's a soft bound consistent with the fire-and-forget Track assumption, not a correctness issue.

@github-actions

This comment has been minimized.

@johnml1135

Copy link
Copy Markdown
Contributor Author

(Follow-up: trimmed the two comments from the fix above down to one line each, per repo convention.)

@johnml1135 johnml1135 force-pushed the telemetry-migration-baseline branch 2 times, most recently from 3a59a1c to 77ce5a6 Compare July 8, 2026 20:50
@johnml1135 johnml1135 marked this pull request as draft July 8, 2026 21:09
@github-actions

This comment has been minimized.

@johnml1135 johnml1135 changed the title Telemetry baseline: durable analytics outbox, session tracking, UI-framework marker Telemetry baseline: session tracking and UI-framework marker Jul 9, 2026
Instrument a legacy WinForms usage/crash/session baseline for a future
Avalonia UI migration to compare against. Events go through
DesktopAnalytics.Analytics directly; offline durability is the
SIL.DesktopAnalytics library's job, not FieldWorks' (design D4).

- Session baseline: SessionStart/SessionEnd with a per-launch SessionId
  and clean-vs-crashed classification, guarded so exactly one terminal
  event fires per session (session duration, crash-free rate).
- UiFramework marker: SetApplicationProperty("UiFramework", "WinForms")
  once at startup, a forward-compatible constant a future Avalonia
  surface flips per-surface without changing how events are tracked.
- SwitchToTool dwell time: the throttled event now carries time spent in
  the previous tool, plus a final record at shutdown.

Also adds local DesktopAnalytics.net iteration tooling
(Manage-LocalLibraries.ps1) and two offline-durability smoke tests for
the library's Mixpanel spool. openspec: telemetry-migration-baseline.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@johnml1135 johnml1135 force-pushed the telemetry-migration-baseline branch from c286887 to b5f17ad Compare July 9, 2026 20:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant