-
Notifications
You must be signed in to change notification settings - Fork 37
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
Metrics for block-level sync #1877
Conversation
Facility | ||
.with_discarded | ||
.updated_on_server_since(other_facilities_processed_since, limit) | ||
Statsd.instance.time("other_facility_records.Facility") do |
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.
These time
calls are now timing the old sync code (post revert), but whenever we ship the new-new perf changes, the body can be swapped and the timer can stay.
@@ -35,6 +39,27 @@ def disable_audit_logs? | |||
false | |||
end | |||
|
|||
def log_block_level_sync_metrics(response_key) |
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.
Centralize all the block-sync logging in one place so that the loggers don't end up being called multiple times during the code-path and are easy to find in one place.
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.
Couple of thoughts. Lookin' pretty solid.
@@ -11,10 +11,14 @@ def __sync_from_user__(params) | |||
end | |||
|
|||
def __sync_to_user__(response_key) | |||
AuditLog.create_logs_async(current_user, records_to_sync, "fetch", Time.current) unless disable_audit_logs? | |||
records = records_to_sync |
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.
Shall we put proper ivar caching into the methods to prevent this kind of inefficiency in future?
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.
👍🏼 #1921 is doing a bunch of nice memoization, I think we can do this there. I'll add a task for it in the PR.
def passport_assigning_facility | ||
"COALESCE((metadata->'assigning_facility_id'), (metadata->'assigningFacilityUuid'))::text" | ||
end | ||
end |
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.
What does the output of this report generally look like? Can we post some logs from a test run for good measure?
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.
Let me fetch some logs and post a counter...
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.
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.
This happens to be the actual number as it stands on production...
Noice. |
Story card: ch2073
Because
Measure and track in different ways how block-sync is performing.
This addresses
Added a a block-level sync dashboard on datadog that will get populated from this data.