fix: do not expose auth params with Redis 5 #1370
Conversation
zvkemp
requested review from
fbogsany,
mwear,
robertlaurin,
dazuma,
ericmustin,
arielvalentin,
ahayworth,
plantfansam,
robbkidd,
simi,
kaylareopelle,
xuan-cao-swi and
a team
as code owners
| @@ -297,7 +338,7 @@ def redis_with_auth(redis_options = {}) | |||
| _(exporter.finished_spans.size).must_equal 3 | |||
|
|
|||
| set_span = exporter.finished_spans[0] | |||
| _(set_span.name).must_equal 'AUTH' | |||
| _(set_span.name).must_equal(redis_gte_5? ? 'PIPELINED' : 'AUTH') | |||
There was a problem hiding this comment.
We may want to look into splitting these tests up. I know it will lead to duplication but conditional logic in tests that go beyond using skip become difficult to reason about IMO.
There was a problem hiding this comment.
Do you mean to individually duplicate the tests that should be forked, or duplicate the file entirely for v5?
| redis_options[:host] = redis_host | ||
| redis_options[:port] = redis_port | ||
| RedisClient.new(**redis_options) | ||
| redis_options[:password] ||= password |
There was a problem hiding this comment.
Why are these lazily initialized?
There was a problem hiding this comment.
Seemed appropriate not to clobber the options from the argument if they are given — the redis_with_auth(host: 'example.com' ... test fails without this.
|
|
||
| skip('FIXME: what is this intended to test?') |
There was a problem hiding this comment.
I missed this skip during my last review.
I think what the intent was was to ensure the span had the net.peer.* attributes where captured during a network failure to execute the auth command.
Are there changes in how the driver works that would make this impossible to do?
There was a problem hiding this comment.
Ah yes, forgot to follow up on that one. RedisClient tries to do the connect before any of the middlewares are invoked, so we don't get a span here under that implementation. I don't think it's within the scope of the PR to change that behavior (previously, the test was erroneously running the RedisV4 client instrumentation), but I didn't want to complete clobber the expectations (these still exist in the other test file). I can clarify the comment so that if that span is ever added for RedisClient, it should have those attributes.
There was a problem hiding this comment.
I see. In that case then I think it's fine to delete the test since we won't be able to get a span generated from it.
Additionally: - Add a redis 5 appraisal - use RedisClient in RedisClient tests
|
@arielvalentin thanks for your feedback; I clarified some of the test changes. |
There was a problem hiding this comment.
Thanks for your work on this, @zvkemp!
it may be more clear to group the tests based on their API rather than which patch/middleware applies.
I like this idea. I don't think refactoring the tests needs to be accomplished in this PR. What do you think about opening an issue to save this work for a future date?
Redis 5 is a wrapper around RedisClient, so the middleware-based instrumentation (intended for RedisClient) applies here.
redis_gte_5?helper method.queuemethod invocations (deprecated in v4, removed in v5) have been replaced withpipelinedRedis.newdoes not use RedisClient internally).