Use AbstractMiddleware for Rails middleware #1107
Conversation
tombruijn
force-pushed
the
rails-middleware-refactor
branch
from
8fa872c
to
82e557c
Compare
tombruijn
force-pushed
the
rails-middleware-refactor
branch
from
82e557c
to
0a6073a
Compare
tombruijn
force-pushed
the
rails-middleware-refactor
branch
2 times, most recently
from
9bc8bc3
to
b7cdd26
Compare
This comment has been minimized.
This comment has been minimized.
It comes from the RailsInstrumentation middleware, which is the point we previously started instrumentating requests. Every subclass of AbstractMiddleware adds its own instrumentation event. Not sure if it's the best name 🤷♂️ |
|
@unflxw do you think it would be better to omit this instrumentation event? On a timeline like below it might be difficult to understand the differences between the action dispatch and rails events.
|
Define what is a private API on the classes themselves, and not the parent module for Rack middlewares.
We've seen in the Rails instrumentation that the `ActionDispatch::Request` class can raise an error on an invalid request method type. Guard against it and don't try to set a value on error.
Make sure the AbstractMiddleware allows apps to set custom params, and that they're not overwritten by the middleware.
Use our new AbstractMiddleware as a basis for the Rails instrumentation middleware. This moves all responsibility for handling nested instrumentation middleware to the AbstractMiddleware. The Rails middleware can now focus on only the part it needs to do: the action name and some additional metadata. The RailsInstrumentation middleware previously didn't create an event, but the AbstractMiddleware does do that now. It will report a new `middleware.rails` event now. I didn't add a changeset for this change as it's mostly an internal refactor.
Updating the RailsInstrumentation middleware to inherit from the AbstractMiddleware broke the error reporting for the middleware. It relied on the EventHandler to report errors, but before it gets there, Rails's exception handling middleware rescues the error and shows an error page instead. Add a config option to the AbstractMiddleware to configure if it should report errors or not. Enable it for the RailsInstrumentation to restore the error reporting functionality.
tombruijn
force-pushed
the
rails-middleware-refactor
branch
from
b7cdd26
to
0cba389
Compare
|
Turns out I broke error reporting in this PR for Rails apps. I've restored that functionality in the last commit. |
This comment has been minimized.
This comment has been minimized.
|
@unflxw I thought I'd ask you for another review since your last review. Only the last commit has changed since you last reviewed. |
|
@tombruijn If it doesn't represent anything meaningful for the customer's application, I think we shouldn't show it in the event timeline. I think customers will see it and assume it represents an "interesting" middleware in their Rails application (like |
This is a message from the daily scheduled checks. |
|
@unflxw I can make the abstract middleware not create an event for the Rails middleware. Let's do that in another PR, this one has been open long enough. |
As discussed in PR #1107, do not create a transaction event for a Rails apps using the RailsInstrumentation middleware. The ActiveSupport Notifications and Rack EventHandler events are enough. The `middleware.rails` event didn't add much. Closes #1120 [skip changeset] because the related change in PR #1107 has not been released yet.
As discussed in PR #1107, do not create a transaction event for a Rails apps using the RailsInstrumentation middleware. The ActiveSupport Notifications and Rack EventHandler events are enough. The `middleware.rails` event didn't add much. Closes #1120 [skip changeset] because the related change in PR #1107 has not been released yet.
Part of #329
Fix private API declarations
Define what is a private API on the classes themselves, and not the parent module for Rack middlewares.
Guard against request method throwing an error
We've seen in the Rails instrumentation that the
ActionDispatch::Requestclass can raise an error on an invalid request method type. Guard against it and don't try to set a value on error.Add a test for setting custom params
Make sure the AbstractMiddleware allows apps to set custom params, and that they're not overwritten by the middleware.
Use AbstractMiddleware for Rails middleware
Use our new AbstractMiddleware as a basis for the Rails instrumentation middleware. This moves all responsibility for handling nested instrumentation middleware to the AbstractMiddleware.
The Rails middleware can now focus on only the part it needs to do: the action name and some additional metadata.
The RailsInstrumentation middleware previously didn't create an event, but the AbstractMiddleware does do that now. It will report a new
middleware.railsevent now.I didn't add a changeset for this change as it's mostly an internal refactor.
Fix error reporting for Rails instrumentation
Updating the RailsInstrumentation middleware to inherit from the AbstractMiddleware broke the error reporting for the middleware. It relied on the EventHandler to report errors, but before it gets there, Rails's exception handling middleware rescues the error and shows an error page instead.
Add a config option to the AbstractMiddleware to configure if it should report errors or not. Enable it for the RailsInstrumentation to restore the error reporting functionality.