Skip to content

Add store labels to all Rails.cache metrics - 2nd attempt

Sylvester Chin requested to merge sc1-railscache-metric-granularity-2 into master

What does this MR do and why?

This MR is a 2nd try of !121477 (merged). It has 4 main commits:

  1. Revert of !121692 (merged) which effectively does !121477 (merged)
  2. Adding spec which fails. Good news because now we have a test case to catch for invalid label set errors and any other obscure errors when running multiple subscriber methods in a transaction
  3. The fix: a private method to increment gitlab_cache_misses_total so that we dont have to sync up all call-sites of .increment(:gitlab_cache_misses_total, ...)
  4. Rubocop fix.

The root cause of the revert is that cache_generate and cache_read increments the counter gitlab_cache_misses_total but have different labels. There is a difference in label because only 1 of the 2 callsites added the new label. The respective specs were updated but does not account for when multiple metrics are incremented in a single webtransaction.

Screenshots or screen recordings

Screenshots are required for UI changes, and strongly recommended for all other merge requests.

Before After

How to set up and validate locally

  1. Checkout to the 2nd commit
git checkout 0beb2c0b45e9
  1. Run the rspec
bundle exec rspec spec/lib/gitlab/metrics/subscribers/rails_cache_spec.rb
  1. Observe 1 failed spec
Failures:

  1) Gitlab::Metrics::Subscribers::RailsCache when receiving multiple instrumentation hits in a transaction does not raises InvalidLabelSetError error
     Failure/Error:
       expect {
         subscriber.cache_read(event)
         subscriber.cache_read_multi(event)
         subscriber.cache_write(event)
         subscriber.cache_delete(event)
         subscriber.cache_exist?(event)
         subscriber.cache_fetch_hit(event)
         subscriber.cache_generate(event)
       }.not_to raise_error

       expected no Exception, got #<Prometheus::Client::LabelSetValidator::InvalidLabelSetError: labels must have the same signature: (... [:action, :controller, :feature_category], got: [:action, :controller, :feature_category, :store])> with backtrace:
         # ./lib/gitlab/metrics/transaction.rb:68:in `increment'
         # ./lib/gitlab/metrics/subscribers/rails_cache.rb:60:in `cache_generate'
         # ./spec/lib/gitlab/metrics/subscribers/rails_cache_spec.rb:27:in `block (4 levels) in <top (required)>'
         # ./spec/lib/gitlab/metrics/subscribers/rails_cache_spec.rb:20:in `block (3 levels) in <top (required)>'
         # ./spec/spec_helper.rb:424:in `block (3 levels) in <top (required)>'
         # ./spec/support/sidekiq_middleware.rb:18:in `with_sidekiq_server_middleware'
         # ./spec/spec_helper.rb:415:in `block (2 levels) in <top (required)>'
         # ./spec/spec_helper.rb:411:in `block (3 levels) in <top (required)>'
         # ./lib/gitlab/application_context.rb:61:in `with_raw_context'
         # ./spec/spec_helper.rb:411:in `block (2 levels) in <top (required)>'
         # ./spec/spec_helper.rb:242:in `block (2 levels) in <top (required)>'
         # ./spec/support/system_exit_detected.rb:7:in `block (2 levels) in <main>'
         # ./spec/support/database/prevent_cross_joins.rb:108:in `block (3 levels) in <main>'
         # ./spec/support/database/prevent_cross_joins.rb:62:in `with_cross_joins_prevented'
         # ./spec/support/database/prevent_cross_joins.rb:108:in `block (2 levels) in <main>'
  1. Checkout to sc1-railscache-metric-granularity-2 and rerun the specs. There should be no failures this time.

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

Edited by Sylvester Chin

Merge request reports

Loading