-
Notifications
You must be signed in to change notification settings - Fork 114
feat(attachments): Migrate standalone logic #5703
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
Changes from all commits
8d61d82
3080083
9d30699
e31ba26
f2af288
4990f24
3e99a9e
2c0674b
c3a1bc5
b22e064
51816ad
3e542be
60269da
7600e86
3c7ccca
3e64ddd
1587af6
f4f644d
bf956af
a660b09
89ac3c9
4bf8560
53da87b
bcdd552
5d4ea17
6eb71e4
2c35b8e
87e70d0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,55 @@ | ||
| use crate::Envelope; | ||
| use crate::managed::{Managed, Rejected}; | ||
| use crate::processing::attachments::AttachmentsOutput; | ||
| use crate::processing::{self, Forward}; | ||
|
|
||
| impl Forward for AttachmentsOutput { | ||
| fn serialize_envelope( | ||
| self, | ||
| _: processing::ForwardContext<'_>, | ||
| ) -> Result<Managed<Box<Envelope>>, Rejected<()>> { | ||
| let Self(attachments) = self; | ||
| Ok(attachments.map(|attachments, _| { | ||
| Envelope::from_parts(attachments.headers, attachments.attachments) | ||
| })) | ||
| } | ||
|
|
||
| #[cfg(feature = "processing")] | ||
| fn forward_store( | ||
| self, | ||
| s: processing::StoreHandle<'_>, | ||
| ctx: processing::ForwardContext<'_>, | ||
| ) -> Result<(), Rejected<()>> { | ||
| use crate::processing::attachments::Error; | ||
|
|
||
| let Self(attachments) = self; | ||
|
|
||
| let Some(event_id) = attachments.headers.event_id() else { | ||
| return Err(attachments.reject_err(Error::NoEventId).map(drop)); | ||
| }; | ||
|
|
||
| let use_objectstore = { | ||
| let options = &ctx.global_config.options; | ||
| crate::utils::sample(options.objectstore_attachments_sample_rate).is_keep() | ||
| }; | ||
|
|
||
| for attachment in attachments.split(|attachment| attachment.attachments) { | ||
| let store_attachment = attachment.map(|attachment, _| { | ||
| use crate::services::store::StoreAttachment; | ||
| let quantities = attachment.quantities(); | ||
| StoreAttachment { | ||
| event_id, | ||
| attachment, | ||
| quantities, | ||
| } | ||
| }); | ||
| if use_objectstore { | ||
| s.send_to_objectstore(store_attachment); | ||
| } else { | ||
| s.send_to_store(store_attachment); | ||
| } | ||
| } | ||
|
|
||
| Ok(()) | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,138 @@ | ||||||||||||||||||||||||||||||||||||
| use std::sync::Arc; | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| use relay_quotas::RateLimits; | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| use crate::envelope::{EnvelopeHeaders, Item, ItemType, Items}; | ||||||||||||||||||||||||||||||||||||
| use crate::managed::{Counted, Managed, ManagedEnvelope, OutcomeError, Quantities, Rejected}; | ||||||||||||||||||||||||||||||||||||
| use crate::processing::{self, CountRateLimited, Output, QuotaRateLimiter}; | ||||||||||||||||||||||||||||||||||||
| #[cfg(feature = "processing")] | ||||||||||||||||||||||||||||||||||||
| use crate::services::outcome::DiscardReason; | ||||||||||||||||||||||||||||||||||||
| use crate::services::outcome::Outcome; | ||||||||||||||||||||||||||||||||||||
| use crate::statsd::RelayCounters; | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| mod forward; | ||||||||||||||||||||||||||||||||||||
| mod process; | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| #[derive(Debug, thiserror::Error)] | ||||||||||||||||||||||||||||||||||||
| pub enum Error { | ||||||||||||||||||||||||||||||||||||
| /// The Attachment was rate limited. | ||||||||||||||||||||||||||||||||||||
| #[error("rate limited")] | ||||||||||||||||||||||||||||||||||||
| RateLimited(RateLimits), | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| /// The envelope did not contain an event ID. | ||||||||||||||||||||||||||||||||||||
| #[cfg(feature = "processing")] | ||||||||||||||||||||||||||||||||||||
| #[error("missing event ID")] | ||||||||||||||||||||||||||||||||||||
| NoEventId, | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| impl OutcomeError for Error { | ||||||||||||||||||||||||||||||||||||
| type Error = Self; | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| fn consume(self) -> (Option<crate::services::outcome::Outcome>, Self::Error) { | ||||||||||||||||||||||||||||||||||||
| let outcome = match &self { | ||||||||||||||||||||||||||||||||||||
| Self::RateLimited(limits) => { | ||||||||||||||||||||||||||||||||||||
| let reason_code = limits.longest().and_then(|limit| limit.reason_code.clone()); | ||||||||||||||||||||||||||||||||||||
| Some(Outcome::RateLimited(reason_code)) | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| #[cfg(feature = "processing")] | ||||||||||||||||||||||||||||||||||||
| Self::NoEventId => Some(Outcome::Invalid(DiscardReason::Internal)), | ||||||||||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I assume Relay somehow already ensures there is always an event id? What happens if the there is a standalone attachment sent in an envelope without event id? Does it make sense to ingest it? And does that actually mean it's
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The old logic checks it in the store here, so this check "just moved into the processor". relay/relay-server/src/services/store.rs Line 371 in bf82189
The outcome is the same as that of the store error (but we can ofcourse improve it): relay/relay-server/src/services/store.rs Lines 52 to 67 in bf82189
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 Figure we could/should do an explicit validation in the processing code and then reject as invalid but not internal, if it can be caused by users sending broken data. |
||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||
| (outcome, self) | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| impl From<RateLimits> for Error { | ||||||||||||||||||||||||||||||||||||
| fn from(value: RateLimits) -> Self { | ||||||||||||||||||||||||||||||||||||
| Self::RateLimited(value) | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
Comment on lines
+44
to
+48
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Gets this funky error: |
||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| /// A processor for Attachments. | ||||||||||||||||||||||||||||||||||||
| pub struct AttachmentProcessor { | ||||||||||||||||||||||||||||||||||||
| limiter: Arc<QuotaRateLimiter>, | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| impl AttachmentProcessor { | ||||||||||||||||||||||||||||||||||||
| /// Creates a new [`Self`]. | ||||||||||||||||||||||||||||||||||||
| pub fn new(limiter: Arc<QuotaRateLimiter>) -> Self { | ||||||||||||||||||||||||||||||||||||
| Self { limiter } | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| impl processing::Processor for AttachmentProcessor { | ||||||||||||||||||||||||||||||||||||
| type UnitOfWork = SerializedAttachments; | ||||||||||||||||||||||||||||||||||||
| type Output = AttachmentsOutput; | ||||||||||||||||||||||||||||||||||||
| type Error = Error; | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| fn prepare_envelope( | ||||||||||||||||||||||||||||||||||||
| &self, | ||||||||||||||||||||||||||||||||||||
| envelope: &mut ManagedEnvelope, | ||||||||||||||||||||||||||||||||||||
| ) -> Option<Managed<Self::UnitOfWork>> { | ||||||||||||||||||||||||||||||||||||
| debug_assert!( | ||||||||||||||||||||||||||||||||||||
| !envelope.envelope().items().any(Item::creates_event), | ||||||||||||||||||||||||||||||||||||
| "AttachmentProcessor should not receive items that create events" | ||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| let attachments = envelope | ||||||||||||||||||||||||||||||||||||
| .envelope_mut() | ||||||||||||||||||||||||||||||||||||
| .take_items_by(|i| i.requires_event() && matches!(i.ty(), ItemType::Attachment)); | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| if attachments.is_empty() { | ||||||||||||||||||||||||||||||||||||
| return None; | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| let headers = envelope.envelope().headers().clone(); | ||||||||||||||||||||||||||||||||||||
| let work = SerializedAttachments { | ||||||||||||||||||||||||||||||||||||
| headers, | ||||||||||||||||||||||||||||||||||||
| attachments, | ||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||
| Some(Managed::with_meta_from(envelope, work)) | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| async fn process( | ||||||||||||||||||||||||||||||||||||
| &self, | ||||||||||||||||||||||||||||||||||||
| attachments: Managed<Self::UnitOfWork>, | ||||||||||||||||||||||||||||||||||||
| ctx: processing::Context<'_>, | ||||||||||||||||||||||||||||||||||||
| ) -> Result<processing::Output<Self::Output>, Rejected<Self::Error>> { | ||||||||||||||||||||||||||||||||||||
| for item in &attachments.attachments { | ||||||||||||||||||||||||||||||||||||
| let attachment_type_tag = match item.attachment_type() { | ||||||||||||||||||||||||||||||||||||
| Some(t) => &t.to_string(), | ||||||||||||||||||||||||||||||||||||
| None => "", | ||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||
| relay_statsd::metric!( | ||||||||||||||||||||||||||||||||||||
| counter(RelayCounters::StandaloneItem) += 1, | ||||||||||||||||||||||||||||||||||||
| processor = "new", | ||||||||||||||||||||||||||||||||||||
| item_type = item.ty().name(), | ||||||||||||||||||||||||||||||||||||
| attachment_type = attachment_type_tag, | ||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| let mut attachments = self.limiter.enforce_quotas(attachments, ctx).await?; | ||||||||||||||||||||||||||||||||||||
| process::scrub(&mut attachments, ctx)?; | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| Ok(Output::just(AttachmentsOutput(attachments))) | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| /// Serialized attachments extracted from an envelope. | ||||||||||||||||||||||||||||||||||||
| #[derive(Debug)] | ||||||||||||||||||||||||||||||||||||
| pub struct SerializedAttachments { | ||||||||||||||||||||||||||||||||||||
| /// Original envelope headers. | ||||||||||||||||||||||||||||||||||||
| headers: EnvelopeHeaders, | ||||||||||||||||||||||||||||||||||||
| /// A list of attachments. | ||||||||||||||||||||||||||||||||||||
| attachments: Items, | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| impl Counted for SerializedAttachments { | ||||||||||||||||||||||||||||||||||||
| fn quantities(&self) -> Quantities { | ||||||||||||||||||||||||||||||||||||
| self.attachments.quantities() | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| impl CountRateLimited for Managed<SerializedAttachments> { | ||||||||||||||||||||||||||||||||||||
| type Error = Error; | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| /// Output produced by the [`AttachmentProcessor`]. | ||||||||||||||||||||||||||||||||||||
| #[derive(Debug)] | ||||||||||||||||||||||||||||||||||||
| pub struct AttachmentsOutput(Managed<SerializedAttachments>); | ||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| use crate::managed::{Managed, Rejected}; | ||
| use crate::processing::attachments::{Error, SerializedAttachments}; | ||
| use crate::processing::{self, utils}; | ||
|
|
||
| /// Runs PiiProcessors on the attachments. | ||
| pub fn scrub( | ||
| attachments: &mut Managed<SerializedAttachments>, | ||
| ctx: processing::Context<'_>, | ||
| ) -> Result<(), Rejected<Error>> { | ||
| attachments.try_modify(|attachments, records| { | ||
| utils::attachments::scrub( | ||
| attachments.attachments.iter_mut(), | ||
| ctx.project_info, | ||
| Some(records), | ||
| ); | ||
| Ok::<_, Error>(()) | ||
| }) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Move imports to top of file (feature gated).