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

Consolidate instrument(:query | :multiplex, ...) into trace modules, deprecate instrument #4771

Merged
merged 10 commits into from
Jan 19, 2024

Conversation

rmosolgo
Copy link
Owner

@rmosolgo rmosolgo commented Jan 9, 2024

This is an old instrumentation API which is redundant with trace modules. But it has been used a long time, so it's out there.

Removing this from the execution code removes 9 (🎉 🙄 ) objects per query

  # profile_small_query
- Total allocated: 118064 bytes (1167 objects)
+ Total allocated: 117288 bytes (1156 objects)

  # profile_large_query
- Total allocated: 10161440 bytes (74457 objects)
+ Total allocated: 10160664 bytes (74446 objects)

TODO:

  • Move the implementation to a trace module
  • Update documentation and internal usages
  • Add a deprecation warning (or don't, since it works fine?)
  • Add docs about handling errors in trace methods I think these migration notes will do the job.
  • Migrate and release graphql-pro / graphql-enterprise
  • Update graphql-batch Use trace_with to start and end batches Shopify/graphql-batch#164

Migration notes

  • In general, before_query and after_query should be done in def execute_multiplex. That gives you better control because multiplex.queries haven't actually started execution yet (so you can still change their initial parameters) and you can manually handle errors using rescue.

  • Instrumentation had the following error handling rule:

    Any instrumenter whose before_* hook ran without an error would also have its after_* hook run.

    You can implement that using rescue in Ruby:

    def execute_query(query:)
      do_something_instrumenty
      result = nil 
      begin 
        result = super # maybe this will raise an error 
      ensure 
         do_something_after_execution 
      end 
      result 
    end 

    In that way, an error in do_something_instrumenty would prevent do_something_after_execution from being called, but an error further down the stack (during super) would not prevent do_something_after_execution from being called.

  • The old before_/after_ hooks can be implemented on top of a trace module, see:

    You could copy that into your app, or better yet, open an issue on this repo and let's see how trace_with could handle it.

@rmosolgo rmosolgo mentioned this pull request Jan 15, 2024
20 tasks
@rmosolgo rmosolgo changed the title Consolidate instrument(:query | :multiplex, ...) into trace modules Consolidate instrument(:query | :multiplex, ...) into trace modules, deprecate instrument Jan 16, 2024
@rmosolgo rmosolgo added this to the 2.2.6 milestone Jan 19, 2024
@rmosolgo rmosolgo merged commit 85e7117 into master Jan 19, 2024
12 checks passed
@rmosolgo rmosolgo deleted the consolidate-instrumentation branch January 19, 2024 15:54
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.

1 participant