Publish App analytics for 6.4 (BL-16716) - #8260
Conversation
|
| Filename | Overview |
|---|---|
| src/BloomExe/Publish/Rab/RabPublishApi.cs | Adds non-throwing post-operation analytics for App Builder build and install actions; no eligible new follow-up issue remains. |
Reviews (2): Last reviewed commit: "Publish App analytics for 6.4 (BL-16716)" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
[Claude Opus 5 (1M context) from John Thomson's machine during preflight] Consulted Devin on 2026-08-31 16:12 UTC up to commit Three reviews ran across the branch, one per commit. Devin raised two Bugs, no Investigate flags and no Informational items:
Both threads are replied-to and resolved. Note for anyone opening Devin's review page: it still lists both bugs against Greptile also reviewed and raised one P1 (a failed build can report a stale APK's size) — that one is a data-semantics question, so it is going to the developer rather than being fixed here. Its thread is deliberately left open. |
|
[Claude Opus 5 (1M context) from John Thomson's machine during preflight] Consulted Devin on 2026-08-31 17:42 UTC up to commit Both are the already-fixed pair, still re-listed with their pre-fix descriptions and pre-fix line numbers — one now pointing at a comment line. Verified again against the current code; the reasoning is on each thread. Every review thread on this PR is now resolved, Greptile's included. |
JohnThomson
left a comment
JohnThomson
left a comment
There was a problem hiding this comment.
@JohnThomson+AGNT resolved 2 discussions.
Reviewable status: 0 of 1 files reviewed, all discussions resolved.
Bloom 6.4 ships the App Builder and reports nothing about it. Whether a real Reading App Builder build succeeds for a user, how long it takes, and how it fails are things we have no way to see: CI never runs a real App Builder build, so field telemetry is the only continuous coverage this path can ever have, and 6.4 has none. PR #8228 wrote that telemetry, but only on 6.5, and the App Builder is one of the features that also ships in 6.4. This backports the one event that covers the feature, and nothing else. "Publish App" is reported when a build or an install finishes, carrying stage, result, elapsedSeconds, errorKind on a failure, bookCount, and apkSizeMB when there is an APK. It is identical to 6.5's event, so data from the two versions is directly comparable. Prepare is fast and local and, as in 6.5, deliberately is not reported. None of the rest of #8228 comes with it: the other six events cover features 6.4 does not have, and there is no analytics/track endpoint, no front-end trackEvent() helper, no BloomAnalytics wrapper and no commit-time check for it. The event is entirely C#-side, so the whole change is one file. Reporting an event cannot disturb the thing it reports on. The event is sent only after the operation's own outcome has been settled and shown to the user, and the whole of it -- gathering the properties, sending, and logging -- cannot throw. That matters here more than it usually would: this code runs between "the build finished" and "tell the front end the build finished", so an exception escaping it would skip PostSucceeded and leave a build that actually succeeded looking to the user like a hung one. Hence LogAnalyticsWithoutThrowing, whose bare inner catch is deliberate: standard error is the last channel available, and with nowhere left to report to, returning is the only correct move. Two differences from 6.5 are forced by 6.4's code rather than chosen. This branch calls DesktopAnalytics.Analytics.Track directly, as all of its existing call sites do, with a Logger.WriteEvent line alongside so the event can be confirmed on a machine where tracking is off -- on 6.5 the BloomAnalytics wrapper does that for every event; here there is no wrapper. And there is no "cancelled" result, because 6.4's build and install cannot be cancelled. One known and deliberate wrinkle, also recorded in the code: on a failed build apkSizeMB is whatever APK an earlier successful build left behind, so a failed event can carry a size it did not produce. Left alone because the event says the build failed, which makes the size self-evidently meaningless, and any aggregate question about app sizes has to filter to successful builds anyway. 6.5 behaves identically. This commit wants a null merge (-s ours) into master and 6.5, which already carry the real implementation. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
07655cd to
7e74177
Compare
1 participant
Problem
Bloom 6.4 ships the App Builder, and reports nothing about it. Whether a real Reading App
Builder build actually succeeds for a user, how long it takes, and how it fails are things we
have no way to see: CI never runs a real App Builder build, so field telemetry is this path's
only possible continuous coverage — and 6.4 has none. #8228 wrote exactly that telemetry, but
only on the 6.5 branch, and the App Builder is one of the features that also ships in 6.4.
Fix
Backport the one event that covers this feature, and nothing else.
Publish App, reported when a build or an install finishes, carryingstage,result,elapsedSeconds,errorKindon a failure,bookCount, andapkSizeMBwhen there is an APK.Identical to 6.5's event, so data from the two versions is directly comparable. Prepare is fast
and local and, as in 6.5, deliberately is not reported.
the plumbing comes with this: no
analytics/trackendpoint, notrackEvent()front-endhelper, no
BloomAnalyticswrapper and no commit-time check for it. The event is entirelyC#-side, so the whole change is one file.
build's own outcome has been settled and shown to the user, and the whole of it — gathering the
properties, sending, and logging — cannot throw. That matters more here than it looks: this
code runs between "the build finished" and "tell the front end the build finished", so an
escaping exception would leave a successful build looking to the user like a hung one. Both
halves of that came from review (see the resolved threads).
Two differences from 6.5 are forced by 6.4's code rather than chosen: this branch calls
DesktopAnalytics.Analytics.Trackdirectly, as all ~30 of its existing call sites do (with alog line kept alongside so the event can be verified on a dev machine, where tracking is off);
and there is no
"cancelled"result, because 6.4's build and install cannot be cancelled.This commit wants a null merge (
-s ours) into master and 6.5, which already carry the realimplementation.
Ref: https://issues.bloomlibrary.org/youtrack/issue/BL-16716
Devin review
This change is