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:Omit command arguments from span.resource by default #3235

Merged
merged 2 commits into from
Nov 1, 2023

Conversation

marcotc
Copy link
Member

@marcotc marcotc commented Oct 30, 2023

For release notes

The Datadog Agent removes command arguments from the resource name. However there are cases, like compressed keys, where this obfuscation cannot correctly remove command arguments. To safeguard that situation, the resource name set by the tracer will only be the command (e.g. SET) with no arguments. To retain the previous behavior and keep arguments in the span resource, with the potential risk of some command arguments not being fully obfuscated, set DD_REDIS_COMMAND_ARGS=true or option c.instrument :redis, command_args: true.

What does this PR do?

This PR changes the value of span.resource for the Redis tracing instrumentation to only capture the command name by default (e.g. GET, BLPOP).

It's possible to revert back to the previous default, capturing command arguments, with the environment variable DD_REDIS_COMMAND_ARGS or option command_args.

Motivation:

Redis command arguments can contain sensitive information and thus must not be captured by default.

Additional Notes:

How to test the change?

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.

Unsure? Have a question? Request a review!

@github-actions github-actions bot added integrations Involves tracing integrations tracing labels Oct 30, 2023
@marcotc marcotc self-assigned this Oct 30, 2023
@marcotc marcotc added this to the 1.16.0 milestone Oct 30, 2023
@marcotc marcotc marked this pull request as ready for review October 30, 2023 21:55
@marcotc marcotc requested review from a team as code owners October 30, 2023 21:55
docs/GettingStarted.md Outdated Show resolved Hide resolved
Co-authored-by: Ursula Chen <58821586+urseberry@users.noreply.github.com>
@codecov-commenter
Copy link

Codecov Report

Merging #3235 (47d136c) into master (4d22403) will increase coverage by 0.00%.
Report is 3 commits behind head on master.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #3235   +/-   ##
=======================================
  Coverage   98.22%   98.22%           
=======================================
  Files        1252     1252           
  Lines       72201    72173   -28     
  Branches     3353     3348    -5     
=======================================
- Hits        70917    70890   -27     
+ Misses       1284     1283    -1     
Files Coverage Δ
...og/tracing/contrib/redis/configuration/settings.rb 100.00% <100.00%> (ø)
...b/datadog/tracing/contrib/redis/instrumentation.rb 100.00% <100.00%> (ø)
lib/datadog/tracing/contrib/redis/tags.rb 100.00% <100.00%> (ø)
.../datadog/tracing/contrib/redis/trace_middleware.rb 100.00% <100.00%> (ø)
.../datadog/tracing/contrib/rails/redis_cache_spec.rb 99.23% <100.00%> (ø)
...adog/tracing/contrib/redis/instrumentation_spec.rb 100.00% <ø> (ø)
spec/datadog/tracing/contrib/redis/redis_spec.rb 100.00% <100.00%> (ø)
...c/datadog/tracing/contrib/redis/shared_examples.rb 100.00% <100.00%> (ø)

... and 3 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@marcotc marcotc merged commit 713fb1e into master Nov 1, 2023
217 checks passed
@marcotc marcotc deleted the redis-only-command-default branch November 1, 2023 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrations Involves tracing integrations tracing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants