Redis support for sentry-ruby

[lewispb]

Description

Implements #1693

Patches a Redis client to capture breadcrumb and span data when calls are made to Redis.

Todo

• Test scenario where Redis client isn't available
• Test more complex Redis commands
• Tests for breadcrumb logger
• Manual testing
• Changelog

Outstanding questions

• Some folks might find it useful to also capture the values of the Redis commands. That would likely contain PII so we'd likely want to respect the config.send_default_pii setting. Is capturing these values something we need right away, or can it wait for a future PR?
• Should we add configuration to opt-out of the Redis tracing?
• What should the OP_NAME be? Right now I've set it to "db.redis.command", but could be "redis.command" or simply "redis"?

[st0012]

@lewispb thanks for the work! I think you've done a fantastic job on this 👍
but it may take another week or 2 for me to start merging changes for the 5.1.0 release. so I will give detailed review around then.

[lewispb]

No problem @st0012, thanks for looking!

[st0012]

I think the implementation looks good and logging is probably the best place to patch for this 👍

I left 2 comments but generally it's ready to merge. After merging I'll play a bit with it and perhaps add some screenshot to the changelog & improve some tests around SDK initialization check.

[st0012]

I think it's ok to put this in spec_helper and before require "sentry-ruby". Then we don't need to force the redis.rb reload anymore. Wdyt?

[lewispb]

Good idea!

[st0012]

Some folks might find it useful to also capture the values of the Redis commands. That would likely contain PII so we'd likely want to respect the config.send_default_pii setting. Is capturing these values something we need right away, or can it wait for a future PR?

I can implement that in a separate PR and ask for your opinion, wdyt?

Should we add configuration to opt-out of the Redis tracing?

I haven't received any request to opt-out specific tracing integration. So it's probably not needed now.
Even if we do receive that, it'll be a broader discussion around the entire SDK design. So it's out of this PR's scope.

What should the OP_NAME be? Right now I've set it to "db.redis.command", but could be "redis.command" or simply "redis"?

I think db.redis.command matches the recommended span names. @sl0thentr0py can you confirm this is the case?

[sl0thentr0py]

yes that span status is correct, only the db. prefix matters currently.

[sl0thentr0py]

thanks for the contribution @lewispb!
There is a high-level architecture discussion that I wanted to get into at some point and this new redis integration is a good opportunity to do that.

In the ruby SDK currently, we have separate gems encapsulating specific integrations (rake is a one-off here that has it's own :skip_rake_integration config param). In other sentry SDKs, we generally use a config.integrations = [:foo, :bar] list to enable/disable specific integrations. Ruby is of course different here because of how Gemfiles work and how autoloading works in rails, so this different architecture is certainly justified.

Now with redis, we're adding it to the main sentry-ruby gem, and so it lies in neither broader architecture. I don't want to start too many large changes now, but it would be nice if we come up with some kind of convention and all agree going forward on how to add new integrations to the ruby SDK.

[st0012]

There is a high-level architecture discussion that I wanted to get into at some point and this new redis integration is a good opportunity to do that.

I think there are different categories in terms of "integrations":

1. Libraries that are opted-in very intentionally, like Rails, Sidekiq...etc.
2. Default-like libraries (90% of the apps have them), like Rake or Rack.
3. Low-level components like net-http and redis. They usually power the libraries like 1) and other gems, but not used directly.

For 1), the current approach is to ask users to install the related sentry-* gem. I think it's makes sense as they are aware of the library/framework they chose and should be motivated to install the related Sentry integration.

For 2), the current approach is to have their code in sentry-ruby but the integration is handled case by case:

• Because Rack middleware requires explicit API to inject a middleware, so we let user do that themselves.
• On the other hand, Rake integration is more like a patch-based approach. So we apply that by default.
  • However, some users are not happy about that so we provided options to opt-out

For 3), the current approach is to detect if those components exist in the app, and opt-in for the user.

Let me explain a bit on my decision on 3):

Most users don't spend time configuring their Sentry SDK. They copy and paste the configuration from documentation or setup wizard, check if errors go through, and then forget about it. I have 2 examples for this assumption:

1. We mention config.traces_sampler and config.traces_sample_rate, and tell users to pick one of them, which looks like this:

  # To activate performance monitoring, set one of these options.
  # We recommend adjusting the value in production:
  config.traces_sample_rate = 0.5
  # or
  config.traces_sampler = lambda do |context|
    0.5
  end

And guess what happened, many users leave both of them uncommented 😂

2. There were many problems on the ActiveSupport breadcrumb logger (not the logger code itself but they way it collects all the data without proper filtering). While the logger was added years ago to sentry-raven, we didn't receive much issues report until the release of sentry-rails a year ago. Why? Because most users didn't even know it's a thing and never used it before. My the other project is one of those.

So my concern is that most users may not aware of those lower-level components and thus won't intentionally make the effort to opt-in the integration. That's why I didn't want to make sentry-redis or sentry-net_http, because I know many users won't even know them exist even if they do use those libraries somewhere. And that'll be a shame because they also provide valuable information. You can also argue that it'll give users an impression that sentry-ruby provides much less feature when compare to other services' SDKs, where those integrations are opted-in by default.

[codecov-commenter]

Codecov Report

Merging #1697 (d258ebb) into master (8538717) will decrease coverage by 2.22%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1697      +/-   ##
==========================================
- Coverage   98.39%   96.16%   -2.23%     
==========================================
  Files         136      115      -21     
  Lines        7833     6696    -1137     
==========================================
- Hits         7707     6439    -1268     
- Misses        126      257     +131     

Impacted Files	Coverage Δ
sentry-ruby/lib/sentry/configuration.rb	98.41% <ø> (ø)
...ntry-ruby/spec/sentry/client/event_sending_spec.rb	99.60% <ø> (-0.01%)	⬇️
sentry-ruby/lib/sentry-ruby.rb	94.33% <100.00%> (+0.05%)	⬆️
sentry-ruby/lib/sentry/redis.rb	100.00% <100.00%> (ø)
...y-ruby/spec/sentry/breadcrumb/redis_logger_spec.rb	100.00% <100.00%> (ø)
sentry-ruby/spec/sentry/redis_spec.rb	100.00% <100.00%> (ø)
sentry-rails/lib/sentry/rails/railtie.rb	26.02% <0.00%> (-71.24%)	⬇️
sentry-rails/lib/sentry/rails/active_job.rb	33.33% <0.00%> (-66.67%)	⬇️
sentry-rails/lib/sentry/rails/tracing.rb	34.21% <0.00%> (-65.79%)	⬇️
...entry-rails/lib/sentry/rails/capture_exceptions.rb	36.00% <0.00%> (-60.00%)	⬇️
... and 33 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8538717...d258ebb. Read the comment docs.

[lewispb]

I can implement that in a separate PR and ask for your opinion, wdyt?

Sounds great, thanks.

I haven't received any request to opt-out specific tracing integration. So it's probably not needed now.

Cool, OK!

[sl0thentr0py]

Completely agree on the zero-config front, that is also very much our design philosophy as well.
It's just that these 3 implicit categories add some cognitive overhead for the end-user too and make docs potentially confusing. Other SDKs just have them all in the same 'integration' category and a lot of them are also enabled by default so most users don't have to care.

Either way, I don't wanna bikeshed on this too much, but it's instructive that we had this conversation. Let's just go with the PR in its current form. 👍 We can agree on a simple rule like 'large' frameworks go in separate gems and everything else goes in sentry-ruby. If we add more optional things in the future, we will have to figure out how we let users switch them on/off.

[st0012]

👍 Thanks for this great PR!

[st0012]

@lewispb sorry that I'm not able to open the PR for command arguments before the 5.1.0 release. but I have added #1718 as a followup task.

[lewispb]

Sure, no problem! I could also take a look at it if you like, although probably not till next week?

[st0012]

@lewispb I scheduled it for 5.2.0, which will be released in March or April. so there's no rush on that and whoever has time can give it a try.