Unify rack middleware integrations

[julik]

For the moment this is a sketch, @tombruijn would like to hear your thoughts. The approach goes like this:

• There is just one Appsignal transaction across the middleware stack. Spans will be honored if nested integrations are used, but complete_current! will be done just once
• Action / method / etc. should be set "inside-out" - we should ensure that the innermost middleware wins when setting those, this is not covered yet but is the intention
• If filtered_params were made available on either request or in Rack env with ActionDispatch, use them. This to prevent inadvertently leaking unfiltered params via Rack::Request or similar

There are a few bits and bobs for eg request_id but we can address those once we are good with the basic approach. TBH in tests I would like to use actual rails/sinatra/rack_test if possible since I found the setup with this amount of mocks fairly brittle - and testing actual Rails controllers is not as complicated (see https://github.com/julik/zip_tricks/blob/next/spec/zip_tricks/rails_streaming_spec.rb#L27)

The resulting "Übermiddleware" has a few indirections but it does seem worth it to be honest.

Closes #329

[tombruijn]

I hope we can move these kinds of checks to the middleware of library they're for. That way the instrumentation of each library is together with all the other instrumentation for that library and easier to find back.

See also my comment on: #329 (comment)

[julik]

Yeah this is a side-effect of trying to use the filtered_params no-matter-what. If request may be re-assigned this is moot (and too clever).

[tombruijn]

There are a few bits and bobs for eg request_id but we can address those once we are good with the basic approach. TBH in tests I would like to use actual rails/sinatra/rack_test if possible since I found the setup with this amount of mocks fairly brittle - and testing actual Rails controllers is not as complicated (see https://github.com/julik/zip_tricks/blob/next/spec/zip_tricks/rails_streaming_spec.rb#L27)

There's a history of too much mocking and stubbing in the test suite. From using doubles everywhere for objects from libraries and asserting method calls on gem internals rather than testing the actual results. If possible, I'd like to rid of that as much as possible and have the tests be close as the real world as possible.

For the AppSignal elements, there's an old ongoing issue about it. Every time I update a spec, I rewrite it to the new style, using the Appsignal::Transaction#to_h method. This requires some setup with start_agent, keep_transactions, and calling created_transaction and last_transaction.to_h. Other examples in the test suite should be easy to find by searching for those method names or having a close look at our testing spec helpers and test env specific moneypatches

[tombruijn]

I'm not quite sure what this is doing now, how this would be used, or how it sets errors.

[julik]

It is in progress - like I said, it's a sketch for now (because so many things need to get touched)

[tombruijn]

It's not yet clear to me how these classes would work together. For Rails, I would expect us to add two middlewares to the Rails middleware stack: one at the very front of the stack to report the whole request duration and one to report the action name, errors and other library specific metadata.

For other libraries I assume there would only be one middleware we would add.

If any are nested middlewares they reuse the existing AppSignal transacction to report their data on.

If it's a lot of work to make it work with the request object on the transaction, I wonder if we could make the transaction less dependent (or not dependent at all) on the request object and instead have the middlewares set that meta/sample data directly on the request from the middlewares.

[julik]

It's not yet clear to me how these classes would work together. For Rails, I would expect us to add two middlewares to the Rails middleware stack: one at the very front of the stack to report the whole request duration and one to report the action name, errors and other library specific metadata.

This is what I am considering as well. What I do suspect is that for all cases except of "just auto-attaching things using the Railtie" people would already have integrations (middleware) configured manually, and it should keep working. "Sandwiching" two Appsignal middleware layers inside of an existing middleware stack is what this PR is about - the reason why it has inheritance and all that is that if you do not (and only "supplement" the already open transaction with data from a particular integration, say) you put the onus on the user to "also not forget to" install the "outer" middleware - which creates the transaction and makes sure it gets completed/sent off. There is also a lot of potential for nesting, some of it can be quite elaborate - I'm not saying everybody does that, but it is quite neat when necessary. I.e.

`config.ru - `use WhateverMiddleware; run Raills.application`
   |__>  `middleware.insert(WhateverMiddleware)
       |___> mount SomeRackApp, path: "/subdir"
           |___>  use_middleware(WhateverMiddlewareForSomeRackApp)
                 |___> run Rack::File.new...

So saying "there must be just 2 middleware layers, and one of them must be somewhere on the outside, and and and..." seems a bit too strict. Hence why I think allowing both "app nesting" and just having one integration is justified. But for this every integration needs to be able to both create a transaction and set the transaction properties (the supplement_...). Since it is not hard to do - that's what I went for.

If it's a lot of work to make it work with the request object on the transaction, I wonder if we could make the transaction less dependent (or not dependent at all) on the request object and instead have the middlewares set that meta/sample data directly on the request from the middlewares.

For filtered_params and all - yes, that would be preferrable.

[tombruijn]

This is what I am considering as well. What I do suspect is that for all cases except of "just auto-attaching things using the Railtie" people would already have integrations (middleware) configured manually, and it should keep working.

We could auto add it to the very front of the middleware stack by default. And if people have their own middlewares added we can provide instructions on how to remove and then readd AppSignal to the right position.

Something like:

Rails.config.application.middlewares.delete Appsignal::Rack::TopRackInstrumentation
Rails.config.application.middleware.unshift Appsignal::Rack::TopRackInstrumentation

Hence why I think allowing both "app nesting" and just having one integration is justified.

I agree. But if the middlewares can handle existing transactions that should not be a major issue, no? I think closing the transaction in the right place would be the most difficult.

I'll be away for a bit after today so someone else from my team will need to take over.

[backlog-helper]

• This Pull Request has not had any activity in 14 days. Please provide a status update if it is still relevant. Closed it if it is no longer relevant. Or move it to another column if it's blocked or requires another look at it. - (More info)

---

This is a message from the daily scheduled checks.

New issue guide | Backlog management | Rules | Feedback

[tombruijn]

I'm looking into refactoring the rack middleware like this. Took the first steps in #1089 and #1097. I will try to apply the ideas from this PR in a new PR based on the new way the middlewares work. Still some things to figure out. I will pick this up as part of #329.