feat(eap): Track removed keys in EAP trimming processor #5570
Conversation
| i = key.as_str(); | ||
| } | ||
|
|
||
| let split_key = i.to_owned(); |
There was a problem hiding this comment.
I think there's no way around this to_owned; we need the key to be detached from the map or we can't modify it.
jjbayer
left a comment
jjbayer
left a comment
There was a problem hiding this comment.
IMO DeleteValueHard should have carried a remark from the beginning, like the old TODOs suggest. But changing that now seems risky because it would suddenly increase event sizes, which would worse-case cause other data in the event to be dropped.
Introducing a new mode seems like the cautious thing to do, and we can gradually migrate fields to DeprecateHardWithRemark if we want to. I would definitely make sure that the old trimming processor can handle the new delete type as well.
Is it reasonable to introduce a new ProcessingAction for this, or should the behavior of one of the others be modified?
Should DeleteFirm be able to carry data, like the name of the rule that caused the deletion, or maybe the entire remark?
Should we extend this to the original trimming processor? It contains some TODOs.
relay-protocol/src/annotated.rs
Outdated
| self.0 = None; | ||
| self.1.add_remark(Remark { | ||
| ty: RemarkType::Removed, | ||
| rule_id: "delete_firm".to_owned(), |
There was a problem hiding this comment.
nit: I believe something like "trimmed" would provide more valuable info to the user here.
| rule_id: "delete_firm".to_owned(), | |
| rule_id: "trimmed".to_owned(), |
|
|
||
| /// Discards the value entirely, but leaves a remark. | ||
| #[error("value should be hard-deleted (unreachable, should not surface as error!)")] | ||
| DeleteValueFirm, |
There was a problem hiding this comment.
Though I appreciate "firm", I would call this "hard with remark" or just "with remark" instead.
| DeleteValueFirm, | |
| DeleteValueWithRemark, |
There was a problem hiding this comment.
Oh, "Firm" was fully intended as a placeholder name.
I think we need to keep a way of deleting stuff no questions asked because we can't afford to keep arbitrary user-set keys around without some limit. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
The variable tracking the current key was updated after the match statement, causing incorrect split points when breaking early on DeleteValueHard. Move the update before the match to ensure the split key is always current. Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
Apply the same deletion logic pattern from process_attributes to process_array. When size budget is exceeded, mark items with deletion remarks when remark budget allows, falling back to hard deletion only when the remark budget runs out. Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
Add a DeleteAction enum as the return type for delete_value that can be converted into ProcessingAction. This makes the code more explicit about the deletion logic and removes unreachable branches from match statements. Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
loewenheim
changed the title
Co-authored-by: Claude Sonnet 4.5 <[email protected]>
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
This introduces a new variant
DeleteFirm(name provisional) ofProcessingActionthat is intended to sit betweenDeleteHardandDeleteSoft:Hardremoves the value entirely,Firmremoves the value and and puts a remark in the metadata,Softmoves the value into theold_valuein the metadata.This new
ProcessingActionis used in the EAP trimming processor to remember the keys of deleted attributes, up to a limit.Open questions:
ProcessingActionfor this, or should the behavior of one of the others be modified?DeleteFirmbe able to carry data, like the name of the rule that caused the deletion, or maybe the entire remark?ref: #5568. ref: RELAY-196.