-
Notifications
You must be signed in to change notification settings - Fork 1.7k
fix(codecs): gelf encoder error message #23021
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
Conversation
The error message in case the field name contains incorrect characters is misleading and can cause quite an confusion. The issue can be reproduced with following Vector config: ```yaml sources: test: type: demo_logs format: shuffle decoding: codec: json count: 1 lines: - '{"foo:baz": "baz", "short_message": "x"}' sinks: console: type: console encoding: codec: gelf inputs: - test ``` With current version of vector I got: ```sh ❯ vector -c /tmp/vector.yaml -q 2025-05-09T19:55:47.827681Z ERROR sink{component_kind="sink" component_id=console component_type=console}: vector::internal_events::codecs: Failed serializing frame. error=LogEvent does not contain required field: "foo:baz" error_code="encoder_serialize" error_type="encoder_failed" stage="sending" internal_log_rate_limit=true 2025-05-09T19:55:47.827769Z ERROR sink{component_kind="sink" component_id=console component_type=console}: vector_common::internal_event::component_events_dropped: Events dropped intentional=false count=1 reason="Failed serializing frame." internal_log_rate_limit=true 2025-05-09T19:55:47.827800Z ERROR sink{component_kind="sink" component_id=console component_type=console}: vector::topology: An error occurred that Vector couldn't handle: the task completed with an error. ``` The part where it says `LogEvent does not contain required field: "foo:baz"` is quite confusing, because the field is there and it's certainly not required. (though that might not be obvious for less unusual field name) It looks like the error message was there since early gelf implementation and not really intentional. (possibly a placeholder that was never replaced) The fixed version generates pretty straightforward message: ``` ❯ vector -c /tmp/vector.yaml -q 2025-05-09T20:22:13.387678Z ERROR sink{component_kind="sink" component_id=console component_type=console}: vector::internal_events::codecs: Failed serializing frame. error=LogEvent contains field with invalid character in name: "foo:baz" error_code="encoder_serialize" error_type="encoder_failed" stage="sending" internal_log_rate_limit=true 2025-05-09T20:22:13.388008Z ERROR sink{component_kind="sink" component_id=console component_type=console}: vector_common::internal_event::component_events_dropped: Events dropped intentional=false count=1 reason="Failed serializing frame." internal_log_rate_limit=true 2025-05-09T20:22:13.388544Z ERROR sink{component_kind="sink" component_id=console component_type=console}: vector::topology: An error occurred that Vector couldn't handle: the task completed with an error. ``` This should hopefully be much easier to troubleshoot.
@@ -190,7 +195,7 @@ fn coerce_field_names_and_values( | |||
_ => { | |||
// additional fields must be only word chars, dashes and periods. | |||
if !VALID_FIELD_REGEX.is_match(field) { | |||
return MissingFieldSnafu { | |||
return BadFieldSnafu { |
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.
Hi @mprasil, thank you for this fix!
InvalidFieldSnafu
is a better name.- What do you think about going further like so:
let invalid_chars: Vec<char> = field
.chars()
.filter(|c| !c.is_alphanumeric() && *c != '_' && *c != '.' && *c != '-' && *c != '@')
.collect();
return InvalidFieldSnafu {
field: field.clone(),
invalid_chars,
}
I don't think it's possible to use VALID_FIELD_REGEX to extract the chars so I did it manually.
The second suggestion is optional.
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.
Thank you for your review @pront!
I like the idea of pointing out specific invalid characters, I also considered it, but I was not sure how much logic you'd like to have there when creating the error message. It can get tricky fast. The core of the issue is that the regex does not really check characters so should it change sometimes in the future, the error message might end up being confusing. For example if it becomes more strict and will not allow .
as first character - then the error message would complain about invalid character, but there would be no list of characters to display.
Perhaps as a compromise we can show the pattern being used to validate the field. That should be enough information to figure out the problematic characters without having it listed explicitly? Also perhaps make the error message be more aligned with the check - say that the field name is not valid rather than specifying invalid characters - which is not really what we check for.
I have added commit that:
- Renames the Snafu struct to
InvalidField
as per your suggestion - Makes the Error message more specific by showing the pattern used to check field name
- Rephrases the message to be more aligned with actual check we do
- Renames the changelog fragment to match the expected
<unique_name>.<fragment_type>.md
format to make the CI succeed there.
CI also complains about unknown PR scope "gelf encoder", but I can't quite figure out which of the suggested scopes would be appropriate open to suggestions there if this is important enough to fix.
- Rename the Snafu struct to `InvalidField` - Make the Error message more specific by showing the pattern used to check field name - Rename the changelog fragment to match the expected '<unique_name>.<fragment_type>.md' format.
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!
Head branch was pushed to by a user without write access
Thanks @pront, I see it failed on rustfmt, hopefully the last commit fixed that. (it might need your approval again) |
Summary
The error message in case the field name contains incorrect characters is misleading and can cause quite an confusion.
The issue can be reproduced with following Vector config:
With current version of vector I got:
The part where it says
LogEvent does not contain required field: "foo:baz"
is quite confusing, because the field is there and it's certainly not required. (though that might not be obvious for less unusual field name)It looks like the error message was there since early gelf implementation and not really intentional. (possibly a placeholder that was never replaced)
The fixed version generates pretty straightforward message:
This should hopefully be much easier to troubleshoot.
Change Type
Is this a breaking change?
How did you test this PR?
I've tested it with above configuration.
Does this PR include user facing changes?