Fixes API design for Sidekiq middleware

[gstark]

Breaking API change: Adds NewrelicGvl::Sidekiq::ClientMiddleware and NewrelicGvl::Sidekiq::ServerMiddleware to allow for the instrumentation of client and server sidekiq middleware. The README has also been updated to note the new class names and usage.

Thanks to Navid EMAD for the issue/PR concerning the incorrect API.

[navidemad]

From the documentation it's says : By default, all metrics are disabled.
From a test example here: https://github.com/Shopify/gvltools/blob/master/test/gvl_tools/test_timer.rb
I think we have to call GVLTools::LocalTimer.enable somewhere otherwise it's always 0, but i'm not sure we have to do it inside the Middleware though. It would be great to have a test case where it compute a heavy fibonacci computation to really test if add_custom_attributes is receiving a positive gvl_wait value.

[navidemad]

I tried something like:

  def cpu_work
    fibonacci(35)
  end

  def fibonacci(number)
    number <= 1 ? number : fibonacci(number - 1) + fibonacci(number - 2)
  end
  
  it "Holds the GVL" do
    GVLTools::LocalTimer.enable

    middleware = NewrelicGvl::Sidekiq::ServerMiddleware.new

    NewRelic::Agent.expects(:add_custom_attributes).at_least_once.with do |attributes|
      attributes[:gvl_wait] > 0
    end

    middleware.call do
      10.times.map do
        Thread.new do
          cpu_work
        end
      end.each(&:join)
    end
  end

But it's seems to be flacky sometimes

[gstark]

Yes, I had a PR to the gvl tools repository and testing that code is challenging. Given that we wrap that library, mock the gvl tools, and set an expectation on the new relic gem, we may have enough testing for this.

I can see the need for calling GVLTools::LocalTimer.enable - would you like to submit a separate PR containing that change?

[navidemad]

I'm not sure if enabling it in the middleware is the right thing to do.
I think the middleware should look something like :

        if GVLTools::LocalTimer.enabled?
          begin
            before = GVLTools::LocalTimer.monotonic_time
            yield
          ensure
            gvl_holding_duration = GVLTools::LocalTimer.monotonic_time - before
            NewRelic::Agent.add_custom_attributes({gvl_wait: gvl_holding_duration})
          end
        else
          yield
        end

And do like a config singleton to store a variable called enabled = true/false