Add new Rack instrumentation middleware #1139
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The AbstractMiddleware requires the rack library. The other middlewares that inherit from it don't have to require it as well.
Add a test that makes sure the action name isn't always set to `unknown`.
Let's no longer the GenericInstrumentation middleware with its default action name set to `unknown`. This new InstrumentationMiddleware will not report any action name, only report an instrumentation event. The action name can be set by the app using the `Appsignal.set_action` helper from now on in this middleware. This will require us to update the Rack docs page to replace GenericInstrumentation with InstrumentationMiddleware.
Deprecate the Appsignal::Rack::GenericInstrumentation in favor of the Appsignal::Rack::InstrumentationMiddleware. This other middleware doesn't set the action name to "unknown" by default. The GenericInstrumentation is no longer loaded automatically. This helps with printing a deprecation warning when it is used. Update the Grape specs which I copied this from to not mention the Minutely constant, which I copied it from there.
tombruijn
force-pushed
the
rack-instrumentation-middleware
branch
from
dd66212
to
2ad6fc1
Compare
This was referenced Jul 2, 2024
luismiramirez
approved these changes
Jul 3, 2024
unflxw
approved these changes
Jul 3, 2024
unflxw
reviewed
Jul 3, 2024
Explain a little bit the differences between middlewares and link to the docs.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Remove duplicate rack requires
The AbstractMiddleware requires the rack library. The other middlewares that inherit from it don't have to require it as well.
Add test for GenericInstrumentation action name
Add a test that makes sure the action name isn't always set to
unknown.Add new Rack instrumentation middleware
Let's no longer the GenericInstrumentation middleware with its default action name set to
unknown.This new InstrumentationMiddleware will not report any action name, only report an instrumentation event.
The action name can be set by the app using the
Appsignal.set_actionhelper from now on in this middleware.This will require us to update the Rack docs page to replace GenericInstrumentation with InstrumentationMiddleware.
Deprecate our GenericInstrumentation middleware
Deprecate the Appsignal::Rack::GenericInstrumentation in favor of the Appsignal::Rack::InstrumentationMiddleware. This other middleware doesn't set the action name to "unknown" by default.
The GenericInstrumentation is no longer loaded automatically. This helps with printing a deprecation warning when it is used.
Update the Grape specs which I copied this from to not mention the Minutely constant, which I copied it from there.