Fix configure_onto when redis instance is early initialized #3278

Drowze · 2023-11-23T11:24:35Z

What does this PR do?
Should fix #3277

Motivation:
Datadog.configure_onto doesn't work if the Redis instance was initialized before Datadog redis tracing is configured.

Additional Notes:
None.

How to test the change?
It is hard to automatically test this, but to manually test, simply use Datadog.configure_onto in a Redis instance that was initialized before the Redis patch is applied (i.e. before the Datadog Redis tracing is configured).

For Datadog employees:

If this PR touches code that signs or publishes builds or packages, or handles
credentials of any kind, I've requested a review from @DataDog/security-design-and-guidance.
This PR doesn't touch any of that.

lib/datadog/tracing/contrib/redis/patcher.rb

TonyCTHsu · 2023-11-23T14:27:14Z

👋 Thanks @Drowze . I can reproduced the issue reported, it seems like Redis 3 and 4 are impacted.

The fix seems feasible and I would like to move forward with test provided. 😄

Let me know how I can help.

Drowze · 2023-11-24T15:07:04Z

Hey @TonyCTHsu thank you very much for taking a look at the issue and even providing a test-case for it! (I wasn't sure about testing it as it was related to the patch lifecycle - looks like adding a new rake task was the way to go!)

Looking forward to v1.18.0 now 😄

marcotc · 2023-11-27T19:43:42Z

spec/datadog/tracing/contrib/redis/regression_spec.rb

+require 'ddtrace'
+
+# The regression task must be a standalone task due to the life cycle of patcher
+RSpec.describe 'Regression', skip: Gem::Version.new(Redis::VERSION) >= Gem::Version.new('5') ? 'Not supported' : false do


Is this really a regression, or just a test to ensure that Redis < 5 can be patched if initialized before Datadog?

marcotc · 2023-11-27T19:45:27Z

spec/datadog/tracing/contrib/redis/regression_spec.rb

+require 'ddtrace'
+
+# The regression task must be a standalone task due to the life cycle of patcher
+RSpec.describe 'Regression', skip: Gem::Version.new(Redis::VERSION) >= Gem::Version.new('5') ? 'Not supported' : false do


What happens in Rails > 5 that makes this test not necessary anymore? Did Redis fix something internally that makes it work?
If so, this knowledge should be captured in a comment in either lib/datadog/tracing/contrib/redis/patcher.rb or lib/datadog/tracing/contrib/redis/integration.rb

Hey @marcotc 👋
Thank you for your comment! It made me realize that my changes were NOT actually working for configure_onto for Redis >=5. Ops!
(please note: no Rails involved here!)

Drowze · 2023-11-30T14:21:29Z

Added a couple commits:

One commit that:
- Removes mentions to regression - I agree this doesn't look like a regression!
- Make sure this test runs with Redis all redis supported versions (3, 4 and 5)
- And more importantly: makes configure_onto work both in Redis < 5 and Redis >=5 😄
And another commit that enables the miniapp_spec to also test Redis >= 5

@marcotc @TonyCTHsu can you have another look please?

EDIT: just rebased with latest master as I saw some failed tests on ruby 3.3 (non related to this PR)
EDIT 2: got another flaky test... This time on ruby 2.2

TonyCTHsu · 2023-12-06T11:25:09Z

lib/datadog/tracing/contrib/redis/trace_middleware.rb

-
-            Datadog.configuration.tracing[:redis, redis_config.server_url].to_h.merge(custom)
+          def resolve(config)
+            # support both:


Thanks! @Drowze to make this work for Redis 5+. However, I was not entirely sure about supporting it because we want to encourage user to leverage the Redis 5+ public interface with

Redis.new(..., custom: { datadog: { ... } })

Furthermore, there is another concern: We have made our redis patcher to detect different scenarios because the target, client instance, was harboured within the redis gem and extracted redis-client after version 5. Which means redis-client could be installed without redis ( an example would be Sidekiq 7) . In such scenario, this line from patcher would be broken because of missing Redis constant.

Things get a bit tricky when using standalone redis-client, because the client instance can be interacted with directly.

require 'redis_client' redis_client = RedisClient.new redis_client.call("PING")

They might want to pin the client instance directly like this (which currently is not supported):

require 'redis_client' redis_client = RedisClient.new Datadog.configure_onto(redis_client, ...) redis_client.call("PING")

Hey @TonyCTHsu 👋

Based on your reply, I understand that when using Redis 5+, the following no longer works (and is no longer supported):

redis = Redis.new Datadog.configure_onto(redis, service_name: 'my-redis')

I'm updating now the PR based on that 👍

By the way, would we event want to support the following? (just wondering - I prefer to keep that out of the scope of this PR)

require 'redis_client' redis_client = RedisClient.new Datadog.configure_onto(redis_client, ...) redis_client.call("PING")

My feeling is that, since configure_onto is not supported on redis 5+, it would be confusing to have it working with redis-client. Also because I believe redis-client can be initialized with custom options... i.e.:

require 'redis_client' redis_client = RedisClient.new(custom_options: { datadog: { ... } }) redis_client.call("PING")

Also add multiple tests for custom tracing options at instance level: - tests for custom tracing options with Datadog.configure_onto (for redis < 5) - add tests for custom tracing options with redis-client's `custom` options (for redis >= 5)

codecov-commenter · 2024-01-28T04:45:59Z

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (e8b75dd) 98.25% compared to head (4e3a446) 98.24%.
Report is 2 commits behind head on master.

Files	Patch %	Lines
lib/datadog/tracing/contrib/redis/patcher.rb	82.35%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3278      +/-   ##
==========================================
- Coverage   98.25%   98.24%   -0.01%     
==========================================
  Files        1262     1263       +1     
  Lines       73606    73699      +93     
  Branches     3445     3455      +10     
==========================================
+ Hits        72319    72409      +90     
- Misses       1287     1290       +3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Drowze · 2024-01-28T04:57:12Z

Hey! 👋

Sorry for the long wait to come back here - just updated this PR by:

reverting the work bringing Datadog.configure_onto support to Redis 5+ (as we don't want to mess with that since redis-client has a supported API for defining configurations on custom middlewares)
adding a change to trigger a warning everytime a user tries to use Datadog.configure_onto on a Redis instance 5+
- note: added this as I think this may help to inform users when they might be making wrong usage of ddtrace - which I believe might be a particularly common scenario when the user migrates from Redis 4 to Redis 5
- note2: the name of the module is a bit silly (NotSupportedNoticePatch) - definitely open to suggestions there 😄
adding/adapting tests related to using Redis custom tracing options on instance level (which is dependent on whether the Redis version is <5 or >=5)

I think the changes in this PR should make the differences between Redis 4 and Redis 5 tracing at instance level a bit more clear :)

… standalone test tasks

TonyCTHsu · 2024-01-30T11:39:09Z

👋 Thanks @Drowze , NotSupportedNoticePatch is a nice addition!

I have carried on with the implementation, with some changes (I also merged master into this branch. )

Simplify our pin logic during runtime so it no longer depends on stale redis instance. see commit
Since the implementation no longer depends on stale redis instance, I believe we don't need a standalone life cycle test task
Add test for RedisClient for custom usage.

I wonder if you could remind me if I am missing anything? Would you be able to validate this change on your project to confirm the issue being fixed?

If yes, I would ask @marcotc for another round of review

maycmlee

A couple of small suggestions!

docs/GettingStarted.md

Co-authored-by: May Lee <mayl@alumni.cmu.edu>

TonyCTHsu · 2024-02-05T14:09:05Z

👋 @Drowze , 1.20.0 has been released! Give it a try 👍

Drowze requested a review from a team as a code owner November 23, 2023 11:24

github-actions bot added integrations Involves tracing integrations tracing labels Nov 23, 2023

Drowze mentioned this pull request Nov 23, 2023

Can't use Datadog.configure_onto on a Redis instance initiated before Datadog was configured #3277

Closed

Drowze commented Nov 23, 2023

View reviewed changes

lib/datadog/tracing/contrib/redis/patcher.rb Outdated Show resolved Hide resolved

Drowze force-pushed the fix-redis-configure_onto branch 2 times, most recently from 06eb7d6 to 294e11a Compare November 23, 2023 14:09

TonyCTHsu self-requested a review November 23, 2023 14:19

TonyCTHsu added this to the 1.18.0 milestone Nov 24, 2023

TonyCTHsu removed their request for review November 24, 2023 15:02

TonyCTHsu self-assigned this Nov 27, 2023

marcotc reviewed Nov 27, 2023

View reviewed changes

Drowze force-pushed the fix-redis-configure_onto branch from c6a7dd2 to 292ad54 Compare November 30, 2023 14:27

Drowze requested a review from marcotc November 30, 2023 14:28

Drowze force-pushed the fix-redis-configure_onto branch from 5153b58 to e4e2838 Compare November 30, 2023 15:09

TonyCTHsu reviewed Dec 6, 2023

View reviewed changes

TonyCTHsu removed this from the 1.18.0 milestone Dec 6, 2023

Drowze force-pushed the fix-redis-configure_onto branch from e4e2838 to c25be38 Compare January 27, 2024 22:41

Drowze requested a review from a team as a code owner January 27, 2024 22:41

Drowze force-pushed the fix-redis-configure_onto branch from c25be38 to 28f89fc Compare January 28, 2024 02:00

Drowze and others added 2 commits January 27, 2024 23:10

Fix configure_onto when redis instance is early initialized

9a46403

Fail case for redis instance configuration

8ba8f38

Drowze force-pushed the fix-redis-configure_onto branch from 28f89fc to 9da9405 Compare January 28, 2024 02:10

Drowze force-pushed the fix-redis-configure_onto branch from e93d1b2 to 21fcf15 Compare January 28, 2024 04:28

Drowze and others added 5 commits January 28, 2024 05:05

Update gemfiles/*

f80117a

Simplify pin logic

f4bb9fc

Add test for RedisClient with custom

e92714a

Since the implementation changes without stale redis instance, remove…

c3c7053

… standalone test tasks

Merge branch 'master' into fix-redis-configure_onto

ff73a85

Document standalone RedisClient usage

7463884

Drowze requested a review from a team as a code owner January 30, 2024 11:53

maycmlee reviewed Jan 30, 2024

View reviewed changes

docs/GettingStarted.md Outdated Show resolved Hide resolved

TonyCTHsu added this to the 1.20.0 milestone Jan 30, 2024

ekump approved these changes Jan 30, 2024

View reviewed changes

marcotc approved these changes Jan 30, 2024

View reviewed changes

marcotc and others added 2 commits January 30, 2024 10:04

Update docs/GettingStarted.md

d1db442

Co-authored-by: May Lee <mayl@alumni.cmu.edu>

Sacrifice for the linting gods ☠️

4e3a446

marcotc merged commit e50c6b9 into DataDog:master Jan 30, 2024
55 checks passed

TonyCTHsu mentioned this pull request Feb 5, 2024

Bump to version 1.20.0 #3435

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix configure_onto when redis instance is early initialized #3278

Fix configure_onto when redis instance is early initialized #3278

Drowze commented Nov 23, 2023 •

edited

Loading

TonyCTHsu commented Nov 23, 2023 •

edited

Loading

Drowze commented Nov 24, 2023 •

edited

Loading

marcotc Nov 27, 2023

marcotc Nov 27, 2023 •

edited

Loading

Drowze Nov 30, 2023 •

edited

Loading

Drowze commented Nov 30, 2023 •

edited

Loading

TonyCTHsu Dec 6, 2023 •

edited

Loading

Drowze Jan 28, 2024

Drowze Jan 28, 2024 •

edited

Loading

codecov-commenter commented Jan 28, 2024 •

edited

Loading

Drowze commented Jan 28, 2024 •

edited

Loading

TonyCTHsu commented Jan 30, 2024 •

edited

Loading

maycmlee left a comment

TonyCTHsu commented Feb 5, 2024

Fix configure_onto when redis instance is early initialized #3278

Fix configure_onto when redis instance is early initialized #3278

Conversation

Drowze commented Nov 23, 2023 • edited Loading

TonyCTHsu commented Nov 23, 2023 • edited Loading

Drowze commented Nov 24, 2023 • edited Loading

marcotc Nov 27, 2023

Choose a reason for hiding this comment

marcotc Nov 27, 2023 • edited Loading

Choose a reason for hiding this comment

Drowze Nov 30, 2023 • edited Loading

Choose a reason for hiding this comment

Drowze commented Nov 30, 2023 • edited Loading

TonyCTHsu Dec 6, 2023 • edited Loading

Choose a reason for hiding this comment

Drowze Jan 28, 2024

Choose a reason for hiding this comment

Drowze Jan 28, 2024 • edited Loading

Choose a reason for hiding this comment

codecov-commenter commented Jan 28, 2024 • edited Loading

Codecov Report

Drowze commented Jan 28, 2024 • edited Loading

TonyCTHsu commented Jan 30, 2024 • edited Loading

maycmlee left a comment

Choose a reason for hiding this comment

TonyCTHsu commented Feb 5, 2024

Drowze commented Nov 23, 2023 •

edited

Loading

TonyCTHsu commented Nov 23, 2023 •

edited

Loading

Drowze commented Nov 24, 2023 •

edited

Loading

marcotc Nov 27, 2023 •

edited

Loading

Drowze Nov 30, 2023 •

edited

Loading

Drowze commented Nov 30, 2023 •

edited

Loading

TonyCTHsu Dec 6, 2023 •

edited

Loading

Drowze Jan 28, 2024 •

edited

Loading

codecov-commenter commented Jan 28, 2024 •

edited

Loading

Drowze commented Jan 28, 2024 •

edited

Loading

TonyCTHsu commented Jan 30, 2024 •

edited

Loading