Hanami support #322

KlotzAndrew · 2017-07-19T02:45:58Z

mostly the same as the rack implementation, in the middleware
stack Hanami apps can now
use Appsignal::Rack::HanamiInstrumentation
Hanami router matches a path to a controller action, we
set that as the controller. Similar to the rails instrument

tombruijn

Great work! Thanks @KlotzAndrew!

We've been wanting Hanami integration for some time even before issue #125 was created, so it's great to see it's finally happening 😄

Great to see you've got this to work!
I've got some questions, suggestions and requested some changes. Let me know if you are able to work on this further and if we can help out with anything.

tombruijn · 2017-07-19T07:59:54Z

lib/appsignal/rack/hanami_instrumentation.rb

+          if !controller.empty?
+            transaction.set_action_if_nil(controller)
+          else
+            transaction.set_action_if_nil("unknown")


I don't know if we want to report this as an "unknown" action. If no action is set it will be automatically reported as "outside of an action" in AppSignal, which might be more consistent with other integrations. I'd rather only set the action if we know it.

tombruijn · 2017-07-19T08:01:03Z

lib/appsignal/rack/hanami_instrumentation.rb

+      end
+
+      def hanami_action(env)
+        Web.routes.recognize(env["REQUEST_PATH"]).action.to_s


My Hanami is a bit rusty, but is this a constant something which can be customized by a developer? In case of multiple apps can there be more constant names?

passing in env directly lets Hanami resolve it, updated

tombruijn · 2017-07-19T08:09:39Z

spec/lib/appsignal/rack/hanami_instrumentation_spec.rb

+        kind_of(String),
+        Appsignal::Transaction::HTTP_REQUEST,
+        kind_of(Rack::Request)
+      ).and_return(double(:set_action_if_nil => nil, :set_http_or_background_queue_start => nil, :set_metadata => nil))


no need to return anything here I think?

edit: ah I see, also from the Rack specs. Yeah, it's unnecessary there too I think

tombruijn · 2017-07-19T08:19:55Z

spec/lib/appsignal/rack/hanami_instrumentation_spec.rb

+      end
+
+      it "should set the error" do
+        expect_any_instance_of(Appsignal::Transaction).to receive(:set_error).with(error)


Looks like you might have copied this from our Rack setup. We're actually in the process of rewriting these tests (when we have some spare time, see issue #252 ) to remove the amount of mocking and stubbing we have in our test suite.

We've added a new way of testing transaction data which allows us to test the same without expect(transaction).to receive(:method) for every call. The idea being: It shouldn't really matter how the data is set, as long as the transaction contains the data.

You can see how I applied this is in our ActionCable integration, which was added quite recently in PR #309.

describe "some integration" do let(:env) { { :foo => :bar } } let(:transaction) do Appsignal::Transaction.new( SecureRandom.uuid, Appsignal::Transaction::HTTP_REQUEST, ActionDispatch::Request.new(env) ) end before do expect(Appsignal::Transaction).to receive(:create) .with(transaction_id, Appsignal::Transaction::HTTP_REQUEST, kind_of(ActionDispatch::Request)) .and_return(transaction) allow(Appsignal::Transaction).to receive(:current).and_return(transaction) # Make sure sample data is added expect(transaction.ext).to receive(:finish).and_return(true) # Stub complete call, stops it from being cleared in the extension # And allows us to call `#to_h` on it after it's been completed. expect(transaction.ext).to receive(:complete) end it "creates an AppSignal transaction" do # call code to perform action in integration expect(transaction.to_h).to include( "action" => "Hanami#action", "error" => nil, "id" => transaction.transaction_id, "namespace" => Appsignal::Transaction::HTTP_REQUEST, "metadata" => { "method" => "GET", "path" => "/blog" } ) # same for `transaction.to_h["events"]` is the integration creates any # and same for # - `transaction.to_h["sample_data"]` # - `transaction.to_h["error"]` end end

This lengthy comment is nothing specific to your PR, but we'd like to limit the amount of mocking and stubbing we do here. If we can test it through the full Ruby gem code and our extension like this we're a lot more sure everything works like it should.

It would be great if you can have a look at this new setup. Otherwise we'll be able to update it to the new setup, but since adding Hanami integration was unplanned I'm sure when exactly we'll get around to it so we'll put it on our backlog and discuss at the next backlog meeting (which is weekly).

tombruijn · 2017-07-19T08:25:14Z

lib/appsignal/rack/hanami_instrumentation.rb

+require "rack"
+
+module Appsignal
+  # @api private


can you move this comment to the HanamiInstrumentation class?
and can you add the usage as a comment as well?

# @example # class MyHanamiApp # # Add this middleware to your app # use Appsignal::Rack::HanamiInstrumentation # end

or something like that

KlotzAndrew · 2017-07-19T21:10:50Z

Hey! I can take a look later this week

tombruijn · 2017-07-20T07:28:36Z

Great! Ping me if you need any help :)

