-
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
fix(metrics-extraction): Add defaulting of required attributes in light normalization #2961
Conversation
event.platform.get_or_insert_with(|| "other".to_string()); | ||
event.level.get_or_insert_with(|| match event.ty.value() { |
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 have moved both platform
and level
normalization, since both defaulted to values which might be useful for metrics extraction. I could technically move all the default normalizations, @iker-barriocanal what do you think?
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 is good enough for now, thanks!
@@ -1146,6 +1147,8 @@ mod tests { | |||
..Default::default() | |||
}; | |||
|
|||
assert_eq!(Some(Val::String("info")), event.get_value("event.level")); |
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.
Added this test just to make sure the getter works as expected.
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 working on this! Just one minor comment.
// Set required attributes to default values if not present. | ||
normalize_default_required_attrs(event); |
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.
Let's move this lower, as the event may still be rejected after this point. There are some required attribute normalization on line 255, that could be a good place.
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 tip! Will do
event.platform.get_or_insert_with(|| "other".to_string()); | ||
event.level.get_or_insert_with(|| match event.ty.value() { |
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 is good enough for now, thanks!
This PR adds a new normalization step in the light normalization pipeline which normalizes required attributes that have no value. The need for this change arises from metrics extraction, which is run after light normalization and requires the event payload to be normalized almost entirely to perform the correct conditional extraction.