Add ActiveSupport tracer for cache module #2380
Conversation
frederikspang
force-pushed
the
master
branch
2 times, most recently
from
9f44675
to
10be4dd
Compare
CodecovReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2380 +/- ##
==========================================
-Coverage 98.69% 98.68% -0.02%
==========================================
Files 210 212 +2
Lines 13923 14028 +105
==========================================
+Hits 13741 13843 +102
-Misses 182 185 +3
|
There was a problem hiding this comment.
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 ascache_read_multi) that I will also think about the best way to handle.
| op: operation_name(event_name), | ||
| origin: SPAN_ORIGIN, | ||
| start_timestamp: payload[START_TIMESTAMP_NAME], | ||
| description: payload[:service], |
There was a problem hiding this comment.
I don't see:servicein here
https://github.com/rails/rails/blob/5a0b2fa5a3be6ffd49fa7f1c3544c1da403c9cb5/guides/source/active_support_instrumentation.md?plain=1#L520-L539
we seem to have a:storeinstead
There was a problem hiding this comment.
You're absolutely right. That's a bad leftover from activestorage instrumentation! I'll have a look at this one tomorrow.
There was a problem hiding this comment.
since CI is also failing in many places, I can take this over if you wish but you will need to give me maintainer push access
https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork#enabling-repository-maintainer-permissions-on-existing-pull-requests
There was a problem hiding this comment.
Be my guest! I'm happy to resolve the errors in here and in#2383(Also from me/us:) )
I've pushed initial changes, but I am not running a full test suite - I'll get that up and running locally as well.
Any way to accept CI runs from this PR automatically? Instead of manual approval each time I do changes:)
There was a problem hiding this comment.
One thing - Before I go around pure anarchy in terms ofmake_basic_app.With the current set up I am unable to change the Rails.cache store to a memory store, because the block is yielded from an initializer - After initializing the rails app and cache store (Filestore by default).
I can either change the store for allmake_basic_appsetup (Which shouldn't affect much), or we can introduce a new parameter tomake_basic_app(cache_store:),but that seems like a bad direction to go.
The issue in specs is, that the Prune method, only works for memorystore.
When that is settled, I'll write specs for all of the other Rails.cache.* methods
There was a problem hiding this comment.
Got you! I've added specs for increment and decrement, and marked these as skipped for Rails < 8.0 - That should prepare them for having Rails 8.0 in testing matrix, and being included there.
There was a problem hiding this comment.
Unless we want to introduce Redis or Memcached in the testing, which is a separate container for the GitHub Actions workflow.
There was a problem hiding this comment.
@sl0thentr0pyDo we need anything else here except internal QA/processing from you guys?
We're looking forward to having this merged soon!
There was a problem hiding this comment.
no I will take care of this next week, sorry just swamped with another big feature
sentry-rails/lib/sentry/rails/tracing/active_support_subscriber.rb
Outdated
Show resolved
Hide resolved
Description
Implement child spans for ActiveSupport cache according tohttps://docs.sentry.io/platforms/ruby/tracing/instrumentation/custom-instrumentation/caches-module/
Todo: