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

Hanami support #322

Closed
wants to merge 1 commit into from
Closed

Conversation

KlotzAndrew
Copy link

  • 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 tombruijn self-assigned this Jul 19, 2017
Copy link
Member

@tombruijn tombruijn left a comment

Choose a reason for hiding this comment

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

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.

if !controller.empty?
transaction.set_action_if_nil(controller)
else
transaction.set_action_if_nil("unknown")
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

removed

end

def hanami_action(env)
Web.routes.recognize(env["REQUEST_PATH"]).action.to_s
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

passing in env directly lets Hanami resolve it, updated

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))
Copy link
Member

@tombruijn tombruijn Jul 19, 2017

Choose a reason for hiding this comment

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

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

end

it "should set the error" do
expect_any_instance_of(Appsignal::Transaction).to receive(:set_error).with(error)
Copy link
Member

@tombruijn tombruijn Jul 19, 2017

Choose a reason for hiding this comment

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

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).

require "rack"

module Appsignal
# @api private
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

added

@KlotzAndrew
Copy link
Author

Hey! I can take a look later this week

@tombruijn
Copy link
Member

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
Copy link
Author

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
Copy link
Member

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 :)

@matsimitsu matsimitsu mentioned this pull request Jul 24, 2017
4 tasks
@tombruijn
Copy link
Member

@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 tombruijn removed their assignment Aug 7, 2017
@tombruijn tombruijn mentioned this pull request Jan 10, 2020
36 tasks
@tombruijn tombruijn changed the base branch from master to main July 8, 2020 08:44
@tombruijn
Copy link
Member

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.

@tombruijn tombruijn closed this Jan 28, 2021
end

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

Choose a reason for hiding this comment

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants