Introduce graphql-cache #411

dLobatog · 2020-04-21T13:55:19Z

This PR introduces graphql-cache. I'll comment in each file what are the relevant changes.

dLobatog · 2020-04-21T13:56:12Z

Gemfile

@@ -11,7 +11,7 @@ gem 'puma', '~> 3.12'
 # Build JSON APIs with ease. Read more: https://github.com/rails/jbuilder
 # gem 'jbuilder', '~> 2.5'
 # Use Redis adapter to run Action Cable in production
-gem 'redis', '~> 4.0'
+gem 'hiredis'


This is just a more performant version of the redis gem

dLobatog · 2020-04-21T13:56:42Z

Gemfile

@@ -68,13 +68,14 @@ gem 'prometheus_exporter'

 gem 'activerecord-import'
 gem 'oj'
-gem 'newrelic_rpm'


We are simply not using it and it adds overhead to every API call

dLobatog · 2020-04-21T13:57:27Z

app/graphql/schema.rb

@@ -15,6 +15,5 @@ class Schema < GraphQL::Schema
 mutation Types::Mutation
 lazy_resolve(Promise, :sync)
 use GraphQL::Batch
- use GraphQL::Execution::Interpreter
- use GraphQL::Analysis::AST
+ use GraphQL::Cache


Unfortunately the new interpreter is incompatible with gql-cache (https://graphql-ruby.org/queries/interpreter.html) (stackshareio/graphql-cache#71)

dLobatog · 2020-04-21T13:57:53Z

app/graphql/types/rule.rb

@@ -35,7 +35,7 @@ def references
 if context[:"rule_references_#{object.id}"].nil?
 ::CollectionLoader.for(::Rule, :rule_references)
 .load(object).then do |references|
- references.map { |ref| [ref.href, ref.label] }.to_json
+ references.map { |ref| { href: ref.href, label: ref.label} }.to_json


This just makes sure we always return the same object. Notice how references_from_context doesn't return this object exactly.

dLobatog · 2020-04-21T14:00:23Z

config/initializers/graphql_cache.rb

+ config.namespace = 'compliance' # Cache key prefix for keys generated by graphql-cache
+ config.cache = Rails.cache # The cache object to use for caching
+ config.logger = Rails.logger # Logger to receive cache-related log messages
+ config.expiry = 15552000 # 6 months (in seconds)


We should probably discuss this and even think more seriously about introducing an LRU cache redis instance directly. Fields are cached alongside its updated_at, e.g: compliance:Rule:rules/30b9d4c7-4924-4996-bd89-7e29117fccf7-20200227230834209106:title to force us to realize whether the key can be used or not. It's a sort of 'automatic invalidation' as any time a rule is updated, the old entries will be useless.

So the old entries with older updated_at in the key are kept in redis?

Correct, that's why I'd want to go with an instance like https://redis.io/topics/lru-cache

dLobatog · 2020-04-21T14:00:50Z

config/initializers/graphql_cache.rb

+ module Cache
+ module DeconstructorExtensions
+ def perform
+ return raw.value if method == 'lazy'


GraphQL::Cache::Deconstructor figures out which sort of value each field is returning. Based on that, the current methods cannot handle lazy methods, or Promises. This is just a way to handle those.

dLobatog · 2020-04-21T14:28:19Z

config/initializers/graphql_cache.rb

+
+ class Key
+ def to_s
+ @to_s ||= [


This overrides the way the keys are set - I'll comment more on this in a few

akofink

Looking good so far! Thanks for exploring this option - it looks much cleaner than the heavy caching implementation. I have a couple questions inline.

akofink · 2020-04-21T14:29:40Z

app/graphql/types/interfaces/rules_preload.rb

@@ -4,6 +4,7 @@
 # rule results, compliant,and references
 module RulesPreload
 include GraphQL::Schema::Interface
+ field_class GraphQL::Cache::Field


Is this required for some reason? This module is implemented from classes which inherit from BaseObject, so I thought the field_class would be set already.

akofink · 2020-04-21T14:33:50Z

config/initializers/graphql_cache.rb

+ config.namespace = 'compliance' # Cache key prefix for keys generated by graphql-cache
+ config.cache = Rails.cache # The cache object to use for caching
+ config.logger = Rails.logger # Logger to receive cache-related log messages
+ config.expiry = 15552000 # 6 months (in seconds)


So the old entries with older updated_at in the key are kept in redis?

akofink

Nice work! Thanks for figuring this all out! 🥇 It's definitely faster in the UI. I tested with the in memory cache (not with redis), but the speed should be comparable. I also tested by directly querying the graphql API; Despite caching, it seems much of the time is still spent doing something else (note views and AR only take ~3 milliseconds): the complete response with cache hits still takes ~6 seconds. Hopefully most of that time is dev-related stuff like determining if there are any code changes or something. 🤷 Also note that this graphql query is simple - caching should really speed up those complex queries like we have in the UI.

Started POST "/api/compliance/graphql" for 172.18.0.1 at 2020-04-21 21:57:08 +0000
Processing by GraphqlController#query as JSON
  Parameters: {"query"=>"{\n  allProfiles {\n    policy {\n      id\n    }\n  }\n}", "variables"=>{}, "operationName"=>nil, "graphql"=>{"query"=>"{\n  allProfiles {\n    policy {\n      id\n
    }\n  }\n}", "variables"=>{}, "operationName"=>nil}}
  Account Load (0.3ms)  SELECT  "accounts".* FROM "accounts" WHERE "accounts"."account_number" = $1 LIMIT $2  [["account_number", "1212729"], ["LIMIT", 1]]
  ↳ app/controllers/concerns/authentication.rb:53
  Profile Load (0.6ms)  SELECT "profiles".* FROM "profiles" WHERE "profiles"."account_id" = $1  [["account_id", "a1f12604-dc7c-4b29-8433-78eb4118a3d8"]]
  ↳ app/controllers/graphql_controller.rb:8
  ProfileHost Load (0.4ms)  SELECT "profile_hosts".* FROM "profile_hosts" WHERE "profile_hosts"."profile_id" = $1  [["profile_id", "aa141eed-3237-4d5e-8658-3342b9c9f382"]]
  ↳ app/controllers/graphql_controller.rb:8
  Host Load (0.6ms)  SELECT "hosts".* FROM "hosts" WHERE "hosts"."id" = $1  [["id", "fb04c74a-b6cd-4981-81a5-67cd0b791768"]]
  ↳ app/controllers/graphql_controller.rb:8
Cache miss: (compliance:Profile:profiles/aa141eed-3237-4d5e-8658-3342b9c9f382-20200421171203786540:policy)
Cache miss: (compliance:Profile:profiles/aa141eed-3237-4d5e-8658-3342b9c9f382-20200421171203786540:id)
Completed 200 OK in 6619ms (Views: 0.1ms | ActiveRecord: 27.3ms)

Started POST "/api/compliance/graphql" for 172.18.0.1 at 2020-04-21 21:47:38 +0000
Processing by GraphqlController#query as JSON
  Parameters: {"query"=>"{\n  allProfiles {\n    policy {\n      id\n    }\n  }\n}", "variables"=>{}, "operationName"=>nil, "graphql"=>{"query"=>"{\n  allProfiles {\n    policy {\n      id\n    }\n  }\n}", "variables"=>{}, "operationName"=>nil}}
  Account Load (0.6ms)  SELECT  "accounts".* FROM "accounts" WHERE "accounts"."account_number" = $1 LIMIT $2  [["account_number", "1212729"], ["LIMIT", 1]]
  ↳ app/controllers/concerns/authentication.rb:53
  Profile Load (0.8ms)  SELECT "profiles".* FROM "profiles" WHERE "profiles"."account_id" = $1  [["account_id", "a1f12604-dc7c-4b29-8433-78eb4118a3d8"]]
  ↳ app/controllers/graphql_controller.rb:8
  ProfileHost Load (0.8ms)  SELECT "profile_hosts".* FROM "profile_hosts" WHERE "profile_hosts"."profile_id" = $1  [["profile_id", "aa141eed-3237-4d5e-8658-3342b9c9f382"]]
  ↳ app/controllers/graphql_controller.rb:8
  Host Load (0.8ms)  SELECT "hosts".* FROM "hosts" WHERE "hosts"."id" = $1  [["id", "fb04c74a-b6cd-4981-81a5-67cd0b791768"]]
  ↳ app/controllers/graphql_controller.rb:8
Cache hit: (compliance:Profile:profiles/aa141eed-3237-4d5e-8658-3342b9c9f382-20200421171203786540:policy)
Cache hit: (compliance:Profile:profiles/aa141eed-3237-4d5e-8658-3342b9c9f382-20200421171203786540:id)
Completed 200 OK in 5977ms (Views: 0.3ms | ActiveRecord: 3.0ms)

* GQL-cache POC * Don't cache batched fields * Cache lazy types * Wait for promises before saving to cache * Re-enable promise fields and to_s * Use hiredis * Uncomment timeout in schema * Rubocop fixes * Fix href/label test

* Fix transient profile test failure (#407) Signed-off-by: Andrew Kofink <[email protected]> * Hardcode latest RHEL supported versions (#408) * Hardcode latest RHEL supported versions * Add cache for latest supported versions * Freeze constant * Return an AR relation * Properly search for match ref_id/version * Task to import supported SSG benchmarks (#412) * Task to import supported SSG benchmarks * Regex way of getting RHEL number Co-Authored-By: Andrew Kofink <[email protected]> * Fix rubocop Co-authored-by: Andrew Kofink <[email protected]> * Introduce graphql-cache (#411) * GQL-cache POC * Don't cache batched fields * Cache lazy types * Wait for promises before saving to cache * Re-enable promise fields and to_s * Use hiredis * Uncomment timeout in schema * Rubocop fixes * Fix href/label test * Add benchmarks API endpoint (#409) Signed-off-by: Andrew Kofink <[email protected]> * Fixes for rake tasks (#414) * Require HostInventoryNotFound * Execute instead of invoking import SSG task * Add specific config for cache redis (#415) Co-authored-by: Andrew Kofink <[email protected]> Co-authored-by: Andrew Kofink <[email protected]>

This reverts commit 3854f85.

* Revert "Introduce graphql-cache (#411)" This reverts commit 3854f85. * Unify references JSON response

This reverts commit 3854f85.

* Revert "Introduce graphql-cache (RedHatInsights#411)" This reverts commit 3854f85. * Unify references JSON response

* Revert "Introduce graphql-cache (#411)" This reverts commit 3854f85. * Unify references JSON response

* GQL-cache POC * Don't cache batched fields * Cache lazy types * Wait for promises before saving to cache * Re-enable promise fields and to_s * Use hiredis * Uncomment timeout in schema * Rubocop fixes * Fix href/label test

dLobatog added 7 commits April 21, 2020 11:48

GQL-cache POC

034082d

Don't cache batched fields

87371bc

Cache lazy types

e533cb3

Wait for promises before saving to cache

f9a26e2

Re-enable promise fields and to_s

98a07be

Use hiredis

56e99b9

Uncomment timeout in schema

5d34353

dLobatog commented Apr 21, 2020

View reviewed changes

Rubocop fixes

3ecb973

akofink reviewed Apr 21, 2020

View reviewed changes

Fix href/label test

70f48be

dLobatog mentioned this pull request Apr 21, 2020

Heavy caching GQL #405

Closed

akofink approved these changes Apr 21, 2020

View reviewed changes

dLobatog merged commit 3854f85 into RedHatInsights:master Apr 22, 2020

dLobatog added a commit to dLobatog/compliance-backend that referenced this pull request Apr 22, 2020

Revert "Introduce graphql-cache (RedHatInsights#411)"

44d9090

This reverts commit 3854f85.

dLobatog added a commit that referenced this pull request Apr 22, 2020

Revert GQL-cache (#417)

4e6dadc

* Revert "Introduce graphql-cache (#411)" This reverts commit 3854f85. * Unify references JSON response

dLobatog added a commit to dLobatog/compliance-backend that referenced this pull request Apr 22, 2020

Revert "Introduce graphql-cache (RedHatInsights#411)"

1bab8da

This reverts commit 3854f85.

dLobatog added a commit to dLobatog/compliance-backend that referenced this pull request Apr 22, 2020

Revert GQL-cache (RedHatInsights#417)

32e9f48

* Revert "Introduce graphql-cache (RedHatInsights#411)" This reverts commit 3854f85. * Unify references JSON response

dLobatog added a commit that referenced this pull request Apr 22, 2020

Revert GQL-cache (#417) (#419)

56fc21d

* Revert "Introduce graphql-cache (#411)" This reverts commit 3854f85. * Unify references JSON response

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce graphql-cache #411

Introduce graphql-cache #411

dLobatog commented Apr 21, 2020

dLobatog Apr 21, 2020

dLobatog Apr 21, 2020

dLobatog Apr 21, 2020

dLobatog Apr 21, 2020

dLobatog Apr 21, 2020

akofink Apr 21, 2020

dLobatog Apr 21, 2020

dLobatog Apr 21, 2020

dLobatog Apr 21, 2020

dLobatog Apr 21, 2020

akofink left a comment

akofink Apr 21, 2020

akofink Apr 21, 2020

akofink left a comment

Introduce graphql-cache #411

Introduce graphql-cache #411

Conversation

dLobatog commented Apr 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akofink left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akofink left a comment

Choose a reason for hiding this comment