Add syslog writer #507

pkarlowicz · 2023-07-29T21:49:29Z

Description

Linked issue:

#156

Linked website pull request, if the documention of https://tinylog.org/v2/ has to be updated:

tinylog-org/website#000

Definition of Done

I read contributing.md
There are no TODOs left in the code
Code style follows the tinylog standard
All classes and methods have Javadoc
Changes are covered by JUnit tests including edge cases, errors, and exception handling
Maven build works including compiling, tests, and checks (mvn verify)
Changes are committed by a verified email address that is assigned to the GitHub account (https://github.com/settings/emails)
Documentation on the tinylog website is up-to-date or updated by a pull request to the repository tinylog-org/website

Agreements

I agree that my changes will be published under the terms of the Apache License 2.0 (mandatory)
I agree that my GitHub user name will be published in the release notes (optional)

codecov · 2023-07-29T22:00:29Z

Codecov Report

Merging #507 (109a52f) into v2.7 (512e053) will decrease coverage by 0.14%.
Report is 2 commits behind head on v2.7.
The diff coverage is 88.33%.

@@             Coverage Diff              @@
##               v2.7     #507      +/-   ##
============================================
- Coverage     94.46%   94.33%   -0.14%     
- Complexity     2799     2830      +31     
============================================
  Files           143      149       +6     
  Lines          5457     5577     +120     
  Branches        714      722       +8     
============================================
+ Hits           5155     5261     +106     
- Misses          164      176      +12     
- Partials        138      140       +2

Files Changed	Coverage Δ
.../java/org/tinylog/writers/raw/TcpSocketWriter.java	`60.00% <60.00%> (ø)`
.../java/org/tinylog/writers/raw/UdpSocketWriter.java	`70.00% <70.00%> (ø)`
...rc/main/java/org/tinylog/writers/SyslogWriter.java	`71.42% <71.42%> (ø)`
...n/java/org/tinylog/writers/raw/SyslogSeverity.java	`90.47% <90.47%> (ø)`
.../org/tinylog/writers/raw/AbstractSocketWriter.java	`97.22% <97.22%> (ø)`
...n/java/org/tinylog/writers/raw/SyslogFacility.java	`100.00% <100.00%> (ø)`

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

pmwmedia · 2023-07-30T08:14:38Z

Thank you for your pull request! I will review it soon. However, your changes looks very good at first sight. :)

pmwmedia · 2023-08-04T16:24:30Z

tinylog-impl/src/main/java/org/tinylog/writers/AbstractFileBasedWriter.java

@@ -65,7 +65,7 @@ protected String getFileName() {
 	 *
 	 * @return Configured charset
 	 */
-	protected Charset getCharset() {
+	protected final Charset getCharset() {


I think it is not necessary to make this method final. This could break other writers that are not part of tinylog itself.

pmwmedia · 2023-08-04T16:27:37Z

tinylog-impl/src/main/java/org/tinylog/writers/SyslogWriter.java

+
+		String protocol = getStringValue("protocol");
+		if (protocol == null) {
+			throw new IllegalArgumentException("Missing protocol");


Suggestion (but not a must): What do you think about using the UdpSocketWriter or TcpSocketWriter by default if no protocol is configured instead of throwing an exception?

Updated, used udp protocol as a default.

pmwmedia · 2023-08-04T16:28:39Z

tinylog-impl/src/main/java/org/tinylog/writers/raw/AbstractSocketWriter.java

+	 * @return The formated message.
+	 */
+	public byte[] formatMessage(final LogEntry logEntry) {
+


You can delete this empty line.

pmwmedia · 2023-08-04T16:30:37Z

tinylog-impl/src/main/java/org/tinylog/writers/raw/TcpSocketWriter.java

+	@Override
+	public void write(final LogEntry logEntry) throws IOException {
+		byte[] b = formatMessage(logEntry);
+		socket.getOutputStream().write(b);


Could you use a full name for the variable b like data for example?

pmwmedia · 2023-08-04T16:31:22Z

tinylog-impl/src/main/java/org/tinylog/writers/raw/UdpSocketWriter.java

+
+	@Override
+	public void write(final LogEntry logEntry) throws IOException {
+		byte[] b = formatMessage(logEntry);


Please use here a full name for the variable b, too.

pmwmedia · 2023-08-04T16:32:38Z

tinylog-impl/src/test/java/org/tinylog/util/TcpSyslogServer.java

+import org.tinylog.Level;
+import org.tinylog.provider.InternalLogger;
+
+public class TcpSyslogServer extends Thread {


Please start and end the body of the class with an empty line.

pmwmedia · 2023-08-04T16:33:13Z

tinylog-impl/src/test/java/org/tinylog/util/UdpSyslogServer.java

+import org.tinylog.Level;
+import org.tinylog.provider.InternalLogger;
+
+public class UdpSyslogServer extends Thread {


Please start and end the body of the class with an empty line, too.

pmwmedia · 2023-08-04T16:33:56Z

tinylog-impl/src/test/java/org/tinylog/writers/SyslogWriterTest.java

+	private String getExpectedSyslogMessage(final String message,
+											final SyslogFacility facility,
+											final SyslogSeverity severity,
+											final String identification) {


Please check the indentation here.

pmwmedia · 2023-08-04T16:34:56Z

tinylog-impl/src/test/java/org/tinylog/writers/SyslogWriterTest.java

+		assertThat(server.getLastMessage()).isEqualTo(getExpectedSyslogMessage(TEST_MESSAGE, facility, SyslogSeverity.WARNING, ""));
+		server.shutdown();
+	}
+}


Please add an empty line at the end of the class body.

pmwmedia · 2023-08-04T16:39:25Z

tinylog-impl/src/test/java/org/tinylog/writers/SyslogWriterTest.java

+/**
+ * Tests for {@link SyslogWriter}.
+ */
+public final class SyslogWriterTest {


Two general wishes:

1st: If you split lines, please move the dot from the end of the line to the start of the next line.

Example:

assertThatThrownBy(() -> new SyslogWriter(doubletonMap("protocol", "udp", "facility", "invalid")) .write(LogEntryBuilder.empty().message(TEST_MESSAGE).create())).hasMessageMatching("(?i).*SyslogFacility.*");

2nd: Please add some blank lines in longer test methods to group the logic. This makes it easier to read and unterstand the logic.

pmwmedia · 2023-08-04T16:41:19Z

I'm done with the review. I could find only a few minors. You have written very good and maintainable code :)

pmwmedia · 2023-08-04T16:42:54Z

By the way: Would you be able to add some documentation for the tinylog website?

Link:
https://github.com/tinylog-org/website/edit/v2/content/documentation/configuration.md

pmwmedia

Thank you very much for your changes. I could find only one minor. Can you add the missing empty line? Afterwards, I will merge your pull request.

pmwmedia · 2023-08-17T06:39:35Z

tinylog-impl/src/main/java/org/tinylog/writers/raw/AbstractSocketWriter.java

+		builder.append(": ");
+		builder.append(render(logEntry));
+		return builder.toString().getBytes(charset);
+	}


Please add an empty line between line 127 and 128.

pmwmedia · 2023-08-18T02:47:19Z

Thank you very much for your great pull request! It's really awesome to have a syslog writer in tinylog.

pkarlowicz added 2 commits July 29, 2023 22:23

Add Syslog writer.

313b708

Updated comments.

95e1683

pmwmedia requested changes Aug 4, 2023

View reviewed changes

Addressed review issues.

ac1598f

pmwmedia requested changes Aug 17, 2023

View reviewed changes

Added missing blank line.

109a52f

pmwmedia approved these changes Aug 18, 2023

View reviewed changes

pmwmedia merged commit 1566c7f into tinylog-org:v2.7 Aug 18, 2023

github-actions bot locked as resolved and limited conversation to collaborators Aug 26, 2023

pmwmedia added this to the 2.7 milestone Dec 29, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add syslog writer #507

Add syslog writer #507

pkarlowicz commented Jul 29, 2023 •

edited by pmwmedia

Loading

codecov bot commented Jul 29, 2023 •

edited

Loading

pmwmedia commented Jul 30, 2023

pmwmedia Aug 4, 2023

pkarlowicz Aug 10, 2023

pmwmedia Aug 4, 2023

pkarlowicz Aug 10, 2023

pmwmedia Aug 4, 2023

pkarlowicz Aug 10, 2023

pmwmedia Aug 4, 2023

pkarlowicz Aug 10, 2023

pmwmedia Aug 4, 2023

pkarlowicz Aug 10, 2023

pmwmedia Aug 4, 2023

pmwmedia Aug 4, 2023

pkarlowicz Aug 10, 2023

pmwmedia Aug 4, 2023

pkarlowicz Aug 10, 2023

pmwmedia Aug 4, 2023

pkarlowicz Aug 10, 2023

pmwmedia Aug 4, 2023

pkarlowicz Aug 10, 2023

pmwmedia commented Aug 4, 2023

pmwmedia commented Aug 4, 2023

pmwmedia left a comment

pmwmedia Aug 17, 2023

pkarlowicz Aug 17, 2023

pmwmedia commented Aug 18, 2023

Add syslog writer #507

Add syslog writer #507

Conversation

pkarlowicz commented Jul 29, 2023 • edited by pmwmedia Loading

Description

Definition of Done

Agreements

codecov bot commented Jul 29, 2023 • edited Loading

Codecov Report

pmwmedia commented Jul 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmwmedia commented Aug 4, 2023

pmwmedia commented Aug 4, 2023

pmwmedia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmwmedia commented Aug 18, 2023

pkarlowicz commented Jul 29, 2023 •

edited by pmwmedia

Loading

codecov bot commented Jul 29, 2023 •

edited

Loading