feat(eap): Normalize deprecated attributes #5257
Conversation
This implements the normalization (backfilling/replacement) of deprecated attribute names based on `sentry-conventions`. The feature applies to spans V2 and logs alike. See the documentation of `normalize_attribute_names` and the `test_normalize_attributes` test for the exact normalization logic.
| old_attribute.set_value(None); | ||
| old_attribute | ||
| .meta_mut() | ||
| .add_remark(Remark::new(RemarkType::Removed, "attribute.deprecated")); |
There was a problem hiding this comment.
Is there something better that we could add to this remark as the "rule ID"?
| eap::normalize_attribute_types(&mut span.attributes); | ||
| eap::normalize_attribute_names(&mut span.attributes); |
There was a problem hiding this comment.
Did we agree on an approach how to handle normalization and PII scrubbing? Was it that PII scrubbing rules need to be migrated on the sentry side? IMO the best approach would be if PII Selectors would match on both field names, but that might be a stretch for this PR.
Even if we don't solve the PII problem right away, I would add a test case that documents current behavior (i.e. user has an advanced rule for a deprecated field, but the normalized new field does not get scrubbed).
There was a problem hiding this comment.
Was it that PII scrubbing rules need to be migrated on the sentry side?
Maybe. It has its upsides, but also massive downsides :(
IMO the best approach would be if PII Selectors would match on both field names
I think that's maybe what we'd have to do, when resolving pii we'd need to consider all aliases.
Even if we don't solve the PII problem right away, I would add a test case that documents current behavior
👍
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Co-authored-by: Joris Bayer <[email protected]>
| old_attribute.set_value(None); | ||
| old_attribute | ||
| .meta_mut() | ||
| .add_remark(Remark::new(RemarkType::Removed, "attribute.deprecated")); |
There was a problem hiding this comment.
I think we'd want to differentiate here on the remarks:
- removed
- moved (renamed)
- removed, not moved (because new attribute already exists)
There was a problem hiding this comment.
- What is the difference between the first and the third?
- Which
RemarkTypewould you use for the second?
There was a problem hiding this comment.
What is the difference between the first and the third?
- it's just a deletion
- it was supposed to be a rename, but the target already existed so it was removed. This rename does degrade to a remove, but to better understand what it is happening, I imagine a hint that something was supposed to be moved but it degraded to a remove is helpful.
Which RemarkType would you use for the second?
Removed fits closest imo, we can further differentiate on the rule_id imo. Not sure if it's worth introducing a rename, when that is essentially a just a remove.
Alternatively, we can also only mark the new value that it was renamed.
| eap::normalize_attribute_types(&mut span.attributes); | ||
| eap::normalize_attribute_names(&mut span.attributes); |
There was a problem hiding this comment.
Was it that PII scrubbing rules need to be migrated on the sentry side?
Maybe. It has its upsides, but also massive downsides :(
IMO the best approach would be if PII Selectors would match on both field names
I think that's maybe what we'd have to do, when resolving pii we'd need to consider all aliases.
Even if we don't solve the PII problem right away, I would add a test case that documents current behavior
👍
…/relay into sebastian/normalize-attributes
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
jjbayer
left a comment
jjbayer
left a comment
There was a problem hiding this comment.
LGTM, do you think you could add the PII test I mentioned?
Even if we don't solve the PII problem right away, I would add a test case that documents current behavior (i.e. user has an advanced rule for a deprecated field, but the normalized new field does not get scrubbed).
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
This implements the normalization (backfilling/replacement) of deprecated attribute names based on
sentry-conventions. The feature applies to spans V2 and logs alike. See the documentation ofnormalize_attribute_namesand thetest_normalize_attributestest for the exact normalization logic.Integration tests are a bit more basic than I would like because right now no attribute is set to
"normalize"insentry-conventions.This PR also updates
sentry-conventionsfrom getsentry/sentry-conventions@7b0c420 to getsentry/sentry-conventions@4c7cda0 to revert the deprecation ofsentry.observed_timestamp_nanos.Closes INGEST-558.