-
Notifications
You must be signed in to change notification settings - Fork 93
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): Normalize and copy measurements to segments #2953
Conversation
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.
As discussed, instead of running normalization on spans extracted from events twice, let's make SpanKafkaMessage
as strict as the kafka schema and "normalize" the message before producing, like we do here:
relay/relay-server/src/services/processor/span/processing.rs
Lines 398 to 408 in 7970a7c
sentry_tags.retain(|key, value| match value.value() { | |
Some(s) => { | |
match key.as_str() { | |
"group" => { | |
// Only allow up to 16-char hex strings in group. | |
s.len() <= 16 && s.chars().all(|c| c.is_ascii_hexdigit()) | |
} | |
"status_code" => s.parse::<u16>().is_ok(), | |
_ => true, | |
} | |
} |
We should probably also make the numeric values in SpanKafkaMessage
the new FiniteF64
, because serde_json
will serialize infinity / NaN
as null
as well.
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.
Please see comments before merging. No blockers, but we still cannot be 100% sure that this won't generate new validation errors on the consumer side. To prevent that, I think we need to make SpanKafkaMessage
more strict (this would sacrifice some performance) and / or make the consumer more lenient. See also Sentry's development philosophy:
“be conservative in what you send, be liberal in what you accept”.
The second bullet point in the PR description can be removed I believe.
7f356d2
to
32d66ee
Compare
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.
See comments about RawValue
.
This will: