Add filter_metadata config option #973
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
We had a report of an app that contains sensitive information in the request path and the desire to filter this out. We have no system in place to filter metadata like path and request method, as set by the Sinatra middleware. This change allow apps to filter out some metadata that's set by default, like `path`, to avoid sending PII or other sensitive data, using the `filter_metadata` config option. Filtering is done with String based keys, like all the other `filter_*` config options are, so the keys need to be transformed to keys beforehand to make sure they're filtered out. I didn't merge how we set the metadata, now it's set using `Transaction#set_metadata` and through `sample_data` when the Transaction is being sampled as sample data. I've left the behavior the same as much as possible to avoid breaking things. See also this internal discussion: https://appsignal.slack.com/archives/CNPP953E2/p1687785270464119
tombruijn
force-pushed
the
filter-metadata
branch
from
c7d09bd to
caad6db
Compare
2 tasks
tombruijn
requested review from
thijsc,
jeffkreeftmeijer,
luismiramirez and
unflxw
tombruijn
added a commit
to appsignal/test-setups
that referenced
this pull request
Jun 26, 2023
Added in PR appsignal/appsignal-ruby#973 in favor of the `sinatra_sanitized_routes` option.
unflxw
approved these changes
Jun 26, 2023
|
Nit: |
This doesn't filter tags though, even if the metadata appears in the tags box. |
|
@tombruijn Oh! Today I learned there's a separate I think that, in the other integrations, there's no separate code paths for tags and metadata. |
thijsc
approved these changes
Jun 26, 2023
tombruijn
added a commit
to appsignal/test-setups
that referenced
this pull request
Jun 28, 2023
Added in PR appsignal/appsignal-ruby#973 in favor of the `sinatra_sanitized_routes` option.
tombruijn
added a commit
to appsignal/test-setups
that referenced
this pull request
Jun 28, 2023
* Add example app for Sinatra sanitized routes Add test route to Sinatra app to test the sanitized routes / route definition to be set as the `path` metadata to avoid storing PII and other sensitive data. Related PR appsignal/appsignal-ruby#972 * Update config option to filter_metadata Added in PR appsignal/appsignal-ruby#973 in favor of the `sinatra_sanitized_routes` option.
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.
We had a report of an app that contains sensitive information in the request path and the desire to filter this out. We have no system in place to filter metadata like path and request method, as set by the Sinatra middleware.
This change allow apps to filter out some metadata that's set by default, like
path, to avoid sending PII or other sensitive data, using thefilter_metadataconfig option.Filtering is done with String based keys, like all the other
filter_*config options are, so the keys need to be transformed to keys beforehand to make sure they're filtered out.I didn't merge how we set the metadata, now it's set using
Transaction#set_metadataand throughsample_datawhen the Transaction is being sampled as sample data. I've left the behavior the same as much as possible to avoid breaking things.See also this internal discussion: https://appsignal.slack.com/archives/CNPP953E2/p1687785270464119
To do
To do after merge