-
Notifications
You must be signed in to change notification settings - Fork 87
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
feat(metrics): Actually batch by partition, set request header [INGEST-1562] #1440
Conversation
fut.spawn(context); | ||
} | ||
}); | ||
let num_partitions = self.config.flush_partitions.unwrap_or(1); |
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.
This means that we always set the X-Sentry-Relay-Shard
header, even though no partitioning was requested by config. We could easily change this behavior but I thought this would keep the code nice and simple.
@@ -554,6 +554,10 @@ impl UpstreamRelay { | |||
} | |||
} | |||
|
|||
if let Some(partition_key) = request.request.partition_key() { | |||
builder.header("X-Sentry-Relay-Shard", partition_key); |
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.
TODO: Decide on the actual name for the header.
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.
Going with X-Sentry-Relay-Shard
. The proposal mentions X-Relay-Shard
, but other headers we set also start with X-Sentry-Relay-
.
@@ -66,6 +66,7 @@ pub struct SendEnvelope { | |||
pub http_encoding: HttpEncoding, | |||
pub response_sender: Option<oneshot::Sender<Result<(), SendEnvelopeError>>>, | |||
pub project_key: ProjectKey, | |||
partition_key: Option<String>, |
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.
Having a String
here means that other envelopes could theoretically use the same header with a different partitioning strategy.
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.
PR looks fine, all i commented are nits. let's do a careful canary deploy tomorrow
relay-server/src/actors/envelopes.rs
Outdated
@@ -353,6 +362,7 @@ impl fmt::Debug for SendMetrics { | |||
.field("buckets", &self.buckets) |
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.
@jan-auer any idea why we don't use derive(Debug)
here?
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.
I think we couldn't because SendMetrics
used to hold an Addr<Project>
. Now that Project
is not an actor anymore, and this struct only holds a ProjectKey
, I think we can use derive(Debug)
.
Convert the dry run implemented in #1425 into an actual batching mechanism that splits metrics buckets into logical partitions.
The
partition_key
has to be passed throughProjectCache
,EnvelopeManager
andUpstreamRelay
to be set as a header on the outgoing envelope request.