Skip to content

Support nested Grape apps #1118

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 27, 2024
Merged

Support nested Grape apps #1118

merged 1 commit into from
Jun 27, 2024

Conversation

tombruijn
Copy link
Member

@tombruijn tombruijn commented Jun 26, 2024

⚠️ Based on rebased PRs #1107 and #1117, as they both contain changes needed for this change. Only the last commit in this PR (as of writing) contains the reviewable change. Please also review PRs #1107 and #1117 so I can rebase this branch only to contain the relevant changes.

Support nested Grape apps

Refactor the Grape middleware to inherit from the AbstractMiddleware class. I've removed the inheritance on the Grape::Middleware::Base. We don't rely on the base Grape middleware.

By using an AbstractMiddleware subclass for Grape, we support Grape apps nested in Sinatra, Rails, and other apps already using AppSignal instrumentation middleware.

To keep the grape.skip_appsignal_error request env logic to work, I've made the report_errors option accept a proc and call it if it's callable to determine per request if the error should be reported.

Part of #329

Refactor the Grape middleware to inherit from the AbstractMiddleware
class. I've removed the inheritance on the Grape::Middleware::Base. We
don't rely on the base Grape middleware.

By using an AbstractMiddleware subclass for Grape we support Grape apps
nested in Sinatra, Rails and other apps already using AppSignal
instrumentation middleware.

To keep the `grape.skip_appsignal_error` request env logic to work, I've
made the `report_errors` option accept a proc and call it if it's
callable to determine per request if the error should be reported.
@tombruijn tombruijn force-pushed the grape-abstract-middleware branch from 93a2974 to 718cdc1 Compare June 27, 2024 08:29
@tombruijn tombruijn merged commit 66bb7a6 into main Jun 27, 2024
16 checks passed
@tombruijn tombruijn deleted the grape-abstract-middleware branch July 10, 2024 18:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants