Standardize rack middleware

[tombruijn]

There are problems with mounting grape (and other) apps on Rails apps. This creates two transactions and flip-flops between the two integrations. To make sure only one transaction gets created standardize the rack middleware integration.

TODO

Rack middlewares to refactor

• [x] Create one middleware at the top of the stack, instrument other middleware #159
• [x] pure-Rack - EventHandler and GenericInstrumentation middleware
  • [x] Add GenericInstrumentation replacement InstrumentationMiddleware that is basically the AbstractMiddleware and doesn't set an action name, but people can use it (without the action name going to "unknown" by default) and they will have response body instrumentation that way.
• [x] Rails - #1089
  • [x] #1107
• [x] Sinatra - #1097
  • [x] Update auto-sinatra hook/integration to also use event handler - #1112
• [x] Hanami
  • [ ] Avoid having to monkeypatch - https://github.com/appsignal/appsignal-ruby/issues/911
  • [x] Apply the same fix as for Sinatra to not have a boot loop for nested app. - #1130
• [x] Grape
  • [x] Have appsignal/integrations/grape require appsignal - #1122
  • [x] ~Start AppSignal from the integrations file like for Sinatra and Hanami?~ Can create a boot loop...
  • [x] ~Apply the same fix as for Sinatra to not have a boot loop for nested app. #1105~
• [x] Streaming middleware
  • Deprecated in #1140
• [ ] Padrino
  • Still maintained?
  • [ ] Apply the same fix as for Sinatra to not have a boot loop for nested app. #1105
  • [ ] Auto add EventHandler and InstrumentationMiddleware?
• [x] ~Webmachine~ - #1141 (technically not middleware)

Features

• [x] Instrument response bodies: #1099
• [x] Support nested apps
  • [x] EventHandler - #1101
  • [x] AbstractMiddleware - #1100
• [ ] Add response status code to transaction metadata - #183
  • [x] Fix issue for errors not reporting any response status code. Should it return a 500? - #1125

Update docs

• [x] Mention the EventHandler for Rack - https://github.com/appsignal/appsignal-docs/pull/1947
• [x] Mention apps need to call Appsignal.set_error in Hanami apps to call handle_exception if they want to report the error to AppSignal. See Hanami exception handling docs. - https://github.com/appsignal/appsignal-docs/pull/1948
• [x] Update Grape docs: https://github.com/appsignal/appsignal-docs/pull/1949
• [x] Update Grape docs to not require appsignal manually - https://github.com/appsignal/appsignal-docs/pull/1950
• [x] Update Grape docs to mention including the EventHandler - https://github.com/appsignal/appsignal-docs/pull/1950
• [x] ~Mention how to instrument sinatra without the auto integrations file~ Now that it supported nested apps there's not a lot of reason to document this.
• [ ] Update Rack instrumentation page with a default middleware in combination with the EventHandler - see #1139 for more information

Could unblock

• [x] Avoid Rails middleware injection issue #155
  • Doesn't seem to fail on newer Rails versions anymore
• [x] Improve Grape validation error reporting #771
• [ ] Could fix #289 if we check the appsignal.transaction on the request env in padrino
• [ ] Instrument other middleware #159

[tombruijn]

Waiting for a Grape app where we can reproduce the original problem with

[matsimitsu]

We're going to take a day to dive into this issue and create a concrete plan for this, with examples of how the new API is going to change.

[jeffkreeftmeijer]

https://app.intercom.io/a/apps/yzor8gyw/inbox/inbox/540654/conversations/25701975802

[julik]

I've given this some thinking: the Rack-derived middleware (integrations) are very similar to one another, so in terms of code reuse they could be brought together or at least inherit from the same superclass (GenericInstrumentation I'd say). In terms of making this more robust, I see a few distinct use cases:

• Rails app which has a Rack or Sinatra app mounted as an engine in routes.rb - the sidekiqs, mission controls, admins of various kinds etc.
• Rails app which has a Rack or Sinatra app mounted at the root in config.ru - this does get done sometimes to ensure that there is 0 rails overhead on a particular URL
• Rack app which uses Rack builder in config.ru and builds a tree of URLs using map and run

In all of those cases it is possible that such a "matroska" (or "umblerella" or whatever) Rack app will actually have multiple instances of the Appsignal middleware active, within the same call to call. For example, a Sinatra app might have Sinatra integration configured, and Rails app might have the Rails integration configured and so on. So whichever nesting is applied, the first order of business would be to ensure that regardless of the number of these instrumentations which get mounted there is only 1 Appsignal transaction used for 1 unit of work (web request), and it gets "centrally" created and centrally completed, once.

This should be very easy to do, provided that 1 decision gets made. If there is nesting between apps, which app wins on setting the action, controller name, path and the params? Do we preserve what gets set in the innermost middleware or we allow the outermost middleware to overwrite instead?

I think all of the above could be achieved in a backwards-compatible way and will also avoid flipflopping. Wdyt @tombruijn ?

