Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Not reporting request params (for Sinatra?) #1108

Closed
tombruijn opened this issue Jun 21, 2024 · 0 comments · Fixed by #1109
Closed

Not reporting request params (for Sinatra?) #1108

tombruijn opened this issue Jun 21, 2024 · 0 comments · Fixed by #1109
Assignees
@tombruijn
Labels
bug

Comments

@tombruijn
Copy link
Member

tombruijn commented Jun 21, 2024 •
edited
Loading

Describe the bug

We're not reporting request parameters for (at least) Sinatra apps since Ruby gem 3.9.0.

Probably related to #1100

Confirmed broken. This is because we now set params at the beginning of the request handling, instead of in the ensure block. If set beforehand, something happens with the parsing (possibly because POST payload processing happens in some middleware), and memoizes the request params as an empty Hash.

Solution: set if afterwards, which means a little rethink of how we set parameters.
@tombruijn tombruijn added the bug label Jun 21, 2024
@tombruijn tombruijn mentioned this issue Jun 21, 2024
Merged
tombruijn added a commit that referenced this issue Jun 21, 2024
@tombruijn
Add Transaction set_params(_if_nil) methods
9a20bf6 
Add methods to set the parameters on the Transaction. This replaces and
deprecates the Transaction `params=` writer method that was used before
this.

We need the `_if_nil` variant for the AbstractMiddleware to only write
the parameters if they're not already set by the app it's instrumenting.
Currently it tries to set the parameters before the app handles the
request, but this is unreliable and doesn't always report the parameters
of the request.

The `_if_nil` variant isn't perfect, because it doesn't consider
whatever parameters could be fetched from the request object, because it
now skips this. I can improve this later if necessary.

Part of #1108
tombruijn added a commit that referenced this issue Jun 21, 2024
@tombruijn
Fix parameter reporting for Rack and Sinatra apps
01710af 
The AbstractMiddleware tried to set the request parameters before the
app handled the request. This could cause the request object to end up
in a weird state where, if some other middleware (like a JSON payload
parsing middleware) hadn't parsed the request payload yet, the
parameters were reported as empty.

Move the parameter setting to the "after" hook so that it doesn't try to
read the parameters before they're parsed. Use the new
`Transaction#set_params_if_nil` so that it doesn't overwrite any custom
parameters set by the app.

Fixes #1108
tombruijn added a commit that referenced this issue Jun 21, 2024
@tombruijn
Fix parameter reporting for Rack and Sinatra apps
0a253aa 
The AbstractMiddleware tried to set the request parameters before the
app handled the request. This could cause the request object to end up
in a weird state where, if some other middleware (like a JSON payload
parsing middleware) hadn't parsed the request payload yet, the
parameters were reported as empty.

Move the parameter setting to the "after" hook so that it doesn't try to
read the parameters before they're parsed. Use the new
`Transaction#set_params_if_nil` so that it doesn't overwrite any custom
parameters set by the app.

Fixes #1108
@tombruijn tombruijn mentioned this issue Jun 21, 2024
Merged
@tombruijn tombruijn self-assigned this Jun 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@tombruijn tombruijn

Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix parameter reporting for Rack and Sinatra app appsignal/appsignal-ruby
1 participant
@tombruijn