appsignal-ruby icon indicating copy to clipboard operation
appsignal-ruby copied to clipboard

Padrino integration creates two transactions?

Open tombruijn opened this issue 8 years ago • 4 comments

Which means that the events of the first one are ignored resulting in the missing "process_action.padrino" event.

This happens because Padrino calls route! in its own route! method again, this time with the base.superclass argument: https://github.com/padrino/padrino-framework/blob/4a80dca4ec0c52a9c7daa2e28d17c1d8cb024e4b/padrino-core/lib/padrino-core/application/routing.rb#L974

tombruijn avatar May 09 '17 09:05 tombruijn

That we don't track the perform_action.padrino event comes from a throw :halt Padrino uses. Changing the instrument method to use an ensure rather than a "soft" value capture would fix this.

Should be changed in Appsignal.instrument and Appsignal::Transaction#instrument, or just once in the proposed PR #288

class Foo
  def exits_too_early
    catch :halt do
      puts "instrument:"
      instrument "foo" do
        instrumented_method
      end
    end
  end

  def exits_fine
    catch :halt do
      puts "ensure_instrument:"
      ensure_instrument "foo" do
        instrumented_method
      end
    end
  end

  def instrumented_method
    puts "hello!"
    throw :halt
  end

  def instrument(name)
    puts "-- start #{name}"
    r = yield if block_given?
    puts "-- end #{name}"
    r
  end

  def ensure_instrument(name)
    puts "-- start #{name}"
    yield if block_given?
  ensure
    puts "-- end #{name}"
  end
end

Foo.new.exits_too_early
puts "\n"
Foo.new.exits_fine

# $ ruby foo.rb
# instrument:
# -- start foo
# hello!
#
# ensure_instrument:
# -- start foo
# hello!
# -- end foo

tombruijn avatar May 09 '17 14:05 tombruijn

Looks like the original problem: "2 transactions being started for a request to a Padrino app", only occurs when the route doesn't exist. When the route doesn't exist in Padrino, it gets asked to a Sinatra app (through the Padrino route! method for some reason).

tombruijn avatar May 10 '17 14:05 tombruijn

Should be fixed with the standardized middleware in #329

tombruijn avatar Aug 11 '17 08:08 tombruijn

https://app.intercom.com/a/apps/yzor8gyw/inbox/inbox/540709/conversations/27054497129

tombruijn avatar May 13 '20 11:05 tombruijn