dd-trace-rb icon indicating copy to clipboard operation
dd-trace-rb copied to clipboard

Add `cache_key_enabled` option for ActiveSupport

Open bouwkast opened this issue 1 year ago • 1 comments

Change log entry

Adds a new option for ActiveSupport to disable adding the cache_key as a Span Tag with the cache_key_enabled option.

What does this PR do?

  • Adds cache_key_enabled to ActiveSupport (defaults to true)
  • When cache_key_enabled is false it will no longer set the cache_key as a Span Tag

Motivation:

Customer feature request TODO GITHUB ISSUE LINK HERE

Additional Notes:

I have basic grasp on Ruby. An alternative here is to obfuscate/quantize cache_key values when we add them, the issue here is I'm not entirely sure what these values would look like and if they'd be valuable if they were obfuscated/quantized.

How to test the change?

On Windows can't install Ruby :( Can't get WSL to work after Windows Update Can't find tests for ActiveSupport that assert on tags Help

Unsure? Have a question? Request a review!

bouwkast avatar Oct 23 '24 14:10 bouwkast

Benchmarks

Benchmark execution time: 2024-11-29 20:06:00

Comparing candidate commit 3c24e29ee9b71414430954ebbf94b0e6c7c4d1c5 in PR branch steven/activesupport-cachekey-config with baseline commit 28675b6daa898731bc1c9824421b11a539fbde12 in branch master.

Found 0 performance improvements and 1 performance regressions! Performance is the same for 30 metrics, 2 unstable metrics.

scenario:profiler - sample timeline=false

  • 🟥 throughput [-0.479op/s; -0.469op/s] or [-7.318%; -7.164%]

pr-commenter[bot] avatar Oct 23 '24 15:10 pr-commenter[bot]

(I guess this is related to https://github.com/DataDog/dd-trace-rb/issues/4017 ?)

ivoanjo avatar Oct 24 '24 08:10 ivoanjo

(I guess this is related to #4017 ?)

Yes it is @ivoanjo! I posted in the slack channel some context surrounding this

I didn't link to it yet as this was just a draft and was waiting to determine whether or not this is something acceptable to do within the Ruby Datadog SDK or if the expected solution would be to add obfuscation rules to this.

bouwkast avatar Oct 24 '24 12:10 bouwkast

Datadog Report

Branch report: steven/activesupport-cachekey-config Commit report: 3c24e29 Test service: dd-trace-rb

:white_check_mark: 0 Failed, 22050 Passed, 1461 Skipped, 5m 23.17s Total Time

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 97.75%. Comparing base (28675b6) to head (3c24e29).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4022      +/-   ##
==========================================
- Coverage   97.76%   97.75%   -0.01%     
==========================================
  Files        1351     1351              
  Lines       81733    81845     +112     
  Branches     4147     4150       +3     
==========================================
+ Hits        79904    80009     +105     
- Misses       1829     1836       +7     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov-commenter avatar Nov 27 '24 18:11 codecov-commenter

Hey @bouwkast thoughts on merging this in? Looks like a nice improvement

ivoanjo avatar Dec 09 '24 09:12 ivoanjo

Hey @bouwkast thoughts on merging this in? Looks like a nice improvement

I was out on PTO, looked like I just had a couple of conversations that needed to be resolved for it to be merged, so did that

bouwkast avatar Dec 17 '24 14:12 bouwkast

No worries! I was doing a big clean-up and leaving a similar comment on PRs I saw in good shape :D

ivoanjo avatar Dec 17 '24 14:12 ivoanjo