feat(data-collection): Port Rails log subscriber #3034
Conversation
* Rails controller span data query inclusion is modified to comply with URL collection spec. * Rails `filter_parameters` are backfilled into `url_query_params` * Allow regexes in `terms` as a result to enable this
* Remove `ParameterFilter` usage completely * Nested payloads will now **not** be scrubbed (we will revisit this later if necessary)
d8a5cd0 to
f24bdc4
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit f24bdc4. Configure here.
| # @return [Hash] a new filtered hash, or an empty hash when collection is off | ||
| def filter(values, cookie: false) | ||
| return {} if mode == :off | ||
| return {} unless values.is_a?(Hash) |
There was a problem hiding this comment.
Hash guard drops Parameters
High Severity
The new is_a?(Hash) guard in filter rejects ActionController::Parameters, which has not inherited from Hash since Rails 5. controller_transaction passes request.params into this method, so span params become {} whenever query-param collection is enabled.
Reviewed by Cursor Bugbot for commit f24bdc4. Configure here.
|
|
||
| filter_parameters = ::Rails.application.config.filter_parameters | ||
| collection.terms = Array(collection.terms) + Array(filter_parameters) | ||
| end |
There was a problem hiding this comment.
Duplicate filter param merge
Medium Severity
New configure_data_collection re-merges Rails filter_parameters into url_query_params.terms, but configuration.rb already does this in after(:configured). The railtie copy also skips type filtering and uniq, so procs can become junk terms and valid terms are duplicated.
Reviewed by Cursor Bugbot for commit f24bdc4. Configure here.
| else | ||
| argument | ||
| end | ||
| end |
There was a problem hiding this comment.
Queues gated by query filter
Medium Severity
Job argument inclusion is gated by data_collection.queues, but Hash arguments are passed through url_query_params.filter. When queues is on and url_query_params.mode is :off, that filter returns {}, so hash arguments are dropped even though queue data collection is enabled.
Reviewed by Cursor Bugbot for commit f24bdc4. Configure here.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| def configure_data_collection | ||
| collection = Sentry.configuration.data_collection.url_query_params | ||
| return unless collection.mode == :deny_list | ||
|
|
There was a problem hiding this comment.
Bug: The new configure_data_collection method adds filter_parameters to collection.terms a second time, creating duplicates and adding invalid Proc objects as garbage strings.
Severity: LOW
Suggested Fix
Remove the new configure_data_collection method and its call from the config.after_initialize block. The existing after(:configured) callback in sentry/rails/configuration.rb already handles adding filter_parameters correctly, including filtering for Proc objects and deduplicating the list.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: sentry-rails/lib/sentry/rails/railtie.rb#L89
Potential issue: The new `configure_data_collection` method, called during
`config.after_initialize`, appends `Rails.application.config.filter_parameters` to
`Sentry.configuration.send_default_pii_collection.terms`. This duplicates an existing
`after(:configured)` callback that already performs this action during `Sentry.init`.
The new implementation lacks the filtering present in the original, causing `Proc`
objects to be converted to useless strings like `"#<proc:0x...>"`. This results in a
bloated `terms` list containing both duplicate valid parameters and permanent, invalid
entries, though it does not affect filtering correctness.
Also affects:
sentry-rails/lib/sentry/rails/configuration.rb:37
Did we get this right? 👍 / 👎 to inform future reviews.
1 participant
ParameterFilterusage completelyStack created with GitHub Stacks CLI • Give Feedback 💬
Issues