Add cache_key_enabled option for ActiveSupport

[bouwkast]

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 #4017

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!

[pr-commenter]

Benchmarks

Benchmark execution time: 2024-10-23 21:38:15

Comparing candidate commit 45ef5e6 in PR branch steven/activesupport-cachekey-config with baseline commit 38f6cc1 in branch master.

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

scenario:profiler - sample timeline=false

• 🟥 throughput [-0.675op/s; -0.658op/s] or [-9.803%; -9.558%]

[marcotc]

Suggested change

	set_cache_key(span, key, multi_key) if Datadog.configuration.tracing[:active_support][:cache_key_enabled]
	set_cache_key(span, key, multi_key) if configuration[:cache_key_enabled]

[bouwkast]

@marcotc this seems to be not work (I'm likely doing something wrong)

      Failure/Error: set_cache_key(span, key, multi_key) if configuration[:cache_key_enabled]
      
      NameError:
        undefined local variable or method `configuration' for Datadog::Tracing::Contrib::ActiveSupport::Cache::Instrumentation:Module

[marcotc]

Great work! Clean and well tested 👍

I thing that is missing is the public facing documentation, at https://github.com/DataDog/dd-trace-rb/blob/master/docs/GettingStarted.md#active-support.

[datadog-datadog-prod-us1]

⚪ Code Quality Violation

Suggested change

	get = spans[0]
	get = spans.first

Improve readability with first (...read more)

This rule encourages the use of first and last methods over array indexing to access the first and last elements of an array, respectively. The primary reason behind this rule is to improve code readability. Using first and last makes it immediately clear that you are accessing the first or last element of the array, which might not be immediately obvious with array indexing, especially for developers who are new to Ruby.

The use of these methods also helps to make your code more idiomatic, which is a crucial aspect of writing effective Ruby code. Idiomatic code is easier to read, understand, and maintain. It also tends to be more efficient, as idioms often reflect patterns that are optimized for the language.

To adhere to this rule, replace the use of array indexing with first or last methods when you want to access the first and last elements of an array. For instance, instead of arr[0] use arr.first and instead of arr[-1] use arr.last. However, note that this rule should be applied only when reading values. When modifying the first or last elements, array indexing should still be used. For example, arr[0] = 'new_value' and arr[-1] = 'new_value'.

    

[datadog-datadog-prod-us1]

⚪ Code Quality Violation

Suggested change

	get = spans[0]
	get = spans.first

Improve readability with first (...read more)

This rule encourages the use of first and last methods over array indexing to access the first and last elements of an array, respectively. The primary reason behind this rule is to improve code readability. Using first and last makes it immediately clear that you are accessing the first or last element of the array, which might not be immediately obvious with array indexing, especially for developers who are new to Ruby.

The use of these methods also helps to make your code more idiomatic, which is a crucial aspect of writing effective Ruby code. Idiomatic code is easier to read, understand, and maintain. It also tends to be more efficient, as idioms often reflect patterns that are optimized for the language.

To adhere to this rule, replace the use of array indexing with first or last methods when you want to access the first and last elements of an array. For instance, instead of arr[0] use arr.first and instead of arr[-1] use arr.last. However, note that this rule should be applied only when reading values. When modifying the first or last elements, array indexing should still be used. For example, arr[0] = 'new_value' and arr[-1] = 'new_value'.

    

[ivoanjo]

(I guess this is related to #4017 ?)

[bouwkast]

(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.

[delner]

Overall the implementation is clean! I do have one suggestion for the public API, I think should be weighed before we merge.

[delner]

I want to caution that any addition of configuration like this is an expansion of the public API, so in that sense, we're bound to this method, and other measures (in other integrations) should be consistent with this style.

Allowing this to be disabled is value-add in capability. But users may find it insufficient, and may also need a more sophisticated quantization feature that itself may require more configuration options.

If we stick with cache_key_enabled, and we wanted to add regex capability (for example), then we would likely have to do cache_key_exclude_pattern, cache_key_include_pattern, etc... this is OK, but idiomatically, it may be better to have a subgroup instead (e.g. cache_key.enabled, cache_key.include_pattern, cache_key.exclude_pattern.)

So the time to decide on this behavior is now. I would recommend nesting this setting in its own subgroup to produce the cache_key.enabled behavior, such that we have the option of idiomatically expanding the API later without introducing a breaking change down the line.

[bouwkast]

If we stick with cache_key_enabled, and we wanted to add regex capability (for example), then we would likely have to do cache_key_exclude_pattern, cache_key_include_pattern, etc... this is OK, but idiomatically, it may be better to have a subgroup instead (e.g. cache_key.enabled, cache_key.include_pattern, cache_key.exclude_pattern.)

I like this idea, it seems much more effective and better long-term so I'll refactor to that.