feat: Add redis based reliability reporting #778
Conversation
This file contains 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
This PR introduces tracking throughput for the database. It also introduces the PushReliability reporting skeleton. This will be fleshed out with full reporting later. Closes: #SYNC-4324
… feat/SYNC-4324_track_db
… feat/SYNC-4324_track_db
… feat/SYNC-4324_track_db
… feat/SYNC-4324_track_db
This adds a feature flag `reliable_report` that optionally enables Push message reliablity reporting. The report is done in two parts. The first part uses a Redis like storage system to note message states. This will require a regularly run "cleanup" script to sweep for expired messages and adust the current counts, as well log those states to some sequential logging friendly storage (e.g. common logging or steamed to a file). The clean-up script should be a singleton to prevent possible race conditions. The second component will write a record of the state transition times for tracked messages to a storage system that is indexed by the tracking_id. This will allow for more "in depth" analysis by external tooling. The idea being that reporting will be comprised of two parts: One part which shows active states of messages (with a log of prior states to show trends over time), and an optional "in-depth" record that could be used to show things like length of time in storage, overall success rates, survivability rates, etc. This patch also: * fixes a few typos * changes several methods that should consume Notifications, actually consume them. * convert from `tracking_id` to `reliability_id` * convert instance of specialized `Metrics` to generic Cadence (to make calls more consistent) * adds a `RELIABLE_REPORT` flag to testing. Closes: SYNC-4327
* alter `setup_bt` to include reliability family * alter config.yml for eventual integration test changes
… feat/SYNC-4327_redis
… feat/SYNC-4327_redis
jrconlin
force-pushed
the
feat/SYNC-4327_redis
branch
from
5ff3bb9 to
3de403d
Compare
|
Well, that was less than entertaining, but at least educational. |
Trinaa
reviewed
Dec 6, 2024
Trinaa
reviewed
Dec 13, 2024
… feat/SYNC-4327_redis
… feat/SYNC-4327_redis
… feat/SYNC-4327_redis
… feat/SYNC-4327_redis
pjenvey
requested changes
Jan 14, 2025
There was a problem hiding this comment.
I have a few nitpicks and cleanup to the recording code in this branch:
feat/SYNC-4327_redis...feat/SYNC-4327_redis-pjenvey
I'm still thinking about some of the state changes we discussed last week but that can happen in a later PR.
scripts/setup_bt.sh
Outdated
| cbt -project $PROJECT -instance $INSTANCE setgcpolicy $TABLE_NAME $MESSAGE_FAMILY "maxage=1s or maxversions=1" | ||
| cbt -project $PROJECT -instance $INSTANCE setgcpolicy $TABLE_NAME $MESSAGE_TOPIC_FAMILY "maxage=1s or maxversions=1" | ||
| cbt -project $PROJECT -instance $INSTANCE setgcpolicy $TABLE_NAME $ROUTER_FAMILY maxversions=1 | ||
| cbt -project $PROJECT -instance $INSTANCE setgcpolicy $TABLE_NAME $MESSAGE_TOPIC_FAMILY "maxage=60d or maxversions=1" |
There was a problem hiding this comment.
Suggested change
| cbt -project $PROJECT -instance $INSTANCE setgcpolicy $TABLE_NAME $MESSAGE_TOPIC_FAMILY "maxage=60d or maxversions=1" | |
| cbt -project $PROJECT -instance $INSTANCE setgcpolicy $TABLE_NAME $RELIABILITY_FAMILY "maxage=60d or maxversions=1" |
scripts/setup_bt.sh
Outdated
| cbt -project $PROJECT -instance $INSTANCE setgcpolicy $TABLE_NAME $MESSAGE_FAMILY "maxage=1s or maxversions=1" | ||
| cbt -project $PROJECT -instance $INSTANCE setgcpolicy $TABLE_NAME $MESSAGE_TOPIC_FAMILY "maxage=1s or maxversions=1" | ||
| cbt -project $PROJECT -instance $INSTANCE setgcpolicy $TABLE_NAME $ROUTER_FAMILY maxversions=1 | ||
| cbt -project $PROJECT -instance $INSTANCE setgcpolicy $TABLE_NAME $MESSAGE_TOPIC_FAMILY "maxage=60d or maxversions=1" |
There was a problem hiding this comment.
but log_report calls the cell timestamp an expiry and also sets it to this 60d from now -- so I don't understand. timestamp of 60d from now + 60d gcpolicy = 120d lifetime
Comment on lines
81
to
85
| let expiry = if subscription.reliability_id.is_some() { | ||
| Some(timestamp + headers.ttl as u64) | ||
| } else { | ||
| None | ||
| }; |
There was a problem hiding this comment.
nit: would be worth if we're keeping this but I think we can kill it (my branch)
Suggested change
| let expiry = if subscription.reliability_id.is_some() { | |
| Some(timestamp + headers.ttl as u64) | |
| } else { | |
| None | |
| }; | |
| let expiry = subscription.reliability_id.map(|_| timestamp + headers.ttl as u64); |
Comment on lines
33
to
34
| /// The UTC expiration timestamp for this message | ||
| pub expiry: Option<u64>, |
There was a problem hiding this comment.
An Option is confusing as all notifs have an expiry. I'll argue the pre-computed expiry field isn't necessary, calculating it on the fly is cheap (killed this in my branch)
Comment on lines
+485
to
+488
| // Record that we've sent the message out to APNS. | ||
| // We can't set the state here because the notification isn't | ||
| // mutable, but we are also essentially consuming the | ||
| // notification nothing else should modify it. |
There was a problem hiding this comment.
It could be mutable now that Router::route_notification was changed to own it: the webpush router mutates it -- but you have a point here, with the notification being consumed it doesn't need to be modified here or in fcm -- does it even need to be modified in the webpush router?
… feat/SYNC-4327_redis
…topush-rs into feat/SYNC-4327_redis
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.
This adds a feature flag
reliable_reportthat optionally enables Pushmessage reliablity reporting. The report is done in two parts.
The first part uses a Redis like storage system to note message states.
This will require a regularly run "cleanup" script to sweep for expired
messages and adust the current counts, as well log those states to some
sequential logging friendly storage (e.g. common logging or steamed to
a file). The clean-up script should be a singleton to prevent possible
race conditions.
The second component will write a record of the state transition
times for tracked messages to a storage system that is indexed by the
tracking_id. This will allow for more "in depth" analysis by external
tooling.
The idea being that reporting will be comprised of two parts:
One part which shows active states of messages (with a log of prior
states to show trends over time), and an optional "in-depth" record
that could be used to show things like length of time in storage,
overall success rates, survivability rates, etc.
This patch also:
tracking_idtoreliability_idMetricsto generic Cadence (to make calls more consistent)RELIABLE_REPORTflag to testing.Closes: SYNC-4327