feat(pii): Adopt new selectors #818
Conversation
This adds new selectors for the minidump for the heap and stack.
| let slice = data | ||
| .get_mut(range) | ||
| .ok_or(ScrubMinidumpError::InvalidAddress)?; | ||
| let attrs = Cow::Owned(FieldAttrs::new().pii(Pii::Maybe)); |
There was a problem hiding this comment.
You don't have to reinstantiate this I believe? Shouldn't you be able to pass down a Cow::Borrowed for the individual visits?
There was a problem hiding this comment.
I can't pass this to multiple calls and it also is not clonable. And since in the future we'll want some to be set to Pii::Yes de-duplicating this seems more cognitive overhead than it saves. That was the logic anyway, I don't feel strongly so could change this.
|
Good points on the tests, I fixed them but it doesn't actually change the outcome. In particular the |
|
On reflection the original asserts were fine due to the |
|
@flub the remaining one test works as designed right now, |
|
@untitaker I added some tests in selectors.rs which show a bunch of new problems. I spent some time trying to come up with something constructive rather than just find problems but... by now I'm wondering if this is too difficult and perhaps just giving up and shipping what we have is better. |
relay-general/src/pii/minidumps.rs
Outdated
| serde_json::json!( | ||
| { | ||
| "applications": { | ||
| "$minidump.$stack_memory": ["@anything:mask"], |
There was a problem hiding this comment.
This selector doesn't have to work, customers currently use $stack_memory
With some justifications. We'll just have to document these things as known issues.
|
Frankly, I think this entirely defeats the point now since any selector with So I'm actually -1 on merging this. The changes in |
|
To address my own comment, once we change some fields to |
This adds new selectors for the minidump for the heap and stack.
@untitaker as the new tests show this sadly means any generic rule for
$binarywill apply to the stack. Which is what we were trying to avoid. So I'm not sure this proposed change will work. I guess the question is whether we still want to this for just the heap or not.