ref(server): Move body compression into EnvelopeManager (INGEST-357) #1085
Conversation
relay-server/src/actors/envelopes.rs
Outdated
| let (envelope_body, envelope_meta) = match Self::encode_envelope(envelope, http_encoding) { | ||
| Ok(v) => v, | ||
| Err(e) => { | ||
| return Box::new(future::err(SendEnvelopeError::SendFailed( |
There was a problem hiding this comment.
i'm not particularly happy about the type of this error cause it's all backwards. i was thinking of making a new variant of SendEnvelopeError (two actually, a BodyEncodingError(std::io::Error) and an EnvelopeBuildError(EnvelopeError)), but then i'd have to carry them into ProcessingError too, and i'm not 100% sure it's worth it? idk
There was a problem hiding this comment.
I'd do exactly that, it's fine to have so many variants
Michcioperz
changed the title
Michcioperz
changed the title
|
i ran some preliminary benchmarks and i get 1500 rps? not very cool. |
| // Override the `sent_at` timestamp. Since the envelope went through basic | ||
| // normalization, all timestamps have been corrected. We propagate the new | ||
| // `sent_at` to allow the next Relay to double-check this timestamp and | ||
| // potentially apply correction again. This is done as close to sending as | ||
| // possible so that we avoid internal delays. |
There was a problem hiding this comment.
Note: I think we need to live with this. This value will be horribly wrong if there is significant amount of data piling up in the queue but if we need to fix that, I question why double-checking is done at all.
There was a problem hiding this comment.
y'know this comment is not mine, i'm just copying it along with the rest
There was a problem hiding this comment.
That's right but you were moving the code below this comment from after the upstream actor queue to before it
There was a problem hiding this comment.
hmm tbh i think i don't quite understand what double checking we have
|
benchmarking again against fake-sentry+nginx. 1000 clients over 4 locust workers. relay pinned to 6 cpu cores. just reading the rps gauge, so no flamegraph overhead.
so gzip is spectacularly good here imo but i'm worried about identity. thinking either a small refactor to skip encoding handling for identity or a canary to see if it matters? |
Instructions and example for changelogFor changes exposed to the Python package, please add an entry to For changes to the Relay server, please add an entry to
To the changelog entry, please add a link to this PR (consider a more descriptive message): - Move body compression into EnvelopeManager (INGEST-357). ([#1085](https://github.com/getsentry/relay/pull/1085))If none of the above apply, you can opt out by adding #skip-changelog to the PR description. |
While researching performance of Relay I discovered that the body compression for upstream requests is run on the system arbiter instead of on a thread pool, and that removing compression increases throughput drastically.
This change moves the workload of compression from UpstreamRelay to EnvelopeProcessor, which runs on a SyncArbiter.