Repository navigation
feat(backend): record server_disconnect metric when a server peer is left mid-sync - #303
gmaclennan wants to merge 2 commits into
Conversation
gmaclennan
left a comment
There was a problem hiding this comment.
I'm not sure of the benefit of this if we're shipping the other fixes, and it adds quite a lot of code to maintain. Isn't the metric that's valuable to collect is when a connection is disconnected before sync completes? Does that need a change in core to capture? And then we can also capture this metric on the server side also.
|
Agreed, the number worth having is "a connection closed while sync with that peer was unfinished", and this PR only approximates it for servers by watching sync state from the outside. Core is the only place that knows both facts at the moment they happen: I would close this PR in favour of that. If you agree I will open the core PR, with the two one-line consumers to follow once it lands. |
We have no telemetry showing whether a phone leaves the cloud archive while it still wants blocks from the phone. The investigation found that leaving the Exchange screen tears down the server websocket on blur regardless of progress, and that Stop (or the 30 s autostop) stops serving a still-connected server, so the cloud is left holding partial originals. Nothing in Sentry today tells us how often that happens in the field.
This adds one count metric,
comapeo.sync.server_disconnect, emitted from the existing sync observer inbackend/lib/sync-observer.js. The watcher wraps$sync.stop()and$sync.disconnectServers()on the project's sync API and treats any data-sync-off edge that neither call produced as the autostop, soreason=autostopcovers both the 30 s timer and the full stop that fires when the app is backgrounded. It fires once per data-sync session while a server peer is connected, withreason(disconnect,stop,autostop),data_enabled(was data sync on when the call came in) and, at the usage tier only,server_want_bucket(blocks the server still wanted from us:0,1-10,11-100,101-1000,1000+). Server peers are the members whosedeviceTypeisselfHostedServer, looked up through$member.getMany()and cached in the watcher. No peer or project identifiers are attached; the bucket helper and the metric wrapper live inbackend/lib/metrics.jsbeside the existing ones. The tests cover each reason, the usage-off case, a local-only exchange recording nothing, and the server leaving and reconnecting re-arming the event.Two things for reviewers. The want bucket is usage-gated like
peers_bucket, so at the diagnostics tier we only learn that a server was left and whether data sync was on; if the bucket should be always-on, it is a one-line change. And the "once per session" rule resets when the server peer disappears or data sync starts again, which means Stop followed by leaving the screen records a singlestopevent rather than two.Related changes. digidem/comapeo-core#1353 and digidem/comapeo-mobile#2213 remove the two ways of leaving the server with a partial upload that this metric counts. The metric is still worth having to see how often it happens on phones that have not updated. Once the mobile change ships, the reason=disconnect count should fall to near zero; if it does not, something else is closing the socket.