Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Split out Instrumentation from Extractor #6

Merged
merged 4 commits into from
Nov 20, 2018

Conversation

chrisbutcher
Copy link
Contributor

@chrisbutcher chrisbutcher commented Oct 10, 2018

Introduces a new public method, GraphQLMetrics::Extractor#extract!, which takes in a GraphQL::Query and permits stateful extractors to be created that aren't used as instrumentation. See the test case below for an idea of its usage.

Only field_extracted, initialize and an attr_reader to pull out stored values is needed, but I implemented no-ops for the rest of the Extractor interface to ensure we detect breakage in those due to this or future changes.

lib/graphql_metrics/extractor.rb Show resolved Hide resolved
lib/graphql_metrics/extractor.rb Outdated Show resolved Hide resolved
@chrisbutcher chrisbutcher changed the title Introduce public method, extract_query_metrics Introduce public method, extract! Oct 10, 2018
@chrisbutcher
Copy link
Contributor Author

chrisbutcher commented Oct 16, 2018

Updated to extract a new boolean via variable_extracted called used_in_query which indicates if the variable was actually used in the selected operation.

Also introduces a running_as_instrumentation mode, to more cleanly branch between code paths that only make sense when running as instrumentation, vs. an ad hoc extractor.

@chrisbutcher
Copy link
Contributor Author

tldr: We / other clients can use this to conditionally log, or log differently unused variables.

@chrisbutcher
Copy link
Contributor Author

chrisbutcher commented Nov 12, 2018

(Note to self: Wrap the clock_gettime calls within the gem within something like GraphQLMetrics::TimedBatchExecutor.current_time, so that we can stub that, instead of the stdlib method, with Process.stubs(:clock_gettime))

edit: Done.

@chrisbutcher
Copy link
Contributor Author

@swalkinshaw @eapache At your earliest convenience, please see the latest 2 commits (perhaps start with the changes to README.md in the first commit).

This introduces a breaking change (the meaning of Extractor changed), so I'll be bumping the gem to 2.0 after this PR.

-class LoggingExtractor < GraphQLMetrics::Extractor
+class LoggingExtractor < GraphQLMetrics::Instrumentation

README.md Show resolved Hide resolved
lib/graphql_metrics/timed_batch_executor.rb Show resolved Hide resolved
lib/graphql_metrics/extractor.rb Outdated Show resolved Hide resolved
lib/graphql_metrics/extractor.rb Outdated Show resolved Hide resolved
@chrisbutcher chrisbutcher force-pushed the support-non-instrumentation-use-cases branch 2 times, most recently from f9adaee to 1f474b5 Compare November 19, 2018 18:25
@chrisbutcher
Copy link
Contributor Author

Thanks! Updated.

Copy link
Contributor

@swalkinshaw swalkinshaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome 🎉

@chrisbutcher chrisbutcher changed the title Introduce public method, extract! Split out Instrumentation from Extractor Nov 19, 2018
@chrisbutcher
Copy link
Contributor Author

@swalkinshaw Here's where I settled on the instrumentor/start/end time checks in extract_query.

start_time, end_time = if instrumentor
  start_time = instrumentor.ctx_namespace[Instrumentation::START_TIME_KEY]
  return unless start_time

  [start_time, Instrumentation.current_time]
end

return unless irep_node.definition

field_extracted(
resolver_times = if instrumentor
instrumentor.ctx_namespace.dig(Instrumentation::TIMING_CACHE_KEY, irep_node.ast_node)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it would be cleaner to move logic like this into a method on the instrumentor, and then define a dummy instrumentor class to use instead of nil?

@chrisbutcher chrisbutcher force-pushed the support-non-instrumentation-use-cases branch from 3cd68f3 to 405df32 Compare November 19, 2018 20:49
ns[START_TIME_KEY] = Process.clock_gettime(Process::CLOCK_MONOTONIC)
rescue StandardError => ex
handle_extraction_exception(ex)
def instrumentor
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is here since I'd rather default again at runtime if an extractor calls instrumentor, and the extractor overrode initialize but didn't call super.

Wrap Process.clock_gettime for easier stubbing

pr feedback

Update lib/graphql_metrics/extractor.rb

Co-Authored-By: chrisbutcher <chrisbutcher@users.noreply.github.com>

Refactor of start_time, end_time logic in extract_query

Rename used_in_query to more accurate used_in_operation
@chrisbutcher chrisbutcher force-pushed the support-non-instrumentation-use-cases branch from 66d4e26 to ddedc08 Compare November 19, 2018 22:41
@chrisbutcher chrisbutcher merged commit 4ca38a2 into master Nov 20, 2018
@chrisbutcher chrisbutcher deleted the support-non-instrumentation-use-cases branch November 20, 2018 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants