Report the cache key namespace on ActiveSupport cache spans - #6181
Report the cache key namespace on ActiveSupport cache spans#6181jonthedecepticon wants to merge 6 commits into
Conversation
`rails.cache.key` carries the key the user supplied, before Rails prefixes the store namespace onto it while normalizing. For a store configured with a namespace the tag is therefore ambiguous: `Rails.cache` with `namespace: "my_model"` reports `key: 123` for a key that is really `my_model:123`. Add a `rails.cache.namespace` tag, set from the same options Rails resolves the namespace from, on both the event-based and the legacy monkey-patched paths. Callable namespaces are resolved, and options passed to the call take precedence over the ones the store was configured with, matching `ActiveSupport::Cache::Store#namespace_key`. ActiveSupport only forwards the call options to the `delete` event since Rails 8, so the namespace is backfilled from the store on older versions to keep the tag consistent across the supported range.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 098f4123f7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Do not resolve callable namespaces. Rails resolves them while normalizing the key, and instrumentation was calling them a second time, in the legacy paths before Rails did. A callable namespace is now left untagged. Read the namespace of a `read_multi` call from its trailing options hash, which Rails accepts alongside the keyword form. It was being left in the keys while the tag fell back to the namespace of the store. Carry the namespace of a `delete` call from `#delete` itself on Rails < 8, where the event payload has neither the store options nor the ones given to the call. The backfill reported the namespace of the store even when the call overrode it or disabled it with `namespace: nil`.
|
|
||
| if Datadog.configuration.tracing[:active_support][:cache_key].enabled | ||
| set_cache_key(span, key, mapping[:multi_key]) | ||
| set_cache_namespace(span, payload[:namespace]) |
There was a problem hiding this comment.
In the scope of the operation we are instrumenting here, the callable namespace might not yet be resolved here, in this method, but it has to be resolved by ActiveSupport by active support at some point of the operation, before control is returned to the user's application.
Have you tried to get the resolved namespaced, at some point of the execution after ActiveSupport resolves it? If you tried, and it was not worth the complexity, it's ok, but I didn't see a discussion/rationale for not retrieving it.
There was a problem hiding this comment.
Yes, and it works out. Rails resolves the callable itself while normalizing the key, in #namespace_key, which is the single place it applies the namespace and has existed since 5.2. That method returns the key it was given with "#{namespace}:" in front, so prepending it recovers the resolved value without instrumentation ever calling the callable.
Done in dd83ab6. The tag moved to on_finish, since Rails normalizes the key inside the operation for most events on Rails < 8.
On Rails 6.1, with a proc namespace:
| operation | namespace tag |
|---|---|
read |
resolved |
write |
resolved |
read_multi |
resolved |
write_multi |
resolved |
fetch |
resolved, both spans |
fetch_multi |
resolved |
delete |
not tagged |
Calls to the proc are unchanged from uninstrumented Rails, which the specs assert per operation.
Two limits:
deleteon Rails < 8 is the gap. Its event carries no options, and putting the callable in the payload is what would let Rails' debug logging normalize the key with it. Rails 8+ forwards the options, so it is tagged there.- Rails < 5.2 inlines the namespacing into
#normalize_key, where the key is not yet expanded and the prefix cannot be isolated, so a callable stays untagged.
Cost is one bytesize comparison and one thread-local write per key normalization. On a MemoryStore read, the fastest store there is, 5.2µs to 5.6µs; under 1% of a traced operation or any network-backed store.
Record what ActiveSupport resolves the namespace to while normalizing the key, so a callable namespace is reported without instrumentation invoking it. `#namespace_key`, present since Rails 5.2, is the single place the namespace is applied, so the resolved value is the prefix it adds. `#delete` on Rails < 8 is the one operation left out: its event carries no options, and putting the callable in the payload would let Rails' debug logging invoke it a second time.
|
`Layout/EmptyLinesAroundAccessModifier` wants a blank line before `private` when a comment precedes it. Move the note into the module doc instead.
|
Pushed c4f691e: fixes the single The workflow runs on that commit are all sitting in |
3 participants
What does this PR do?
Adds a
rails.cache.namespacetag torails.cachespans, carrying the namespace Rails prefixes onto the cache key.The tag is set on both instrumentation paths (the
ActiveSupport::Notificationsevents and the legacy monkey patches forfetch,read_multiandfetch_multi). A namespace passed to the call takes precedence over the one the store was configured with, andnamespace: nilfor the call disables the one of the store, matchingActiveSupport::Cache::Store#namespace_key.ActiveSupport only started forwarding the call options to the
deleteevent in Rails 8. On older versions the namespace is carried from#deleteitself, where both the store options and the ones given to the call are still in scope, using the same polyfill approach already used forpayload[:store]on Rails < 6.1.Motivation:
Fixes #3808.
rails.cache.keyreports the key the user supplied, before normalization applies the namespace, so on a namespaced store the tag does not identify the entry the backend was actually addressed with. A store configured withnamespace: "my_model"reportskey: 123for a key that is reallymy_model:123, which makes the span hard to act on when several stores share short keys.Change log entry
Yes. ActiveSupport cache spans now report the cache key namespace in the
rails.cache.namespacetag.Additional Notes:
The namespace is reported as its own tag rather than being folded into
rails.cache.key, so existing dashboards and monitors built on the key tag keep working.A callable namespace is reported as the value ActiveSupport resolved it to, recorded by prepending
#namespace_key, the single place Rails applies the namespace. Instrumentation never invokes the callable itself, so the number of calls to it is unchanged.deleteon Rails < 8 is the exception: its event carries no options, and putting the callable in the payload would let Rails' debug logging invoke it a second time. Rails < 5.2 is the other exception, as it inlines the namespacing into#normalize_key, where the key is not yet expanded and the prefix cannot be isolated.Namespace resolution is gated behind the existing
cache_key.enabledsetting, since the namespace is part of the key.How to test the change?
spec/datadog/tracing/contrib/rails/cache_spec.rbcoversread,write,delete,read_multi,write_multi,fetchandfetch_multi, plus per-call overrides, a per-callnamespace: nil, options given as a trailing hash, callable namespaces,cache_key.enabled = false, and a store with no namespace.