Add store labels to all Rails.cache metrics - 2nd attempt
What does this MR do and why?
This MR is a 2nd try of !121477 (merged). It has 4 main commits:
- Revert of !121692 (merged) which effectively does !121477 (merged)
- 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
- 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, ...)
- 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
- Checkout to the 2nd commit
git checkout 0beb2c0b45e9
- Run the rspec
bundle exec rspec spec/lib/gitlab/metrics/subscribers/rails_cache_spec.rb
- 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>'
- 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.
-
I have evaluated the MR acceptance checklist for this MR.
Edited by Sylvester Chin