Skip to content
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

Add ActiveSupport tracer for cache module #2380

Merged
merged 1 commit into from
Oct 18, 2024

Conversation

frederikspang
Copy link
Contributor

@frederikspang frederikspang commented Aug 16, 2024

Description

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

Todo:

  • Specs

@frederikspang frederikspang force-pushed the master branch 2 times, most recently from 9f44675 to 10be4dd Compare August 20, 2024 12:46
@frederikspang frederikspang marked this pull request as ready for review August 20, 2024 12:46
Copy link

codecov bot commented Aug 27, 2024

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 ⚠️
.../sentry/rails/tracing/active_support_subscriber.rb 96.29% 1 Missing ⚠️
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%) ⬇️
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%> (ø)

Copy link
Member

@sl0thentr0py sl0thentr0py left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks a lot @frederikspang, left a couple of comments.
I will test this out a bit when I have some time with our product.
There are also some other types that are not covered (such as cache_read_multi) that I will also think about the best way to handle.

Copy link
Collaborator

@solnic solnic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@frederikspang
Copy link
Contributor Author

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".

@solnic
Copy link
Collaborator

solnic commented Oct 11, 2024

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.

@frederikspang
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@solnic
Copy link
Collaborator

solnic commented Oct 17, 2024

@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 :)

@frederikspang frederikspang force-pushed the master branch 2 times, most recently from 89b07c3 to 69e41db Compare October 17, 2024 10:49
@frederikspang
Copy link
Contributor Author

frederikspang commented Oct 17, 2024

@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
Copy link
Contributor Author

@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).

CHANGELOG.md Outdated Show resolved Hide resolved
@frederikspang
Copy link
Contributor Author

Spec coverage should be fixed by #2437 I see :)

@sl0thentr0py
Copy link
Member

sl0thentr0py commented Oct 18, 2024

can I merge this now @solnic?

@solnic solnic merged commit ee37a4a into getsentry:master Oct 18, 2024
137 of 139 checks passed
@solnic
Copy link
Collaborator

solnic commented Oct 18, 2024

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.

@frederikspang
Copy link
Contributor Author

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

@st0012
Copy link
Collaborator

st0012 commented Oct 26, 2024

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.

@solnic
Copy link
Collaborator

solnic commented Oct 28, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants