fix(server): Accept and forward unknown envelope items #1246
Conversation
jan-auer
force-pushed
the
fix/accept-unknown-items
branch
from
baed735
to
7750f6d
Compare
|
In Point of Presence Relays, the new configuration flag should be set to |
jan-auer
force-pushed
the
fix/accept-unknown-items
branch
from
7750f6d
to
e8b5710
Compare
| /// By default, items of this type are forwarded without modification. Processing Relays and | ||
| /// Relays explicitly configured to do so will instead drop those items. This allows | ||
| /// forward-compatibility with new item types where we expect outdated Relays. | ||
| Unknown(String), |
There was a problem hiding this comment.
Note to reviewers: A major part of this diff is caused by dropping Copy as a result of using a String here, and it makes using ItemType rather unergonomic.
In practice, our item types are supposed to be really short. mem::size_of::<String>() == 24, which means without size penalty, we could allocate a short static string here, instead, to restore Copy.
There was a problem hiding this comment.
I would do that within this PR but up to you
* master: fix(metrics): Enforce metric name length limit (#1238)
| /// By default, items of this type are forwarded without modification. Processing Relays and | ||
| /// Relays explicitly configured to do so will instead drop those items. This allows | ||
| /// forward-compatibility with new item types where we expect outdated Relays. | ||
| Unknown(String), |
There was a problem hiding this comment.
I would do that within this PR but up to you
relay-config/src/config.rs
Outdated
| /// | ||
| /// Defaults to `true` for all Relay modes other than processing mode. In processing mode, this | ||
| /// is disabled by default since the item cannot be handled. | ||
| unknown_items: Option<bool>, |
There was a problem hiding this comment.
nit: Should we call this accept_unknown_items?
There was a problem hiding this comment.
I originally had it as forward_unknown_items, but it seemed rather long. Subjectively, the combined key of routing.unknown_items: false is clear to me, but I'm happy to update to a more descriptive name if you prefer.
There was a problem hiding this comment.
I thought the same as joris but I get the reasoning from jan as well, so i don't have an opinion on this
There was a problem hiding this comment.
I thought it would make sense to make it consistent with the function name, but I did not consider the routing. prefix. With the prefix I think it's clear enough.
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.
Relay has a policy to accept and forward unknown data on near every level, from individual fields in the event protocol to entire Envelope items. However, unknown envelope item types failed envelope parsing which lead to dropping the full Envelope with all its items.
This patch introduces a new
ItemType::Unknownthat contains the original, case sensitive, item type. Based on a new config flagrouting.accept_unknown_items, this item is either retained and forwarded without modification, or is dropped immediately after parsing the envelope. The default isfalsefor processing Relays (to drop unknown items), andtruefor all other configurations.Unknown items are treated as event-independent. This means, they are split away from an event into a separate envelope. This is the most likely case for future event payloads. As with all other feature updates, customers and users are still recommended to update Relay when adopting new major features on their clients.
Alongside this change, empty envelopes no longer cause a
400 Bad Requestresponse. Most importantly, this allows clients to send envelopes containing all new items without receiving an error. As a side effect, this effectively makes sending empty envelopes legal in the ingestion API. To diagnose SDK bugs in the future, we can start to add internal diagnostics.See also: