Allow ability to disable Redis capturing the args #1276
Conversation
For security (& memory allocation reasons) we want to disable this as the keys could contain sensitive information. Allow for this to be disabled.
There was a problem hiding this comment.
@callumj Thanks for the contrib. A few nits and things I wanted to point out but generally this is something I'm on board with.
-
I think our tracing ingest pipeline already has some hooks in place for the security aspect of this, https://docs.datadoghq.com/tracing/setup_overview/configure_data_security/?tab=redis#agent-trace-obfuscation where you can obfuscate
redis.raw_commandas well as the resource. Completely fine with addressing things at the tracing client level since it also has memory perf improvements, but more out of curiosity, have y'all tried that agent-based approach and found it lacking? Or just simply prefer to address things at the app level? If there were any issues with the agent approach, certainly that feedback is valuable, and I'd be happy to work with the folks maintaining the datadog-agent to try to improve things there. -
With the implementation as is, we're not setting any resource, which is a bit problematic. I think at the datadog-agent level there's some normalization code that kicks in and assigns the
span.nameto be the resource if the resource is nil, but it's probably better to at least do that at the tracer level. If there's a happy path to setting just the operationget / set / etc etci think that would be even better, but i'd like your opinion on this, i left a longer comment in the review.
Overall, great work, thanks for the contribution!
Good call out - normally I would use the span filter as that is very powerful but in this area I figured it would be best to offer this as an option because you then avoid some allocations if you are using Redis really heavily and might be on an older interpreter. I would prefer controlling things at the app level because if we had a shared agent (like a sidecar) it would avoid config changes and instead just require simpler app level changes. |
There was a problem hiding this comment.
Very nice work, @callumj! I added a few comments, but looks pretty good overall.
@ericmustin's feedback (which I see you have already read) has the main points we'd like to addressed to get it merged.
Co-authored-by: Marco Costa <mmarcottulio@gmail.com>
| @@ -59,10 +59,10 @@ def call_pipeline(*args, &block) | |||
| pin.tracer.trace(Datadog::Contrib::Redis::Ext::SPAN_COMMAND) do |span| | |||
| span.service = pin.service | |||
| span.span_type = Datadog::Contrib::Redis::Ext::TYPE | |||
| commands = args[0].commands.map { |c| Datadog::Contrib::Redis::Quantize.format_command_args(c) } | |||
| commands = get_pipeline_commands(args) | |||
There was a problem hiding this comment.
@marcotc Assignment Branch Condition size has been violated because of this change: do you normally extract these out or ignore the cop?
There was a problem hiding this comment.
Your call, to be honest. I'm ok with either approach.
I noticed you extracted it, which actually looks much nicer!
There was a problem hiding this comment.
Sorry @marcotc the issue is in patch_redis_client (which does the class_eval).
There was a problem hiding this comment.
Go ahead and add an "Assignment Branch Condition" exception here, no worries.
There was a problem hiding this comment.
Done, thank you @marcotc
marcotc
added
community
|
Thank you @marcotc, do you know when |
|
@callumj we have one cross-team effort across all Datadog language tracers that we are almost ready to ship and we'd like to sync it with the next release. That work is we expect to be ready next week, so that's our timeline as of now. |
For security (& memory allocation reasons) we want to disable this as the keys could contain sensitive information. Allow for this to be disabled.