Instrument ActiveSupport Caches - #74294
Conversation
| # Convenience method to put a single metric to CloudWatch. | ||
| # Accepts a single '[namespace]/[metric_name]' name parameter | ||
| # and a standard Ruby hash to specify dimension key/values. | ||
| # Accepts a namespace, a metric_name, and a standard Ruby | ||
| # hash to specify dimension key/values. | ||
| # | ||
| # @param [String] name in the form of 'namespace'/'metric_name' | ||
| # @param [String] namespace | ||
| # @param [String] metric_name | ||
| # @param [Number] value | ||
| # @param [Hash{Symbol => String}] dimensions | ||
| # @param [Hash] options Additional keyword arguments to be merged | ||
| # into the {Aws::CloudWatch::Types::MetricDatum} object. | ||
| def self.put(namespace, metric_name, value, dimensions, **options) |
There was a problem hiding this comment.
Looks like the jerk who updated this method signature failed to update the comment to match, so I'm fixing it up while I'm in the area 🙃
Hamms
changed the title
cat5inthecradle
left a comment
cat5inthecradle
left a comment
There was a problem hiding this comment.
LGTM with one CloudWatch namespacing suggestion.
| Cdo::Metrics.put( | ||
| 'Infrastructure', | ||
| 'ActiveSupportCacheRead', | ||
| 1, | ||
| { | ||
| Environment: CDO.rack_env, | ||
| Hit: event.payload[:hit].to_s, | ||
| Store: event.payload[:store] | ||
| } | ||
| ) |
There was a problem hiding this comment.
What about doing two metrics: ActiveSupportCacheHit and ActiveSupportCacheMiss, keeping Environment and Store as dimensions and toggling which metric based on event.payload[:hit]? I think that aligns more with what I'm used to seeing in CloudWatch.
| Environment | Store | Hit | Name |
|---|---|---|---|
| production | mystore | true | ActiveSupportCacheRead |
| production | mystore | false | ActiveSupportCacheRead |
vs
| Environment | Store | Name |
|---|---|---|
| production | mystore | ActiveSupportCacheHit |
| production | mystore | ActiveSupportCacheMiss |
| production | mystore | ActiveSupportCacheBytesRead |
Functionally the same, but it'll play nicer in the CloudWatch UI, and puts it right next to the other metric in this PR.
| 'ActiveSupportCacheRead', | ||
| 1, | ||
| { | ||
| Environment: CDO.rack_env, |
There was a problem hiding this comment.
I prefer using the fully qualified domain name instead of environment type to disambiguate between multiple deployments of our application that share the same environment type. Can we name the Dimension Host and set it to the dashboard domain name?
code-dot-org/lib/cdo/app_server_hooks.rb
Line 127 in 0e6a2f6
| 'ActiveSupportCacheBytesRead', | ||
| result.try(:bytesize) || 0, | ||
| { | ||
| Environment: CDO.rack_env, |
There was a problem hiding this comment.
Same as above. Can we name this Dimension Host and set it to the dashboard hostname?
code-dot-org/lib/cdo/app_server_hooks.rb
Line 127 in 0e6a2f6
|
Can we publish metrics conditionally? Here's logic we've been using lately (production, managed test server, and adhoc) code-dot-org/lib/cdo/app_server_hooks.rb Line 86 in 0e6a2f6 |
3 participants
Part of our ongoing work to address runaway memory usage on frontend web application servers by reexamining our caching strategy.
I want to track hit rate to get better insight into how per-process versus per-server caches improve our efficiency, as well as just generally to get a look at how well we're using our cache. I'd like to track total bytes read out of the cache so we can evaluate whether it would be feasible to fetch it over the network; from Redis, for example.
Links
https://guides.rubyonrails.org/active_support_instrumentation.html#active-support-caching
Testing story
Tested on an adhoc; repeatedly fetched
/courses/allthethingscourse/units/1with curl, and confirmed that results are as expected:Follow-up work
We probably want to remove the byte size metric after we've gathered a representative amount of data, since it's both gross and is adding a little bit of extra work to a very high-throughput method. I don't anticipate it being a problem in the short term, though.