sentry-ruby icon indicating copy to clipboard operation
sentry-ruby copied to clipboard

Add ActiveSupport tracer for cache module

Open frederikspang opened this issue 1 year ago • 1 comments

Description

Implement child spans for ActiveSupport cache according to https://docs.sentry.io/platforms/ruby/tracing/instrumentation/custom-instrumentation/caches-module/

Todo:

  • [x] Specs

frederikspang avatar Aug 16 '24 18:08 frederikspang

Codecov Report

Attention: Patch coverage is 84.21053% with 24 lines in your changes missing coverage. Please review.

Project coverage is 98.82%. Comparing base (03293ef) to head (69e41db).

Files with missing lines Patch % Lines
...ry/rails/tracing/active_support_subscriber_spec.rb 80.83% 23 Missing :warning:
.../sentry/rails/tracing/active_support_subscriber.rb 96.29% 1 Missing :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2380      +/-   ##
==========================================
- Coverage   98.97%   98.82%   -0.15%     
==========================================
  Files         231      233       +2     
  Lines       15094    15244     +150     
==========================================
+ Hits        14939    15065     +126     
- Misses        155      179      +24     
Components Coverage Δ
sentry-ruby 99.14% <ø> (ø)
sentry-rails 97.63% <84.21%> (-0.83%) :arrow_down:
sentry-sidekiq 98.67% <ø> (ø)
sentry-resque 96.85% <ø> (ø)
sentry-delayed_job 98.92% <ø> (ø)
sentry-opentelemetry 99.75% <ø> (ø)
Files with missing lines Coverage Δ
sentry-rails/lib/sentry/rails/configuration.rb 100.00% <100.00%> (ø)
sentry-rails/spec/dummy/test_rails_app/app.rb 100.00% <100.00%> (ø)
...ntry-rails/spec/sentry/rails/configuration_spec.rb 100.00% <100.00%> (ø)
.../sentry/rails/tracing/active_support_subscriber.rb 96.29% <96.29%> (ø)
...ry/rails/tracing/active_support_subscriber_spec.rb 80.83% <80.83%> (ø)

codecov[bot] avatar Aug 27 '24 12:08 codecov[bot]

There are some tests failing under older rubies/rails + a couple of rubocop offenses to fix. Oh and conflict with CHANGELOG still has to be resolved.

Other than that, LGTM. I tested it out locally and it seems to work well.

@solnic I've adjusted rubocop. It seems the failing specs are related to Rails 6.0.x release. I just scanned over the changelog for activesupport, and I see nothing related there - Quick test says, hit is not in the payload from the AS notification.

I can drill down into why, but it seems to be out of "our hands".

frederikspang avatar Oct 11 '24 13:10 frederikspang

I've adjusted rubocop.

Awesome :)

It seems the failing specs are related to Rails 6.0.x release. I just scanned over the changelog for activesupport, and I see nothing related there - Quick test says, hit is not in the payload from the AS notification.

I can drill down into why, but it seems to be out of "our hands".

I think it's perfectly fine to skip it in Rails 6 and add a note to CHANGELOG (and later on in the docs) that it's Rails 7+ only feature.

solnic avatar Oct 11 '24 14:10 solnic

I think it's perfectly fine to skip it in Rails 6 and add a note to CHANGELOG (and later on in the docs) that it's Rails 7+ only feature.

Seems more like a regression in 6.0.x. Works in 5.x and 6.1+ We can skip it, no problem.

frederikspang avatar Oct 12 '24 06:10 frederikspang

I think it's perfectly fine to skip it in Rails 6 and add a note to CHANGELOG (and later on in the docs) that it's Rails 7+ only feature.

Seems more like a regression in 6.0.x. Works in 5.x and 6.1+ We can skip it, no problem.

I was unable to find anything obvious in Rails 6.0.x releases. I have skipped the spec, and rubocop as well as rspec is passing for me locally. @solnic

frederikspang avatar Oct 13 '24 18:10 frederikspang

@frederikspang thanks for addressing remaining issues - could you update your branch to latest master? Coverage should be OK after rebasing as we now merge coverage results from all test runs :)

solnic avatar Oct 17 '24 10:10 solnic

@frederikspang thanks for addressing remaining issues - could you update your branch to latest master? Coverage should be OK after rebasing as we now merge coverage results from all test runs :)

@solnic

Squashed, and rebased :)

frederikspang avatar Oct 17 '24 10:10 frederikspang

@solnic I see coverage is kind of misleading in the _spec.rb file.. Rails 8 is not released yet, and is not in the test matrix. The specs I wrote there are an "improvement" that requires my Rails PR to be release - It's been merged into 8.0, but not backmerged. (Instrumentation of all cache stores).

frederikspang avatar Oct 17 '24 11:10 frederikspang

Spec coverage should be fixed by https://github.com/getsentry/sentry-ruby/pull/2437 I see :)

frederikspang avatar Oct 18 '24 10:10 frederikspang

can I merge this now @solnic?

sl0thentr0py avatar Oct 18 '24 12:10 sl0thentr0py

can I merge this now @solnic?

I merged it :) This PR made me think we could consider adding Rails HEAD to the test matrix - WDYT? Not necessarily as a strict CI step, ie allow it to issue a warning if some specs are failing instead of making CI fail. This way we could catch some issues early on.

solnic avatar Oct 18 '24 21:10 solnic

can I merge this now @solnic?

I merged it :) This PR made me think we could consider adding Rails HEAD to the test matrix - WDYT? Not necessarily as a strict CI step, ie allow it to issue a warning if some specs are failing instead of making CI fail. This way we could catch some issues early on.

Great idea! Might throw in Ruby head as well - or just add it "early" for the preview releases

frederikspang avatar Oct 19 '24 15:10 frederikspang

This PR made me think we could consider adding Rails HEAD

We used to do both Rails head and Ruby head. But eventually they just stayed broken most of the time and nobody would check them. More importantly, many times the failures were actually capturing Rails/Ruby/dependencies' bugs, not the SDK's. But we usually only found out after spending hours on it.

So I've adapted the approach to just update the CI matrix and tests when there are release candidates (e.g. #2444). It's more economical to fix issues at once when things have mostly settled.

st0012 avatar Oct 26 '24 16:10 st0012

@st0012 yes this makes sense, thanks for the insights!

solnic avatar Oct 28 '24 06:10 solnic