-
Notifications
You must be signed in to change notification settings - Fork 9
fix(monitor): DB-accurate max_age metrics #89
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
Conversation
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.
Probably best to view this diff with ?w=1 in the URL (to skip whitespace-only changes).
|
|
||
| match do |block| | ||
| @expected = { event_name: expected_event_name, payload: expected_payload, value: expected_value } | ||
| if @approximately && current_adapter != 'postgresql' |
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.
On postgres, we can fully align Timecop with the transaction's NOW() -- on other DB engines, we need to account for slight variance in query time.
|
|
||
| db_adapter = ENV["ADAPTER"] | ||
| gemfile = ENV["BUNDLE_GEMFILE"] | ||
| db_adapter ||= gemfile && gemfile[%r{gemfiles/(.*?)/}] && $1 # rubocop:disable Style/PerlBackrefs |
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.
I removed this line entirely as we no longer rely on adapter-specific gemfiles.
effron
left a comment
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.
domainLGTM! good catch
5950a87 to
39332a5
Compare
okasten
left a comment
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.
domain lgtm platform lgtm
yay
This ensures that max_lock_age and max_age are not artificially inflated by slower query times. Previously, if the query took N seconds to perform, N seconds would be artificially added to `max_age` and `max_lock_age`. Now, we rely on the DB's own reported UTC time at the time of the query. In PostgreSQL, this will correspond to the start of the transaction, and in other DB engines this will correspond to the DB's clock time as the query executes, so there is still some small variance but it is much smaller than before. /no-platform
39332a5 to
b52bcfa
Compare
effron
left a comment
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.
domainLGTM
This ensures that
max_lock_ageandmax_ageare not artificially inflated by slower query times. Previously, if the query took N seconds to perform, N seconds would be artificially added tomax_ageandmax_lock_age. (Typically this is not a problem for sub-second queries on small-medium tables, but on large tables this can trigger occasional alert thresholds on sensitive/high-priority queues.)With this PR, we rely on the DB's own reported UTC time at the time of the query to compute timestamp ages as of said query.
In PostgreSQL, the DB's "now" will correspond to the start of the transaction (as should the state of the rows it returns), and in other DB engines it will correspond to a clock time during execution, so there is still room for variance but it is much smaller than before.
/no-platform