feat(spans): Store span attachments #5423
Conversation
| @@ -1,5 +1,6 @@ | |||
| use std::collections::BTreeMap; | |||
|
|
|||
| use itertools::Either; | |||
There was a problem hiding this comment.
Either seems to get imported from a couple of different crates, not sure if this is the canonical one.
| project_id: ctx.scoping.project_id.value(), | ||
| trace_id: trace_id.into_value()?.to_string(), | ||
| item_id: attachment_id.into_value()?.into_bytes().to_vec(), | ||
| item_type: 10, // FIXME: use enum from sentry-protos |
There was a problem hiding this comment.
Depends on getsentry/sentry-protos#156. Do not merge until it's published.
![sentry[bot]](https://avatars.githubusercontent.com/u/1396951?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
jjbayer
force-pushed
the
feat/upload-attachment-v2
branch
from
feccbe5 to
02e2506
Compare
| categories=None, | ||
| quantity=None, | ||
| timeout=1, | ||
| timeout=None, |
There was a problem hiding this comment.
Drive-by: assert_rate_limited did not use a shorter timeout than the default one so was prone to flake.
| span_id: Option<SpanId>, | ||
| } | ||
|
|
||
| // TODO: remove code-duplication between logs, trace metrics and attachments. |
| } = fields; | ||
|
|
||
| result.insert( | ||
| "sentry.content-type".to_owned(), |
There was a problem hiding this comment.
Wonder if it's better to have another namespace for these attributes sentry.attachment.content-type, but maybe just having an EAP item 'attachment' is namespace enough.
There was a problem hiding this comment.
Personally l like the un-prefixed version, but I have no strong opinions on this.
relay-server/src/managed/managed.rs
Outdated
| } | ||
|
|
||
| impl<L: Counted, R: Counted> Managed<Either<L, R>> { | ||
| /// Turns a managed option into an optional [`Managed`]. |
There was a problem hiding this comment.
This comment still refers to Option.
|
|
||
| let objectstore_client = Client::builder(objectstore_url).build()?; | ||
| let attachments_usecase = Usecase::new("attachments") | ||
| let event_attachments = Usecase::new("attachments") |
There was a problem hiding this comment.
Not sure if this can be changed at this point, but might be clearer to call this "event_attachments"?
There was a problem hiding this comment.
There is code in the sentry monolith that expects "attachments", so I'm afraid we need to keep it: https://github.com/getsentry/sentry/blob/70575b5ffb0266231d0dc48445c70bf285770800/src/sentry/objectstore/__init__.py#L41
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to a conflict with the base branch
relay-server/src/services/upload.rs
Outdated
| match message { | ||
| Upload::Envelope(envelope) => { | ||
| // Event attachments can still go the old route. | ||
| self.inner.store.send(envelope); | ||
| } | ||
| Upload::Attachment(managed) => { | ||
| let _ = managed.reject_err(Error::LoadShed); | ||
| } | ||
| } |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Yes but this is deliberate (for now).
Upload attachment V2 bodies to objectstore and write a trace item to EAP.
Closes INGEST-614.