refactor(graphql-hive.rb): clean up file, move config to class #47
Conversation
| return yield unless should_collect_usage? | ||
| return yield unless platform_key == "execute_multiplex" | ||
| return yield unless data[:multiplex] | ||
|
|
||
| queries = data[:multiplex].queries | ||
| return yield if queries.empty? | ||
|
|
||
| timestamp = generate_timestamp | ||
| results, duration = measure_execution { yield } | ||
|
|
||
| report_usage(timestamp, queries, results, duration) | ||
| results | ||
| end | ||
|
|
||
| def should_collect_usage? | ||
| @configuration.enabled && @configuration.collect_usage | ||
| end | ||
|
|
||
| def generate_timestamp | ||
| (Time.now.utc.to_f * 1000).to_i | ||
| end | ||
|
|
||
| def measure_execution | ||
| starting = Process.clock_gettime(Process::CLOCK_MONOTONIC) | ||
| results = yield | ||
| ending = Process.clock_gettime(Process::CLOCK_MONOTONIC) | ||
| duration = ((ending - starting) * (10**9)).to_i | ||
|
|
||
| [results, duration] |
There was a problem hiding this comment.
Refactor for legibility. No functional changes here.
cassiascheffer
force-pushed
the
refactor-config
branch
from
c6e64be to
7154962
Compare
cassiascheffer
force-pushed
the
refactor-config
branch
from
7154962 to
71a2a7f
Compare
cassiascheffer
force-pushed
the
refactor-config
branch
from
bcabdd8 to
116d813
Compare
| The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), | ||
| and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). |
There was a problem hiding this comment.
Nice, thank you for adding this file!
lib/graphql-hive.rb
Outdated
| at_exit do | ||
| GraphQL::Hive.instance&.on_exit | ||
| end |
There was a problem hiding this comment.
I think we'll still need this when the server exits / shuts down. Was this intentional? 🤔
There was a problem hiding this comment.
Ah, this was sort of intentional but the work isn't finished. Hooking in to at_exit could cause hive to exit before web workers are finished processing requests. The gem exposes on_exit so that Puma apps can call on_exit in the shutdown sequence. In a Puma app with multiple workers, there would be a race condition between at_exit and on_exit.
It makes more sense to me to delete these lines and rely on the on_exit hook.
There was a problem hiding this comment.
I can add these lines back in to keep this PR scoped to config changes and make a followup to delete this and update the readme to make life-cycle events more clear for single-worker environments.
There was a problem hiding this comment.
Ah gotcha! I assume if we remove this, all single worker apps would need to call on_exit method in the usage reporter manually, right?
There was a problem hiding this comment.
That's right. In a separate PR I'll update the docs to indicate that.
| if @report_schema && (@reporting.dig(:author) || !@reporting.dig(:commit)) | ||
| @logger.warn("GraphQL Hive `author` and `commit` options are required. Disabling Schema Reporting.") | ||
| @report_schema = false | ||
| end |
There was a problem hiding this comment.
Should @report_schema also be false if the the token isn't set?
There was a problem hiding this comment.
Good catch! Fixed in 478fdb2
There was a problem hiding this comment.
This made me realize the configs are a bit confusing. It is possible to have:
{
enabled: false
report_schema: true
token: `a-valid-token`
}
I'll put some thought into the configs for a future PR.
This is a refactor of graphql-hive.rb that moves configuration information to a
Configurationclass. Later, this will allow us to configure GraphQL Hive outside of a schema.rb file.