Add cache key to ActiveSupport::Cache events

[jbockler]

Improves performance debugging by adding cache key info to ActiveSupport::Cache events. Doesn't affect #fetch_multi (already uses read events) or 'fetch_hit' and 'generate' sub-events (info comes from parent events).
This change makes it easier to track down cache-related performance issues by providing more context in the main cache operation events.

Background:
When tracking down performance issues, it's currently hard to distinguish between different cache operations. We're using a Redis-based cache, but even there we don't get a hint about which specific cache is being accessed (see issue #822).

[unflxw]

Hi @jbockler!

First of all, thank you for the proposal! I have two concerns, one easy, one not so easy. I am not a Rails expert myself, so please correct me if I'm making the wrong assumptions.

I'll start with the easy one: as the key can potentially be quite long, rather than setting it as the title of the event, I'd strongly recommend serialising it into the body of the event, the second value in the two-element array returned by #format. This is mostly an UI concern, as the title is limited by width; but as a nice side-effect, if serialised into JSON, it will be syntax-highlighted.

Something that might be suitable for the title, however, is the namespace and/or the store, if those are valuable to know. I welcome your thoughts here on whether that makes sense.

Now, the not so easy one: As a general rule, we require that events be low-cardinality. For example, for a database event, SELECT * FROM users WHERE user = ? is an acceptable, low-cardinality event body, but SELECT * FROM users WHERE user = 123 is not acceptable, because it is high-cardinality, as the user ID will change with each request.

With high-cardinality events, AppSignal users can no longer see aggregate statistics on those events, and screens like the "Slow events" view become cluttered with many duplicates of the same event.

The above is all context for the following questions: are cache keys low-cardinality? Or can they be low-cardinality, in some scenarios? What information can we add to make ActiveSupport::Cache events more informative, while upholding the low-cardinality requirement?

My gut feeling is that, when the value for :key is a plain string or an array, that will almost always be high-cardinality, and therefore unsuitable for an event property. But, when it is a hash, then maybe it is a reasonable assumption that the keys of the hash will be stable enough across calls, and therefore low-cardinality?

If the above is correct, then I would recommend only displaying the keys when it is a hash, and not displaying any key information otherwise. I understand, and it is unfortunate, that this makes this functionality less useful.

[backlog-helper]

• This Pull Request has not had any activity in 14 days. Please provide a status update if it is still relevant. Closed it if it is no longer relevant. Or move it to another column if it's blocked or requires another look at it. - (More info)

---

This is a message from the daily scheduled checks.

New issue guide | Backlog management | Rules | Feedback

[tombruijn]

What @unflxw says is correctly. This is not how we want the events to be stored. I think cache keys are not by nature low-cardinality. It depends on the app and scenario of course, but in an app with a user system that has caches per user there will be many cache keys for the same view.

We're closing this. It's not how we want the data to be stored and it can cause issues with how the data is displayed and in extreme cases even the performance on our frontend.