Repository navigation
Keep track's optimistic metric update when not connected - #72
Open
ryanechternacht wants to merge 1 commit into
Open
ryanechternacht wants to merge 1 commit into
ryanechternacht wants to merge 1 commit into
Conversation
track only bumped the cached company metric when the DataStream client reported connected?, which in replicator mode is the replicator's readiness. While the replicator reported not ready, or the WebSocket was disconnected, usage stopped counting locally and metered limits stayed at the last value the replicator or stream wrote. Drop the connected? condition so the update runs whenever DataStream is in use, in both modes. Server and replicator updates replace metric values outright, so nothing is counted twice, and the update is a no-op for companies that aren't cached. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
2 of 3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
trackenqueues the event and then optimistically adds its quantity to the matching metric on the cached company, viaDataStream::Client#update_company_metrics. That second step only ran when@datastream_client&.connected?was true. In replicator modeconnected?is the health endpoint'sreadyfield, and in websocket mode it is the WebSocket connection state. So while the replicator reported not ready, or the WebSocket was disconnected, usage stopped counting locally and every metered limit stayed at the last value the replicator or stream wrote. The event itself was still enqueued to the API either way.Why the gate existed
The condition arrived in #5 ("Ruby custom code"), the initial port of the hand-written layer, as
if @datastream_client&.connected? && body[:company]with the comment "Update company metrics locally if DataStream is active and connected". The PR description says it follows the SDK spec with schematic-node as the primary reference, and its review has no discussion of the condition. #23 later touched the method (adding track options) without changing the condition, and has no discussion of it either.It is a port of the Go and Node gate, and SchematicHQ/schematic-go#239 traces that back: Go introduced it when
datastreamConnectedwas the only signal that DataStream was in use, a refactor translated it mechanically intoIsConnected(), and replicator mode later redefinedIsConnected()as readiness without revisiting the call site. Nothing in Ruby's history ties it to a correctness concern either.Why removing it is safe
Merge.upsert_metrics, which replaces each metric by(event_subtype, period, month_reset)with the server's value. In replicator mode the replicator writes whole company records to Redis. Whatever arrives next overwrites the optimistic value rather than adding to it, which is the same reconciliation that already happens between a bump and the next push while connected.update_company_metricsreads the company from the cache first and returns if it isn't there, so it only rewrites an entry that already exists. In replicator mode the keys use the last cache version the client knows.The gate is dropped in websocket mode too, for the same reasons, and a single "DataStream is in use" condition is simpler than branching on mode.
Change
In
SchematicClient#track, the metric update now runs when@datastream_client && body[:company], in both modes. Nothing else changes.Related, not changed here
update_company_metricsrewrites the company and its lookup keys with the SDK's DataStreamcache_ttl(24 hours by default), while the replicator writes with no TTL by default. It also rewrites the company when no metric matched the event. Both already happen today whenevertrackruns with the replicator ready. With this change they can also happen while the replicator is not ready, so during a very long outage a company that was tracked and then went idle forcache_ttlcould drop out of Redis until the replicator resyncs. schematic-node#202 raised the same point; a follow-up could keep the existing expiry in replicator mode and skip the write when no metric matched.Overlap
#71 (replicator cache-ready gate) edits the flag-check gates and the health poll in the same files but not
track. The two branches merge cleanly.Parity
This matches SchematicHQ/schematic-go#239. The same change is open for Node (schematic-node#202), Python (schematic-python#134), Java (schematic-java#115) and C# (schematic-csharp#202).
Tests
New
SchematicClient - track metric update while not connectedblock intest/custom.test.rb:/readyanswering 503{"ready": false, "cache_version": "v1"}and a Redis stand-in seeded the way the replicator writes a company (the entity under its ID key plus a lookup key).connected?is false,trackwith quantity 5 moves the cached metric from 10 to 15.track.On main the replicator test reads the cache version from the rules engine, since main ignores a 503's body, so the test sets
v1explicitly. With #71 the 503 recordsv1itself.The two "moves the metric" tests fail on main (value stays 10) and pass with this change. Locally, in a
ruby:3.3container to match CI:bundle exec rubocop(1140 files, no offenses) andbundle exec ruby -I lib -I test test/custom.test.rb(193 runs, 397 assertions, 0 failures, 0 errors) pass.🤖 Generated with Claude Code