* mostly the same as the rack implementation, in the middleware stack Hanami apps can now `use Appsignal::Rack::HanamiInstrumentation` * Hanami router matches a path to a controller action, we set that as the controller. Similar to the rails instrument

KlotzAndrew · 2017-07-21T02:38:21Z

Updated the code, Web.routes.recognize(env) lets Hanami do all the heavy lifting of matching the request to the controller

The tests are copied from the generic instrumentation (most of the code is the same as well). Thanks for the suggestions, they look like improvements but I will leave any test style refactors up to the maintainers. So long as Hanami support makes it in :)

tombruijn · 2017-07-21T09:51:53Z

Great :) I'll bring it up at the next backlog meeting and see where we can schedule this in.
Thanks for your work so far :)

tombruijn · 2017-07-27T11:14:46Z

@KlotzAndrew small update from our end: We discussed this feature further after estimating how much longer it would take to complete this (see issue #125 for more info) and decided we're not immediately going to work on completing this PR.

We're in the middle of some other projects we'd like finish first before working on something new, to try and keep our focus and keep our Work In Progress pipeline small.

Hope you understand and that you can work with your forked gem in the mean time until we officially support this. We'll update you once we've complete the work needed to merge this PR. You can (probably, no promises) expect this in AppSignal for Ruby gem 2.4.

tombruijn · 2021-01-28T08:36:30Z

I'm closing this issue in favor of tracking progress in #125. We'll most likely first need to rewrite out middleware setup in #329. We will get back to this then.

phillipoertel · 2021-03-15T15:43:33Z

lib/appsignal/rack/hanami_instrumentation.rb

+      end
+
+      def hanami_action(env)
+        Web.routes.recognize(env).action.to_s


FYI this fails once you have more than one app, or the app isn't mounted at `/' ...

tombruijn self-assigned this Jul 19, 2017

tombruijn requested changes Jul 19, 2017

View reviewed changes

KlotzAndrew force-pushed the hanami_support branch from 5f93179 to 3b7e28b Compare July 21, 2017 01:53

hanami support

78ef125

* mostly the same as the rack implementation, in the middleware stack Hanami apps can now `use Appsignal::Rack::HanamiInstrumentation` * Hanami router matches a path to a controller action, we set that as the controller. Similar to the rails instrument

KlotzAndrew force-pushed the hanami_support branch from 3b7e28b to 78ef125 Compare July 21, 2017 02:22

matsimitsu mentioned this pull request Jul 24, 2017

Add Hanami support #125

Closed

4 tasks

tombruijn removed their assignment Aug 7, 2017

tombruijn added the feature label Aug 29, 2018

tombruijn mentioned this pull request Jan 10, 2020

Standardize rack middleware #329

Closed

36 tasks

tombruijn changed the base branch from master to main July 8, 2020 08:44

tombruijn closed this Jan 28, 2021

phillipoertel reviewed Mar 15, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hanami support #322

Hanami support #322

KlotzAndrew commented Jul 19, 2017

tombruijn left a comment

tombruijn Jul 19, 2017

KlotzAndrew Jul 21, 2017

tombruijn Jul 19, 2017

KlotzAndrew Jul 21, 2017

tombruijn Jul 19, 2017 •

edited

Loading

tombruijn Jul 19, 2017 •

edited

Loading

tombruijn Jul 19, 2017

KlotzAndrew Jul 21, 2017

KlotzAndrew commented Jul 19, 2017

tombruijn commented Jul 20, 2017

KlotzAndrew commented Jul 21, 2017

tombruijn commented Jul 21, 2017

tombruijn commented Jul 27, 2017

tombruijn commented Jan 28, 2021

phillipoertel Mar 15, 2021

Hanami support #322

Hanami support #322

Conversation

KlotzAndrew commented Jul 19, 2017

tombruijn left a comment

Choose a reason for hiding this comment

tombruijn Jul 19, 2017

Choose a reason for hiding this comment

KlotzAndrew Jul 21, 2017

Choose a reason for hiding this comment

tombruijn Jul 19, 2017

Choose a reason for hiding this comment

KlotzAndrew Jul 21, 2017

Choose a reason for hiding this comment

tombruijn Jul 19, 2017 • edited Loading

Choose a reason for hiding this comment

tombruijn Jul 19, 2017 • edited Loading

Choose a reason for hiding this comment

tombruijn Jul 19, 2017

Choose a reason for hiding this comment

KlotzAndrew Jul 21, 2017

Choose a reason for hiding this comment

KlotzAndrew commented Jul 19, 2017

tombruijn commented Jul 20, 2017

KlotzAndrew commented Jul 21, 2017

tombruijn commented Jul 21, 2017

tombruijn commented Jul 27, 2017

tombruijn commented Jan 28, 2021

phillipoertel Mar 15, 2021

Choose a reason for hiding this comment

tombruijn Jul 19, 2017 •

edited

Loading

tombruijn Jul 19, 2017 •

edited

Loading