Add caching minimum/maximum ids in new instrumentation classes for batch queries
requested to merge 326338-add-a-shared-context-for-instrumentation-classes-for-queries-optimization into master
What does this MR do?
Related to #326338 (closed)
This adds caching minimum/maximum ids for batch queries in new instrumentation database metrics, in order to memoize minimum and maximum ids for big tables and calculate them only once. A similar approach was used in Gitlab::UsageData
.
Caching is done only by instrumentation classes that explicitly pass a cache key using cache_start_and_finish_as
, to prevent unintended pollution.
Redis caching through Rails.cache
was used for simplicity:
- The ids would have to be shared between different subclasses of
DatabaseMetric
. Class variables are not a viable option, as they might have some unexpected side effects on production, where classes are not reloaded and this data might persist between Usage Ping runs. - We'll eventually want to run new instrumentation Usage Ping metrics in parallel (e.g. using Sidekiq). In that case, simple memoization won't be useful, as Sidekiq jobs don't share memory and the only option will be to use an external database anyway.
Does this MR meet the acceptance criteria?
Conformity
-
I have included a changelog entry, or it's not needed. (Does this MR need a changelog?) -
I have added/updated documentation, or it's not needed. (Is documentation required?) -
I have properly separated EE content from FOSS, or this MR is FOSS only. (Where should EE code go?) -
I have added information for database reviewers in the MR description, or it's not needed. (Does this MR have database related changes?) -
I have self-reviewed this MR per code review guidelines. -
This MR does not harm performance, or I have asked a reviewer to help assess the performance impact. (Merge request performance guidelines) -
I have followed the style guides. -
This change is backwards compatible across updates, or this does not apply.
Availability and Testing
-
I have added/updated tests following the Testing Guide, or it's not needed. (Consider all test levels. See the Test Planning Process.) -
I have tested this MR in all supported browsers, or it's not needed. -
I have informed the Infrastructure department of a default or new setting change per definition of done, or it's not needed.
Edited by Piotr Skorupa