Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(server): Forward-compatible AttachmentType #1638

Merged
merged 6 commits into from
Nov 28, 2022

Conversation

jjbayer
Copy link
Member

@jjbayer jjbayer commented Nov 25, 2022

As we did in #1246 for ItemType, add an Unknown(String) variant to AttachmentType. When accept_unknown_items is set to false, items with this attachment type will be dropped.

This will allow outdated external Relays to forward new attachment types instead of dropping the entire envelope.

This defect was discovered in getsentry/rfcs#33, which introduces a new attachment type.

let breadcrumbs2 = envelope
.take_item_by(|item| item.attachment_type() == Some(AttachmentType::Breadcrumbs));
.take_item_by(|item| item.attachment_type() == Some(&AttachmentType::Breadcrumbs));
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AttachmentType is not Copy anymore, because one of its variants now contains a String.

@jjbayer jjbayer marked this pull request as ready for review November 25, 2022 08:51
@jjbayer jjbayer requested review from a team and brustolin November 25, 2022 08:51
Copy link
Contributor

@iker-barriocanal iker-barriocanal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we adding an Unknown(String) variant instead of a specific variant for the view hierarchy? Having a fallback unknown variant seems ok to me, but I wonder if it's the best solution for that specific case we're now trying to solve.

relay-server/src/envelope.rs Show resolved Hide resolved
relay-server/src/actors/store.rs Outdated Show resolved Hide resolved
relay-server/src/envelope.rs Outdated Show resolved Hide resolved
relay-server/src/envelope.rs Outdated Show resolved Hide resolved
@jjbayer
Copy link
Member Author

jjbayer commented Nov 25, 2022

Why are we adding an Unknown(String) variant instead of a specific variant for the view hierarchy?

@iker-barriocanal we will add a specific variant for the view hierarchy in a different PR, so this PR actually won't help with the rollout of that new type. It only ensures that when we add attachment types in the future, envelopes will not be dropped by external Relays.

@jjbayer jjbayer requested a review from jan-auer November 25, 2022 13:04
@jjbayer jjbayer enabled auto-merge (squash) November 28, 2022 11:54
@jjbayer jjbayer merged commit ab46f30 into master Nov 28, 2022
@jjbayer jjbayer deleted the fix/attachment-type-forward branch November 28, 2022 12:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants