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

Add a custom wrapper around #perform #160

Merged
merged 1 commit into from
May 16, 2023

Conversation

toneymathews
Copy link
Contributor

@toneymathews toneymathews commented May 16, 2023

TL;DR This PR adds a wrapper method around the #perform method. This allows us to wrap the #perform method within code blocks that would help with measuring the performance in terms of time taken or the number of sql queries executed or anything else

Today, we have to override the #resolve method as demonstrated by the example below.

class BaseLoader < GraphQL::Batch::Loader
    def resolve
      return if resolved?
      load_keys = queue
      @queue = nil

      instrument_sql_requests do
        perform(load_keys)
      end

      check_for_broken_promises(load_keys)
    rescue => err
      reject_pending_promises(load_keys, err)
    end

    def instrument_sql_requests
      SQLCounter.run { yield }
    end
end

With the proposed changes, the #resolve method does not need to be overridden and a system can call multiple wrappers if required.

class BaseLoader < GraphQL::Batch::Loader
  def custom_wrapper
    instrument_sql_queries do
      run_tracer do
        yield
      end
    end
  end

  def instrument_sql_queries
    SQLCounter.run { super }
  end

  def run_tracer
    CustomTracer.run { super }
  end
end

How did this come up?

Today we override the #resolve method. But we were looking to add tracing as well. While the tracing can be added within the #resolve method, it'd be nicer to add a shared implementation for this gem within Opentelemetry (tracking issue).

This is a non breaking change and should not cause any issues. This PR also releases v0.5.3

@swalkinshaw
Copy link
Contributor

Code looks fine but I'm not loving the name. Couple ideas:

  • around_perform - matches up with Rails callback conventions
  • custom_perform
  • instrument_perform - if this is the main use case, we could name it more explicitly?

@toneymathews
Copy link
Contributor Author

Thanks!, I've renamed to around_perform.

@toneymathews toneymathews merged commit 3b22366 into master May 16, 2023
26 checks passed
@shopify-shipit shopify-shipit bot temporarily deployed to rubygems May 16, 2023 22:49 Inactive
@swalkinshaw swalkinshaw deleted the add-wrapper-around-perform branch June 19, 2023 18:49
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