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

Redis instrumentation: support older redis-client #1673

Merged
merged 6 commits into from
Dec 7, 2022
Merged

Conversation

fallwith
Copy link
Contributor

@fallwith fallwith commented Dec 2, 2022

Redis instrumentation:

  • update call_pipelined_with_tracing to yield early (instead of crashing) if a db value cannot be obtained
  • rename _nr_client to _nr_redis_client_config to make it more clear that it holds a config object and not a client object, even though config getters can be called directly on the redis gem's client object
  • update _nr_redis_client_config to add support for redis-client gem versions older than v0.11
  • update _nr_redis_client_config to raise upon failing to get at the underlying config object
  • minor exception handling updates to include class names
  • update the unit tests accordingly

resolves #1650
resolves #1671

Redis instrumentation:

* update `call_pipelined_with_tracing` to yield early (instead of
  crashing) if a db value cannot be obtained
* rename `_nr_client` to `_nr_redis_client_config` to make it more clear
  that it holds a config object and not a client object, even though
  config getters can be called directly on the `redis` gem's client
  object
* update `_nr_redis_client_config` to add support for `redis-client`
  gem versions older than v0.11
* update `_nr_redis_client_config` to raise upon failing to get at the
  underlying config object
* minor exception handling updates to include class names
* update the unit tests accordingly
Ruby 2.2 raises with this test as expected, but not in the way that's
expected. The others in this group are skipping 2.2 already, just have
this one skip it as well
@fallwith
Copy link
Contributor Author

fallwith commented Dec 6, 2022

One way to reproduce the issue:

  1. Create a new Rails 7 app. NOTE: do not use --minimal as it does not prep ActionCable to use Redis for development.
  2. Optionally confirm that config/cable.yml is referencing Redis for development.
  3. Add newrelic_rpm v8.13.0 or v8.13.1 and redis-client < v0.10 in Gemfile and bundle.
  4. Start a redis server.
  5. Run bin/rails console and perform the following in the console:
require 'action_cable/subscription_adapter/redis'
server = ActionCable.server
keys = server.config.cable.symbolize_keys 
conn = ActionCable::SubscriptionAdapter::Redis.redis_connector.call(keys)
listener = ActionCable::SubscriptionAdapter::Redis::Listener.new(:redis, nil)
listener.listen(conn)

A reproduction will produce a crash with a stack trace. A success after applying this PR will produce a constantly polling listening process without any errors (hit ctrl-c to stop it from listening).

The underlying `NewRelic::Agent::Transaction::DatastoreSegment` class
permits `nil` for the database name argument, so it is perfectly fine to
still create an instance of that class to still record all of the
information we _do_ have, despite the db name value being `nil`.

Thank you, @kaylareopelle
update CHANGELOG to reference issue 1650 and PR 1673
kaylareopelle
kaylareopelle previously approved these changes Dec 7, 2022
Copy link
Contributor

@kaylareopelle kaylareopelle left a comment

Choose a reason for hiding this comment

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

Thanks for the nil value adjustment! The comments are all suggestions/questions that don't need to be addressed. Feel free to disregard and merge.

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Contributor

@hannahramadan hannahramadan left a comment

Choose a reason for hiding this comment

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

makes sense!

@github-actions
Copy link
Contributor

github-actions bot commented Dec 7, 2022

SimpleCov Report

Coverage Threshold
Line 93.23% 93%
Branch 84.52% 84%

@fallwith fallwith merged commit edf9956 into dev Dec 7, 2022
@fallwith fallwith deleted the holiday_spiced_sider branch December 7, 2022 18:31
@fallwith fallwith restored the holiday_spiced_sider branch December 8, 2022 09:12
@fallwith fallwith deleted the holiday_spiced_sider branch February 15, 2023 20:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants