-
Notifications
You must be signed in to change notification settings - Fork 94
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(spans): Route spans according to trace_id #3387
Conversation
relay-server/src/services/store.rs
Outdated
@@ -1378,16 +1378,15 @@ struct SpanKafkaMessage<'a> { | |||
/// Number of days until these data should be deleted. | |||
#[serde(default)] | |||
retention_days: u16, | |||
#[serde(default, skip_serializing_if = "Option::is_none")] | |||
segment_id: Option<&'a str>, | |||
segment_id: &'a str, |
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.
We deserialize a SpanKafkaMessage
from a Span
, right? Can we be sure that it has a segment_id
?
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.
Reverted since we're using trace_id
and not segment_id
for routing.
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.
Thanks for the PR description!
We're in need of buffering spans later in the pipeline and it helps to have them on the same partition. We only need them routed per
segment_id
but since it's not aUuid
, usingtrace_id
is simpler. An alternative would be to properly transform thesegment_id
into au64
and then callUuid::from_u64_pair(segment_id, 0)
but that seems unnecessary.