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.

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

fix: remove inconsistent PII filtering from rust_tracing integration #3773

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

matt-codecov
Copy link
Contributor

the PII filtering in the merged version treats span data set in on_new_span() as safe but span data set in on_record() as sensitive. in actuality, they're two different ways to set the same thing, so they should be treated the same

below i lay out my reasoning for removing this PII filtering. there is a tiny bit more work to do either way:

  • if that approach is accepted, i will update my docs branch to direct the user at the "Scrubbing Sensitive Data" page and i can update the docs for pyo3-python-tracing-subscriber to call out the tracing docs for excluding specific fields that the developer knows are sensitive
  • if we instead do want all-or-nothing PII filtering on by default to be conservative, i will update this PR to apply the filtering to both on_new_span() and on_record() and then update my docs branch to document the send_sensitive_data argument and filtering behavior

(the aforementioned docs branch)

the full scoop

some detail on `tracing` behavior

the #[tracing::instrument(fields(extra1=5, extra2)] attribute at the top of a function adds all of the function's arguments + extra fields listed in the attribute arguments as span data. when on_new_span() is called it will include all of the function arguments and fields, with one exception: fields without default values are not "recorded", so extra2 in this example will be omitted.

during the span, Span::current().record(field, val) can be called to set a value for extra2 after the function starts. it can also overwrite a value for an already-assigned field. on_record() will be called with the recorded fields/values.

to summarize, on_new_span() and on_record() are setting the same data, but on_new_span() sets the data to its default value at the beginning of the function and on_record() can set the data to new values after the function has begun.

the data we get from tracing amounts to stack-local variables and log statements which the Python "Scrubbing Sensitive Data" page list in the section for before_send / before_send_transaction. the python SDK also has event_scrubber for this.

additionally, this integration is basically a port of the Rust SDK's tracing integration which does not use the Rust SDK's version of should_send_default_pii() in this way (on_record() link). the Rust "Scrubbing Sensitive Data" page also describes tracing data in the section for before_send / before_send_transaction

it seems our SDKs typically don't use all-or-nothing filtering for data like this. instead they allow the user to provide targeted filtering at the SDK level with before_send/before_send_transaction/event_scrubber. this PR takes that approach for this integration

@matt-codecov
Copy link
Contributor Author

the web framework CI failures do not appear related to my change but maybe they will go away with a re-run or a merge later

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant