Standardize rack middleware #329
Comments
|
Waiting for a Grape app where we can reproduce the original problem with |
|
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. |
|
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 (
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 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 ? |
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 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. |
|
So far I was thinking we could then do the following:
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 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 For the moment what I'm imagining is that we can either
Either option seems fine tbh, it's just that there will be more deprecations with the latter. The rest seems trivial. |
|
Will pick this up once #1037 is in. |
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.
We could have the Rails instrumentation middleware set the params explicitly on the transaction with |
This comment has been minimized.
This comment has been minimized.
Sinatra apps, mounted in Rails app, would run into the issue that the Rails middleware has already created a transaction for the request. It would "force" a new transaction to be made, which loses information from everything that happened before it. Previously, before PR #1089, it would also leave a transaction that was not closed properly. Even with that change, for one request, now two transactions are created, one for Rails and one for the nested Sinatra app. This change reads if there's a current transaction from the request env, and uses that instead of creating a new one. Some logic in the Transaction class would read from the request object given to it on `Transaction.create` to set metadata like parameters, so these need to be set manually now. It will also make sure not to close the transaction if one existed already before this middleware was called. Part of #329, the Rack middleware refactor.
Sinatra apps, mounted in Rails app, would run into the issue that the Rails middleware has already created a transaction for the request. It would "force" a new transaction to be made, which loses information from everything that happened before it. Previously, before PR #1089, it would also leave a transaction that was not closed properly. Even with that change, for one request, now two transactions are created, one for Rails and one for the nested Sinatra app. This change reads if there's a current transaction from the request env, and uses that instead of creating a new one. Some logic in the Transaction class would read from the request object given to it on `Transaction.create` to set metadata like parameters, so these need to be set manually now. It will also make sure not to close the transaction if one existed already before this middleware was called. Part of #329, the Rack middleware refactor.
Sinatra apps, mounted in Rails app, would run into the issue that the Rails middleware has already created a transaction for the request. It could "force" a new transaction to be made, which loses information from everything that happened before it. Previously, before PR #1089, it would also leave a transaction that was not closed properly. Even with that change, for one request, now two transactions are created, one for Rails and one for the nested Sinatra app. This change reads if there's a current transaction from the request env, and uses that instead of creating a new one. Some logic in the Transaction class would read from the request object given to it on `Transaction.create` to set metadata like parameters, so these need to be set manually now. It will also make sure not to close the transaction if one existed already before this middleware was called. Part of #329, the Rack middleware refactor.
Add the EventHandler to the default Sinatra instrumentation. This will have it report more of the request runtime. It will also report the `response_status` tag and metric, as reported by the EventHandler. This is compatible with other instrumentations using the EventHandler or an AbstractMiddleware subclass. Part of #329
Add the EventHandler to the default Sinatra instrumentation. This will have it report more of the request runtime. It will also report the `response_status` tag and metric, as reported by the EventHandler. This is compatible with other instrumentations using the EventHandler or an AbstractMiddleware subclass. Part of #329
As part of #329, update the Hanami integration to use Rack middleware and the EventHandler to instrument requests made to Hanami apps. This standardizes the instrumentation as much as possible between Rack apps and minimizes our reliance on monkeypatches. The only monkeypatch that remains is setting the action name to the Action class name. I have found no other way yet to fetch this metadata from the request metadata, environment or the Hanami router. Part of #329 Mostly solves #911
As part of #329, update the Hanami integration to use Rack middleware and the EventHandler to instrument requests made to Hanami apps. This standardizes the instrumentation as much as possible between Rack apps and minimizes our reliance on monkeypatches. The only monkeypatch that remains is setting the action name to the Action class name. I have found no other way yet to fetch this metadata from the request metadata, environment or the Hanami router. Part of #329 Mostly solves #911
As part of #329, update the Hanami integration to use Rack middleware and the EventHandler to instrument requests made to Hanami apps. This standardizes the instrumentation as much as possible between Rack apps and minimizes our reliance on monkeypatches. The only monkeypatch that remains is setting the action name to the Action class name. I have found no other way yet to fetch this metadata from the request metadata, environment or the Hanami router. Part of #329 Mostly solves #911
As part of #329, update the Hanami integration to use Rack middleware and the EventHandler to instrument requests made to Hanami apps. This standardizes the instrumentation as much as possible between Rack apps and minimizes our reliance on monkeypatches. The only monkeypatch that remains is setting the action name to the Action class name. I have found no other way yet to fetch this metadata from the request metadata, environment or the Hanami router. Part of #329 Mostly solves #911
As part of #329, update the Hanami integration to use Rack middleware and the EventHandler to instrument requests made to Hanami apps. This standardizes the instrumentation as much as possible between Rack apps and minimizes our reliance on monkeypatches. The only monkeypatch that remains is setting the action name to the Action class name. I have found no other way yet to fetch this metadata from the request metadata, environment or the Hanami router. Part of #329 Mostly solves #911
As part of #329, update the Hanami integration to use Rack middleware and the EventHandler to instrument requests made to Hanami apps. This standardizes the instrumentation as much as possible between Rack apps and minimizes our reliance on monkeypatches. The only monkeypatch that remains is setting the action name to the Action class name. I have found no other way yet to fetch this metadata from the request metadata, environment or the Hanami router. Part of #329 Mostly solves #911
When AppSignal is already active, do not start AppSignal again. This is a good precaution as it prevents boot loops, when AppSignal is started twice with different configurations. This will improve support for nested Hanami applications, when they're mounted in another frameworks we support, like Rails or Sinatra. This is similar to PR #1105 Part of #329
When AppSignal is already active, do not start AppSignal again. This is a good precaution as it prevents boot loops, when AppSignal is started twice with different configurations. This will improve support for nested Hanami applications, when they're mounted in another frameworks we support, like Rails or Sinatra. This is similar to PR #1105 Part of #329
When AppSignal is already active, do not start AppSignal again. This is a good precaution as it prevents boot loops, when AppSignal is started twice with different configurations. This will improve support for nested Hanami applications, when they're mounted in another frameworks we support, like Rails or Sinatra. This is similar to PR #1105 Part of #329
Apply the same logic as we have in the AbstractMiddleware for webmachine apps. When a parent transaction is active, use that and don't create a new one. More importantly, don't close the active transaction. The webmachine instrumentation can't use the AbstractMiddleware, or any middleware, as this discouraged by the webmachine project. In practice I don't see this happening, but if someone happens to add an AppSignal EventHandler to the middleware stack in `config.ru`, we now support it. Part of #329
Apply the same logic as we have in the AbstractMiddleware for webmachine apps. When a parent transaction is active, use that and don't create a new one. More importantly, don't close the active transaction. The webmachine instrumentation can't use the AbstractMiddleware, or any middleware, as this discouraged by the webmachine project. In practice I don't see this happening, but if someone happens to add an AppSignal EventHandler to the middleware stack in `config.ru`, we now support it. Part of #329
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
InstrumentationMiddlewarethat 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.appsignal/integrations/graperequireappsignal- Require appsignal in Grape integration file #1122Start AppSignal from the integrations file like for Sinatra and Hanami?Can create a boot loop...Apply the same fix as for Sinatra to not have a boot loop for nested app. Fix Sinatra instrumentation causing boot loop #1105Webmachine- Support nested webmachine apps #1141 (technically not middleware)Features
Update docs
Appsignal.set_errorin Hanami apps to callhandle_exceptionif they want to report the error to AppSignal. See Hanami exception handling docs. - https://github.com/appsignal/appsignal-docs/pull/1948appsignalmanually - https://github.com/appsignal/appsignal-docs/pull/1950Mention how to instrument sinatra without the auto integrations fileNow that it supported nested apps there's not a lot of reason to document this.Could unblock
appsignal.transactionon the request env in padrinoInstrument other middleware Instrument Rack middleware #159The text was updated successfully, but these errors were encountered: