Implemented logging to Graylog server #14847

Faldon · 2019-03-25T21:06:47Z

For an old feature request, I implemented logging as GELF to a Graylog server.

Features

Configurable to use tcp or udp in system config
Auto-chunking udp messages larger than 1024 bytes
Log server and port configurable in host:port notation

Implementation

Added a new class handling writing the logs to the configured server through a socket connection. If the connection fails, it returns silently.
GELF supports chunking of UDP messages up to 128 chunks. So a log message can be approx. 126kB in size or it is silently discarded.
I also added the ability to configure the logging method through the occ log:manage command scope.

Closes #95

lib/private/Log/Graylog.php

Faldon · 2019-03-26T07:38:33Z

Considering the CI check... Shall / must I rebase my branch onto master?

kesselb · 2019-03-26T09:20:29Z

Considering the CI check... Shall / must I rebase my branch onto master?

The autoloaders are not up to date
Please run: bash build/autoloaderchecker.sh
And commit the result

Added the appropriate logging class. Provided the necessary system config examples. Added loading the log class from the LogFactory. Extended the LogFactory unit tests to include the graylog class. Added Graylog test class skeleton. Signed-off-by: Thomas Pulzer <[email protected]>

Changed method of how to connect to remote server for writing logs. Wrote unit tests for chunked udp, unchunked udp and tcp connections. Signed-off-by: Thomas Pulzer <[email protected]>

Fixed wrong byte packing when sending chunked UDP packets. Improved test code and comments. Signed-off-by: Thomas Pulzer <[email protected]>

Signed-off-by: Thomas Pulzer <[email protected]>

…l with occ Signed-off-by: Thomas Pulzer <[email protected]>

Signed-off-by: Thomas Pulzer <[email protected]>

This revealed former incorrect encoding of field 'level' (is nuber, but was encoded as string. Therefore, the test method had to be adjusted as well. Signed-off-by: Thomas Pulzer <[email protected]>

Signed-off-by: Thomas Pulzer <[email protected]>

Faldon · 2019-03-28T08:15:10Z

Rebased onto master to have all the checks passed.

kesselb

Thanks for the great contribution 👍 Please excuse the late code review 😞

kesselb · 2019-07-15T20:01:31Z

lib/private/Log/Graylog.php

@@ -0,0 +1,109 @@
+<?php
+/**
+ *


Suggested change

*

kesselb · 2019-07-15T20:01:41Z

lib/private/Log/Graylog.php

@@ -0,0 +1,109 @@
+<?php


Suggested change

<?php

<?php

declare(strict_types=1);

kesselb · 2019-07-15T20:01:52Z

tests/lib/Log/GraylogTest.php

@@ -0,0 +1,147 @@
+<?php


Suggested change

<?php

<?php

declare(strict_types=1);

kesselb · 2019-07-15T20:01:58Z

tests/lib/Log/GraylogTest.php

@@ -0,0 +1,147 @@
+<?php
+/**
+ *


Suggested change

*

kesselb · 2019-07-15T20:18:31Z

lib/private/Log/Graylog.php

+		$address = $config->getSystemValue('graylog_host', '');
+		if (false !== strpos($address, ':')) {
+			$this->target = explode(':', $address)[0];
+			$this->port = intval(explode(':', $address)[1]);


Suggested change

$this->port = intval(explode(':', $address)[1]);

$this->port = (int) explode(':', $address)[1];

kesselb · 2019-07-15T20:27:34Z

core/Command/Log/Manage.php

+			$host = $this->config->getSystemValue('graylog_host');
+			$output->writeln('Log server: '.$host);
+			$protocol = $this->config->getSystemValue('graylog_proto', 'udp');
+			$output->writeln('Connection protocol: '.$protocol);


Suggested change

$output->writeln('Connection protocol: '.$protocol);

$output->writeln('Connection protocol: ' . $protocol);

kesselb · 2019-07-15T21:00:20Z

core/Command/Log/Manage.php

+	 * @throws \InvalidArgumentException
+	 */
+	protected function isBackendGraylogSet($new=null) {
+		$old = $this->config->getSystemValue('log_type', self::DEFAULT_BACKEND);


I'm not sure about this 🤔 It's not possible to set greylog_host or greylog_proto if the current log_type or the new log_type is not greylog. That makes sense but is very strict. There is no harm if greylog_host or greylog_proto are set with another log_type. I think we need some input from another reviewer.

kesselb · 2019-07-15T21:02:26Z

core/Command/Log/Manage.php

@@ -99,6 +112,20 @@ protected function execute(InputInterface $input, OutputInterface $output) {
 			$toBeSet['logtimezone'] = $timezone;
 		}

+		if ($host = $input->getOption('host')) {
+			array_key_exists('log_type', $toBeSet) ?


I would like to suggest to change this a bit. Its quite hard to read and we have the same logic two times. The isBackendGraylogSet method should accept an array $toBeSet and do the array_key_exists check.

kesselb · 2019-07-15T21:07:38Z

lib/private/Log/Graylog.php

+	public function __construct(IConfig $config) {
+		$this->host = gethostname();
+		$this->protocol = $config->getSystemValue('graylog_proto', 'udp');
+		$address = $config->getSystemValue('graylog_host', '');


Code is fine. I would pick a slightly different approach but everyone has a different code style so this is up to you.

$address = explode(':', $config->getSystemValue('graylog_host')); if (count($address) === 2) { $this->target = $address[0]; $this->port = (int)$address[1]; } else { $this->target = $address[0]; $this->port = 514; }

kesselb · 2019-07-15T21:09:44Z

core/Command/Log/Manage.php

+	 * @param string $new
+	 * @throws \InvalidArgumentException
+	 */
+	protected function isBackendGraylogSet($new=null) {


We should find another name for it.

MorrisJobke · 2019-07-17T09:45:57Z

lib/private/Log/Graylog.php

+		$msg = json_encode([
+			'version' => self::$VERSION,
+			'host' => $this->host,
+			'short_message' => '{'.$app.'} '.$message,


You might want to use the new way here as well:

Suggested change

'short_message' => '{'.$app.'} '.$message,

'short_message' => $this->logDetailsAsJSON($app, $message, $level),

Was added just now via #16416

maybe the non-encoded one is better here: logDetails(), because it does not return a string with JSON, but an object with keys.

cc @juliushaertl

Yes, if Graylog can handle nested data in the short_message key that should be the way to go.

rullzer · 2019-08-08T07:56:45Z

Lets merge this early in 18.
@juliushaertl could you help out since you touched the logging code recently

ChristophWurst · 2019-12-13T11:02:30Z

Lot's of unaddressed review comments. Some unanswered questions. This needs triage. Not assigning to 19 because pushing this from release to release has no point.

ChristophWurst · 2019-12-13T11:04:49Z

What might make more sense IMO is to use some existing packages like https://packagist.org/packages/giacomofurlan/php-graylog-gelf in combination with #18200. And preferably make this an app.

Like

Use PSR for the logger interface
Make the logger implementation pluggable
Create an implementation for Graylog as an app

ChristophWurst · 2020-07-02T18:35:02Z

Let's close this due to lack of activity. If anyone would like to bring this feature to life, please do so as an app as described in my previous comment.

Faldon added the 3. to review Waiting for reviews label Mar 25, 2019

kesselb reviewed Mar 25, 2019

View reviewed changes

lib/private/Log/Graylog.php Outdated Show resolved Hide resolved

rullzer added this to the Nextcloud 17 milestone Mar 26, 2019

Faldon added 8 commits March 28, 2019 08:38

Reformatted and rearranged some code parts.

d9c25b8

Changed method of how to connect to remote server for writing logs. Wrote unit tests for chunked udp, unchunked udp and tcp connections. Signed-off-by: Thomas Pulzer <[email protected]>

Renamed config key for Graylog protocol to use.

38caffe

Fixed wrong byte packing when sending chunked UDP packets. Improved test code and comments. Signed-off-by: Thomas Pulzer <[email protected]>

Added log backend graylog to occ log:manage command.

b80ddd0

Signed-off-by: Thomas Pulzer <[email protected]>

Added ability to configure graylog remote host and connection protoco…

f98ebee

…l with occ Signed-off-by: Thomas Pulzer <[email protected]>

Added test of the log:manage method if graylog backend is set.

e675d3a

Signed-off-by: Thomas Pulzer <[email protected]>

Dropped constructing the GELF message manually in favour for json_encode

06aadc4

This revealed former incorrect encoding of field 'level' (is nuber, but was encoded as string. Therefore, the test method had to be adjusted as well. Signed-off-by: Thomas Pulzer <[email protected]>

Updated autoloaders.

c5a4be7

Signed-off-by: Thomas Pulzer <[email protected]>

Faldon force-pushed the add_logtype_GELF branch from e8d007a to c5a4be7 Compare March 28, 2019 07:51

MorrisJobke mentioned this pull request Jul 15, 2019

17.0.0 Beta 1 #16408

Merged

28 tasks

kesselb added the enhancement label Jul 15, 2019

kesselb reviewed Jul 15, 2019

View reviewed changes

kesselb requested review from rullzer, nickvergessen and ChristophWurst July 15, 2019 21:12

MorrisJobke reviewed Jul 17, 2019

View reviewed changes

rullzer modified the milestones: Nextcloud 17, Nextcloud 18 Aug 8, 2019

juliusknorr self-assigned this Aug 8, 2019

This was referenced Dec 11, 2019

18 Beta 2 #18343

Merged

18 beta 3 #18370

Merged

ChristophWurst removed the 3. to review Waiting for reviews label Dec 13, 2019

ChristophWurst added the 0. Needs triage Pending check for reproducibility or if it fits our roadmap label Dec 13, 2019

ChristophWurst removed this from the Nextcloud 18 milestone Dec 13, 2019

ChristophWurst closed this Jul 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implemented logging to Graylog server #14847

Implemented logging to Graylog server #14847

Faldon commented Mar 25, 2019

Faldon commented Mar 26, 2019

kesselb commented Mar 26, 2019

Faldon commented Mar 28, 2019

kesselb left a comment

kesselb Jul 15, 2019

kesselb Jul 15, 2019

kesselb Jul 15, 2019

kesselb Jul 15, 2019

kesselb Jul 15, 2019

kesselb Jul 15, 2019

kesselb Jul 15, 2019

kesselb Jul 15, 2019

kesselb Jul 15, 2019

kesselb Jul 15, 2019

MorrisJobke Jul 17, 2019

MorrisJobke Jul 17, 2019

MorrisJobke Jul 17, 2019

juliusknorr Jul 17, 2019

rullzer commented Aug 8, 2019

ChristophWurst commented Dec 13, 2019

ChristophWurst commented Dec 13, 2019

ChristophWurst commented Jul 2, 2020

@@ @@ -0,0 +1,109 @@ @@
+              <?php
+              /**
+               *

@@ @@ -0,0 +1,147 @@ @@
+              <?php
+              /**
+               *

	$this->port = intval(explode(':', $address)[1]);
	$this->port = (int) explode(':', $address)[1];

	$output->writeln('Connection protocol: '.$protocol);
	$output->writeln('Connection protocol: ' . $protocol);

	'short_message' => '{'.$app.'} '.$message,
	'short_message' => $this->logDetailsAsJSON($app, $message, $level),

Implemented logging to Graylog server #14847

Implemented logging to Graylog server #14847

Conversation

Faldon commented Mar 25, 2019

For an old feature request, I implemented logging as GELF to a Graylog server.

Features

Implementation

Faldon commented Mar 26, 2019

kesselb commented Mar 26, 2019

Faldon commented Mar 28, 2019

kesselb left a 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

rullzer commented Aug 8, 2019

ChristophWurst commented Dec 13, 2019

ChristophWurst commented Dec 13, 2019

ChristophWurst commented Jul 2, 2020