feat: Support uploading attachments directly to objectstore#5367
feat: Support uploading attachments directly to objectstore#5367
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Swatinem
force-pushed
the
swatinem/store-async
branch
3 times, most recently
from
cd9d7f2 to
72eff76
Compare
relay-server/src/services/upload.rs
Outdated
| assert!(rx.recv().await.is_none()); | ||
| } | ||
| } | ||
| // #[cfg(test)] |
There was a problem hiding this comment.
We either rely 100% on integration tests, or mock the objectstore somehow.
There was a problem hiding this comment.
the objectstore client is not the reason I commented this out, but rather that setting up all of the store endpoint, etc was too much of a pain.
Its a bit unfortunate that forwarding to Store is now a sideeffect that I can not easily test. Maybe its worth changing things in such a way that I can just test with envelopes directly, without going through all that channel nonesense.
Basically just testing the actual logic, and not all the overhead wrapped around it.
There was a problem hiding this comment.
There is Addr::custom() to set up a dummy receiver that you can inspect.
jjbayer
left a comment
jjbayer
left a comment
There was a problem hiding this comment.
I like the direct flow of the envelope from the upload service to the store service. Apart from some minor comments I think we can merge this, but the question remains how we are going to reconcile the event attachments use case with the span attachment use case.
relay-server/src/services/upload.rs
Outdated
| assert!(rx.recv().await.is_none()); | ||
| } | ||
| } | ||
| // #[cfg(test)] |
There was a problem hiding this comment.
We either rely 100% on integration tests, or mock the objectstore somehow.
Swatinem
force-pushed
the
swatinem/store-async
branch
from
6955a30 to
0de7084
Compare
Swatinem
force-pushed
the
swatinem/store-async
branch
from
6a7c235 to
c2790f7
Compare
jjbayer
left a comment
jjbayer
left a comment
There was a problem hiding this comment.
Looks like the PR description is outdated.
This makes part of the `StoreService` async, and has to pass through the `ManagedEnvelope` into `store_envelope`, as we have to derive a `Managed` instance of the attachment from that. Other than that, this does not look too bad, except that going through a channel is just a pain.
Swatinem
force-pushed
the
swatinem/store-async
branch
from
c2790f7 to
e7c8e3b
Compare
3 participants
Changes to the
UploadServiceto handle whole envelopes, uploading attachments and mutating the envelope items in-place, before forwarding it to theStoreServicefor further forwarding to kafka.