[tombruijn]

This should be very easy to do, provided that 1 decision gets made. If there is nesting between apps, which app wins on setting the action, controller name, path and the params? Do we preserve what gets set in the innermost middleware or we allow the outermost middleware to overwrite instead?

I'd say this would be "won" by the most nested app. That is the actual handler of the request. We already use helpers like set_action_if_nil in all our middlewares, that only set the action name if it wasn't set before. I think we can use the same strategy here.

For such nested apps the instrumentation it should not create a new transaction for the nested apps, but use the current transaction, created by the outer most instrumentation. Preferrably, it wouldn't log this warning, so some "check if there is a current AppSignal transaction already" might need to be done.

Some other thoughts I remember we had were:

In the Rails integration, we insert our middleware in a spot that we can rescue and report errors, but ideally we'd be one of the first, if not the first, middleware in the stack. That way we can report the performance of other middlewares too, and the request's status code. We might need to report errors in a different way (maybe the Rails error handler can be used for this?) or the middlware we have now would only report errors with this change.

[julik]

So far I was thinking we could then do the following:

• We set the transaction that got created into env["appsignal.transaction"]. If there are multiple instances of the Rack instrumentation mounted into the tree (be it Sinatra, vanilla Rack, whatever) they would reuse that transaction
• We do not create a new txn if there already is a env["appsignal.transaction"] and we do not apply the body wrapping. We do, however, set the metadata for the transaction inside ensure of call if the fields are empty
• Only the outermost middleware would apply a BodyWrapper

I don't quite understand the idea of "instrumenting Rack middleware". If there is one Rack middleware mounted at the outermost possible point, it would already monitor everything happening inside the call of all the things upstream from it, as well as the serving out of the body (via BodyWrapper). Middleware is not straightforward to trace just as a "stack" because every middleware may wrap the response body and return it up the chain, the processing of its body will be deferred. So if instrumenting particular pieces of middleware is desired, I wonder how that would look in, say, the waterfall view 🤔

There is one thing here which is not going to be very straightforward, and that is the handling of filtered params. As it stands now, the "outermost" middleware would produce the request that the txn gets created with. That request would then likely be a Rack request (if the user is not careful enough to mount the Rails integration specifically), and the Rack request does not have filtered_params. That would mean that depending on the order in which things get mounted/added to the stack, filtered params could inadvertently get revealed. I am almost certain a workaround here will be needed because otherwise this creates a security risk for users at the slightest misconfiguration. What we could do is provide some kind of "wrapper" which would only resolve filtered_params at the outermost level of the stack and if (and only if) there is an ActionDispatch request object somewhere (it would be findable in the Rack env).

For the moment what I'm imagining is that we can either

• Keep the disparate Rack-related middleware classes, just unify them a bit. Then they could have all their special options still, they would just abide by the "reuse the outermost transaction" rule
• Stuff all the functionality into one and the same Rack middleware, it would just scan the env in a more rigorous way and examine the env and the output headers, and then fill in with the first available option (i.e. scan in order for action name through Appsignal-specific stuff, then Sinatra-specific-stuff, then Rails-specific-stuff etc.)

Either option seems fine tbh, it's just that there will be more deprecations with the latter.

The rest seems trivial.

[julik]

Will pick this up once #1037 is in.

[tombruijn]

I don't quite understand the idea of "instrumenting Rack middleware". If there is one Rack middleware mounted at the outermost possible point, it would already monitor everything happening inside the call of all the things upstream from it, as well as the serving out of the body (via BodyWrapper). Middleware is not straightforward to trace just as a "stack" because every middleware may wrap the response body and return it up the chain, the processing of its body will be deferred. So if instrumenting particular pieces of middleware is desired, I wonder how that would look in, say, the waterfall view 🤔

This would be nice for later. We'd like to show each middleware in the timeline, but there's no central place to hook into for that, that I know off. It might require us to make some kind of thin middleware wrapper or some solution like that.

There is one thing here which is not going to be very straightforward, and that is the handling of filtered params. As it stands now, the "outermost" middleware would produce the request that the txn gets created with. That request would then likely be a Rack request (if the user is not careful enough to mount the Rails integration specifically), and the Rack request does not have filtered_params. That would mean that depending on the order in which things get mounted/added to the stack, filtered params could inadvertently get revealed. I am almost certain a workaround here will be needed because otherwise this creates a security risk for users at the slightest misconfiguration. What we could do is provide some kind of "wrapper" which would only resolve filtered_params at the outermost level of the stack and if (and only if) there is an ActionDispatch request object somewhere (it would be findable in the Rack env).

We could have the Rails instrumentation middleware set the params explicitly on the transaction with Appsignal::Transaction#params=. If that's set, it will not check the request object given to the transaction. Would that help?

[backlog-helper[bot]]

• This issue 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

[backlog-helper[bot]]

• This issue 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

[backlog-helper[bot]]

• This issue 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