Feature: privacy mode to suppress logs and tracing (V2)#94
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements a privacy mode feature (V2) that suppresses logs and tracing for requests that include an X-Privacy-Mode header. The implementation tags request IDs with a "privacy_mode" suffix when the header is detected, and a new PrivacyModeLayer in the tracing subscriber filters out log events and spans for requests with this tag.
Changes:
- Added
X-Privacy-Modeheader constant and detection logic in request context extraction - Created
PrivacyModeLayerto filter logs/traces based on privacy mode tag in request ID - Integrated privacy filter layer into all logging subscriber configurations
- Added test parameter to existing integration tests to exercise privacy mode behavior
Reviewed changes
Copilot reviewed 9 out of 11 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| rust/lit-core/lit-api-core/src/context/mod.rs | Adds X-Privacy-Mode header detection and appends privacy_mode tag to request/correlation IDs when header is present |
| rust/lit-core/lit-observability/src/logging/privacy_filter.rs | New privacy mode layer that filters logs/spans by checking for privacy_mode tag in request context |
| rust/lit-core/lit-observability/src/logging/mod.rs | Integrates PrivacyModeLayer into simple logging subscribers |
| rust/lit-core/lit-observability/src/lib.rs | Adds PRIVACY_MODE_TAG constant and integrates PrivacyModeLayer into main subscriber |
| rust/lit-core/lit-observability/Cargo.toml | Adds explicit tracing-core dependency |
| rust/lit-node/lit-node/tests/common/lit_actions.rs | Adds add_privacy_mode parameter to test helper and header injection logic |
| rust/lit-node/lit-node/tests/integration/lit_actions.rs | Adds test case with privacy mode enabled |
| rust/lit-node/lit-node/tests/integration/session_sigs.rs | Updates test calls with false for add_privacy_mode parameter |
| rust/lit-node/lit-node/tests/common/web_user_tests.rs | Updates test call with false for add_privacy_mode parameter |
| rust/lit-core/lit-observability/src/tracing/mod.rs | Whitespace formatting change (trailing space) |
| rust/lit-node/Cargo.lock | Transitive dependency version updates |
| rust/lit-core/Cargo.lock | Transitive dependency version updates |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #[test_case( | ||
| true, | ||
| "Anyone can sign with a MGB PKP within its permitted Lit Action - Privacy Mode Enabled", | ||
| true |
There was a problem hiding this comment.
The privacy mode test case doesn't programmatically verify that logs are suppressed when privacy mode is enabled. According to the PR description, this test is meant to be verified manually by inspecting logs. Consider adding a comment explaining that this test case should be validated by observing that request ID logs are suppressed when privacy mode is enabled, to help future maintainers understand the test's purpose.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
PASS [ 44.373s] (3/3) lit_node::test toxiproxy::perf_tests::load_with_no_latency |
Garandor
left a comment
Garandor
left a comment
There was a problem hiding this comment.
two suggestions, otherwise LGTM!
|
one more note, I think we should at least be able to track the number of transactions made using privacy mode to see if the feature has any adoption. I'd add an otel counter to trigger each time a request with X-PRIVACY header is found, but this could also be done in a followup |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
* proposed changes * count privacy mode requests received via otel
Garandor
left a comment
Garandor
left a comment
There was a problem hiding this comment.
proposed changes merged in, LGTM from my end and ready to go when CI passes
2 participants
WHAT
Rework of #17 , removing the added threading concerns and simplifiying the concept. This PR now simply adds the tag
privacy_modeto request ids where a header has been added indicating that privacy mode is desired.NOTES
We may want to change this slightly to simply add another field to the context within layers, but this is a quick way to guarantee that the solution works, without having to evaluate specific logs.
The test is relatively straightforward - I added a parameter to an existing test forcing it to either use privacy mode or not. If it's not being used, we see the "req: 203942309" values in the logs. If it's being used, these lines are suppressed. ( The interim test was to add the privacy mode tag, but NOT suppress the logs, to prove this works as well! ).