Skip to content
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

Added support for RFC5424 structured data #67

Merged
merged 3 commits into from
Sep 8, 2023

Conversation

tsaarni
Copy link
Contributor

@tsaarni tsaarni commented Aug 17, 2023

This PR adds support for RFC5424 structured data.

The is the same feature as submitted in #1 but since the author did not sign CLA, the implementation is new and not based on the same work.

Note that the tests do not pass until #51 is fixed, for example by applying #55 which was never merged #68.


Part of elastic/logstash#15236

Copy link
Contributor

@andsel andsel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of using nil ad default value you could simply let undefined the default value, in that case the fields already gets nil value. nil values needs nil checks, I think that in this cas you could define as empty string ("") the default value, and no nil check is needed.

@tsaarni
Copy link
Contributor Author

tsaarni commented Aug 23, 2023

I think that in this cas you could define as empty string ("") the default value, and no nil check is needed.

I've used nil as a special value to avoid the need to evaluate the structured_data per every event, unless structured_data is provided by user. If provided by user, it may contain variables and every event should goes through event.sprintf() call

      sd = @structured_data.nil? ? "-" : event.sprintf(@structured_data)

If you think this optimization is not required and it is ok to call formatting for each event regardless, then I can set default to - which means no structured data according to the syslog spec.

@andsel
Copy link
Contributor

andsel commented Aug 25, 2023

You can check for empty string and use as default value the empty string. Empty string is a good practice to say "no value" which is more meaningful then nil. nil is risky because could always escape nil checks and result in runtime exceptions.
I would prefer the empty string.

@tsaarni
Copy link
Contributor Author

tsaarni commented Aug 25, 2023

Fixed!

Copy link
Contributor

@andsel andsel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a couple of nitpicks before a final check

RFC5424 structured data is a string of one or more structured data elements, including brackets.
The elements need to be formatted according to link:https://datatracker.ietf.org/doc/html/rfc5424#section-6.3[RFC5424 section 6.3], for example:

[source,ruby]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not Ruby code, so no need for that highlighting:

Suggested change
[source,ruby]
["source",subs="attributes"]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to double-check: "source" should be in quotes?

I'm not that familiar with asciidoc and I did not see it used that way before. The doc did not seem to explicitly mention quoted element attributes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I recall correctly is indifferent, but looking the logstash-input-beats doc, the double quotes is used.

CHANGELOG.md Outdated
@@ -1,3 +1,6 @@
## Unreleased
- Added support for RFC5424 structured data.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a reference to this PR:

Suggested change
- Added support for RFC5424 structured data.
- Added support for RFC5424 structured data. [#67](https://github.com/logstash-plugins/logstash-output-syslog/pull/67)

@andsel andsel self-requested a review August 25, 2023 11:15
Copy link
Contributor

@andsel andsel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@andsel
Copy link
Contributor

andsel commented Sep 8, 2023

@tsaarni please rebase this PR to the main, so that it can grab the test fixes please 🙏

@andsel
Copy link
Contributor

andsel commented Sep 8, 2023

We keep all these syslog updates under the 3.0.6version, so probably you have a conflict in release notes, but just add the description of this under what's coming from the upstream :-)

@andsel andsel merged commit 27bacbe into logstash-plugins:main Sep 8, 2023
tsaarni added a commit to Nordix/logstash-output-syslog that referenced this pull request Sep 13, 2023
* Added support for RFC5424 structured data

Signed-off-by: Tero Saarni <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Contributing to logstash-output-syslog and status of the plugin?
2 participants