-
Notifications
You must be signed in to change notification settings - Fork 16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Add redis based reliability reporting #778
Open
jrconlin
wants to merge
40
commits into
master
Choose a base branch
from
feat/SYNC-4327_redis
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
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
pjenvey
previously requested changes
Nov 21, 2024
autoconnect/autoconnect-ws/autoconnect-ws-sm/src/identified/on_server_notif.rs
Outdated
Show resolved
Hide resolved
… feat/SYNC-4327_redis
jrconlin
force-pushed
the
feat/SYNC-4327_redis
branch
from
December 3, 2024 22:19
5ff3bb9
to
3de403d
Compare
Well, that was less than entertaining, but at least educational. |
Trinaa
reviewed
Dec 6, 2024
Trinaa
reviewed
Dec 13, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The integration test changes LGTM!
… feat/SYNC-4327_redis
… 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_report
that 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_id
toreliability_id
Metrics
to generic Cadence (to make calls more consistent)RELIABLE_REPORT
flag to testing.Closes: SYNC-4327