-
Notifications
You must be signed in to change notification settings - Fork 69
Conversation
Signed-off-by: Leonardo Di Donato <leodidonato@gmail.com>
Signed-off-by: Leonardo Di Donato <leodidonato@gmail.com>
Signed-off-by: Leonardo Di Donato <leodidonato@gmail.com>
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 this @bastjan! Great work :)
I've pushed some refinements on top of your work.
Before to merge this in, to avoid any confusion I'd call the option you are introducing WithCompliantMsg
because (it seems to me) it basically implements (almost) all that RFC5424 paragraph 6.4 says about the MSG part of RFC5424 Syslog messages.
Which is: if this option is enabled, parse MSG as valid if they are free-form (but not starting with BOM) or a UTF-8 octet sequence starting with a BOM.
Since the default behavior (no option passed in) stays as is, if we want, we could also relax it more, which should lead to even better performances. In this case a lot of test cases should be updated, tho. :)
Thanks for the review and the refinements! I think the name
So the default (no option) would be relaxed to accept any message; |
Yes, exactly @bastjan :) |
Sounds good to me! I could tackle updating the test cases over the next two days if you need any help. |
I can proceed right now with the option rename. But, I'd love if you could help me with the second and last step of this PR (relaxing the default behaviour and refactoring the test cases) |
Signed-off-by: Leonardo Di Donato <leodidonato@gmail.com>
Signed-off-by: Leonardo Di Donato <leodidonato@gmail.com>
Signed-off-by: Leonardo Di Donato <leodidonato@gmail.com>
Sure, glad to help :) |
- Add tests with latin-1 encoding - Add UTF-8 BOM to the start of UTF-8 related tests - Test if non compliant message become compliant if option 'WithCompliantMsg' is off.
Hey @leodido, I relaxed the default parsing, updated the tests (there are many 😅), and rerun the benchmarks in the PR description. Mind to take a look? :) I also relaxed the builder to not create a situation where a parser->builder roundtrip would remove the MSG. Is this alright with you? |
Hey @bastjan yes, relaxing the builder was needed. Ideally, in a second PR, we should approach the same topic for builder. Ie., making the builder configurable (compliant message or not) and its Anyways, you did a great job, thanks for approaching issue #21 .. I'm about to push some little final refinements and then merge this 🎉 |
Signed-off-by: Leonardo Di Donato <leodidonato@gmail.com>
Signed-off-by: Leonardo Di Donato <leodidonato@gmail.com>
Signed-off-by: Leonardo Di Donato <leodidonato@gmail.com>
WithCompliantMsg()
to enforce a compliant MSG as defined by https://tools.ietf.org/html/rfc5424#section-6.4.Usage
Performance
Initialization of machine moved out of benchmark for all comparisons.
develop
...allow-non-utf8
benchstat
develop
...allow-non-utf8.WithCompliantMsg
benchstat
allow-non-utf8
...allow-non-utf8.WithCompliantMsg
benchstat
Fixes #21