Improve payload cleaning performance #601
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
This is a first iteration of improving how we clean objects when preparing to JSON encode them Currently we iterate over the payload multiple times; sometimes to clean up encoding errors/recursion and other times to filter sensitive data Ideally we should be iterating over the payload once, which is slightly complicated because we should only be filtering parts of the payload (the metadata and breadcrumb metadata)
Helper no longer breaks recursion in 'trim_if_needed', so these tests no longer apply there. However we are still breaking recursion and so can test the same thing elsewhere
For example, in this hash:
{ a: { b: 'c' } }
'c' lives in scope 'a.b' and so should only be filtered if Cleaner
is given 'a.b' in its 'scopes_to_filter'
tomlongridge
reviewed
Jul 4, 2020
This exposed a pretty big regression where filters wouldn't match when they should have. This was caused by us filtering the entire report object in one go, which means the scopes were nested deeper than they were before Previously we filtered the events.metaData directly, so scopes would not include 'events.metaData' and therefore a filter of 'foo' would match 'events.metaData.foo'. Now that we filter the entire report, if a filter relied on 'deep_filters', it would apply and so things that should be redacted wouldn't have been To solve this, we strip each 'scope_to_filter' from the scope before matching it, if deep_filters are enabled The tests passed before this change because we set 'scopes_to_filter' in each test. Now that the instance is shared, the scopes are fetched from the Configuration so this isn't possible and it exposed the bug The tests now cover this case, because they can't set 'scopes_to_filter' directly anymore, so they are testing that the filters they're using match with the Configuration's 'scopes_to_filter'
kattrali
approved these changes
Jul 13, 2020
Merged
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.
Goal
This PR improves performance when sending notifications by only recursively iterating over the report once, rather than doing it several times
We need to iterate through the full payload because we need to filter potentially sensitive data from
metaData, fixup any encoding issues, fix recursive objects and objects that raise when being stringifiedPreviously we would iterate through the report
metaData(and breadcrumb metadata) with filtering enabled, iterate through the rest of the report without filtering it (as we don't want to filter outside ofmetaData) and then iterate through the entire payload (which includes the report) in order to fix encoding/recursion/exceptionsNow we iterate over the full payload in one go, which saves a lot of CPU time
Changeset
Changed
The main
Bugsnag#notifymethod now takes care of filtering as well as fixing up the payload. It knows about which parts of the payload it should apply filtering to based on the newSCOPES_TO_FILTER. This is the same way that the JavaScript notifier applies filtering, for the same performance reasonsTo do this the
Bugsnag::Cleanernow takes ascopes_to_filterparameter on initialisation, which is uses to check if a scope needs to be filteredThis also means that the
Helperclass doesn't apply cleaning itself, but now only trims the payload if it's too longLinked issues
Related to #481
Review
For the submitter, initial self-review:
For the pull request reviewer(s), this changeset has been reviewed for: