feat(processor): Add meta field to react minified processor changes#45030
feat(processor): Add meta field to react minified processor changes#45030AbhiPrasad merged 4 commits intomasterfrom
Conversation
| original_value = exc.get("value") | ||
| if processor.try_process(exc): | ||
| values_meta.enter(index, "value").add_remark( | ||
| {"rule_id": "react_minified_error", "type": "s"}, original_value |
There was a problem hiding this comment.
Not sure about react_minified_error rule id, but I assume this is required. @jan-auer, any suggestions?
There was a problem hiding this comment.
These rule IDs usually refer to data scrubbing rules. All builtin rules are prefixed with @, for example @ip:replace.
In our case we're not doing data scrubbing, so what if we create a new namespace for this, for instance: @processing:react_minified_error (or a shorter version of this)?
There was a problem hiding this comment.
I named it @processing:react. react is grabbed from the processor name itself, so this should be more extensible as well.
jan-auer
left a comment
There was a problem hiding this comment.
Is it possible with reasonable effort to add one test that runs one of the final payloads containing _meta through renormalize, to ensure it's valid?
| for exc in get_path(data, "exception", "values", filter=True, default=()): | ||
|
|
||
| values_meta = meta.enter("exception", "values") | ||
| for index, exc in enumerate(get_path(data, "exception", "values", filter=True, default=())): |
There was a problem hiding this comment.
filter=True means you will skip entries that are None (removed as invalid during ingestion). That means your index is out of sync with the index in data. I'd remove the filter and instead skip inside the loop.
src/sentry/utils/meta.py
Outdated
| range_start: Optional[bytes] | ||
| range_end: Optional[bytes] |
There was a problem hiding this comment.
The ranges here are byte offsets, but not bytes.
There was a problem hiding this comment.
Ah I completely misunderstood you earlier comment, sorry. So int is fine? Or is there a stronger type hint we can use.
There was a problem hiding this comment.
I don't think we have a stronger type hint, and it's also an overkill. We could add a comment on either Remark or add_remark.
I added this here: https://github.com/getsentry/sentry/pull/45030/files#diff-5260559ff17600600ea5618ca49645943f251b211636c4fdc0e1f27ddd2c6700R1522-R1526 Do we need to cover more cases? |
|
Thanks for pointing at it, apparently I scrolled too fast. The test is good. If you like you can update the payload to include a |
ref #44877
Continuing our work from #44884, this PR takes the
add_remarkimplementation from there and uses it to add a substitution remark every time we process a react minified event message.https://github.com/getsentry/relay/blob/d465f2d5effc013039a4fca884bc663efcb46527/relay-general/src/types/meta.rs#L24
In addition, as per feedback, we also test to make sure that store normalization works properly with this new set up.