Created an additional logger that logs to the systemd journal by jernst · Pull Request #9760 · nextcloud/server

jernst · 2018-06-06T04:21:16Z

Usage: in config.php:

'log_type' => 'systemd'

and all messages sent to the logger will go to the journal, mapped similarly to the syslog logger. This requires the php-systemd extension from here: https://github.com/systemd/php-systemd.

Advantage: a single place where to look what might be wrong with a running system.

jernst · 2018-06-06T18:27:20Z

These failing tests seem unrelated to anything I did ...

go2sh · 2018-06-06T20:14:37Z

You need to sign of your commits. ;-) See CONTRIBUTING.md

Added a unit test Signed-off-by: Johannes Ernst <[email protected]>

codecov · 2018-06-06T20:42:47Z

Codecov Report

Merging #9760 into master will decrease coverage by <.01%.
The diff coverage is 18.75%.

@@             Coverage Diff              @@
##             master    #9760      +/-   ##
============================================
- Coverage     51.92%   51.91%   -0.01%     
- Complexity    25808    25812       +4     
============================================
  Files          1637     1638       +1     
  Lines         95513    95526      +13     
  Branches       1318     1318              
============================================
+ Hits          49593    49595       +2     
- Misses        45920    45931      +11

Impacted Files	Coverage Δ	Complexity Δ
config/config.sample.php	`0% <ø> (ø)`	`0 <0> (ø)`	⬇️
lib/private/Log/Systemdlog.php	`0% <0%> (ø)`	`3 <3> (?)`
lib/private/Log/Syslog.php	`0% <0%> (ø)`	`3 <0> (ø)`	⬇️
lib/private/Log/LogFactory.php	`100% <100%> (ø)`	`12 <0> (+1)`	⬆️
core/Command/Log/Manage.php	`78.02% <50%> (ø)`	`28 <0> (ø)`	⬇️

jernst · 2018-06-06T20:44:37Z

@go2sh Done.

go2sh · 2018-06-07T07:21:13Z

BTW. I like this a lot. :-)

The other build failure is related to the absence of php-systemd in the ci images triggering an undefined function error. Regarding the dependency, someone from the core team should have a look. @MorrisJobke

blizzz · 2018-06-11T21:25:46Z

Works nicely! For NC 14 it should work with PHP 7.0, however it seems not to be the case yet: systemd/php-systemd#5

Otherwise, do you know about (Debian) packages for CI?

P.S.: There's actually a PR to get 7.0 running, but it is without reaction since mid 2016: systemd/php-systemd#6, latest commit from 2014. It seems dead, eh? :(

jernst · 2018-06-11T21:35:47Z

I developed this against PHP 7.2 on Arch / UBOS using this patch: systemd/php-systemd#6 (I guess I should have mentioned that earlier).

Here is how I build the package: https://github.com/uboslinux/ubos-php/tree/master/php-systemd which comes from Arch AUR here: https://aur.archlinux.org/packages/php-systemd/

Sorry, I'm not current on how to build packages for debian, it's been a few years ...

jernst · 2018-06-11T22:36:08Z

That php-systemd patch just got merged: systemd/php-systemd#6

blizzz · 2018-06-12T23:34:40Z

lib/private/Log/Systemdlog.php

+	public function write(string $app, $message, int $level) {
+		$journal_level = $this->levels[$level];
+		sd_journal_send('PRIORITY='.$journal_level,
+				'SYSLOG_IDENTIFIER=nextcloud',


instead of hardcoding nextcloud, it might be better to get the name from ThemingDefaults::getName() or have it configurable as syslog does.

Yea, I was thinking about that, but then stopped myself from overcomplicating matters :-). If this turns out to be a problem in practice, we can simply extend it in the future.

Actually, we can reuse the syslog_tag config option, as it cannot be used in parallel anyway. Extra effort would only to rename it to something more generic, it requires a repair step though. Since you require IConfig in the constructor, but don't use it, it should either go, or just read the setting and store it in a class member. Then you need to just pass it as SYSLOG_IDENTIFIER. Does not really make things more complicated.

Another thing is that we should check whether sd_journal_send is available, to react decently if the module is not loaded. I'd check it in the constructor, too, and throw a HintException if it is not available.

blizzz · 2018-06-12T23:35:43Z

I believe i taught the Debian 8 that we use in the test case to compile that module. I'll give a final and push tomorrow.

MorrisJobke · 2018-06-13T10:10:02Z

lib/private/Log/Systemdlog.php

+ *
+ * @author Johannes Ernst <[email protected]>
+ *
+ * @license AGPL-3.0


Would be nice if this is licensed under GNU AGPL version 3 or any later version - cc @schiessle

blizzz · 2018-06-13T10:18:12Z

@jernst I added php-systemd to the corresponding CI container. To have effect, please apply this patch for CI to work:

diff --git a/.drone.yml b/.drone.yml
index 26a72c9b43..cf9b262abb 100644
--- a/.drone.yml
+++ b/.drone.yml
@@ -53,7 +53,7 @@ pipeline:
       matrix:
         TESTS: syntax-php7.1
   phan:
-    image: nextcloudci/php7.2:php7.2-11
+    image: nextcloudci/php7.2:php7.2-12
     commands:
       - composer install
       - composer require --dev "phan/phan:0.11.1"
@@ -170,7 +170,7 @@ pipeline:
         DB: NODB
         PHP: 7.1
   nodb-php7.2:
-    image: nextcloudci/php7.2:php7.2-11
+    image: nextcloudci/php7.2:php7.2-12
     commands:
       - NOCOVERAGE=true TEST_SELECTION=NODB ./autotest.sh sqlite
     when:
@@ -194,7 +194,7 @@ pipeline:
         DB: sqlite
         PHP: 7.1
   sqlite-php7.2:
-    image: nextcloudci/php7.2:php7.2-11
+    image: nextcloudci/php7.2:php7.2-12
     commands:
       - NOCOVERAGE=true TEST_SELECTION=DB ./autotest.sh sqlite
     when:
@@ -218,7 +218,7 @@ pipeline:
         DB: mysql
         PHP: 7.1
   mysql-php7.2:
-    image: nextcloudci/php7.2:php7.2-11
+    image: nextcloudci/php7.2:php7.2-12
     commands:
       - NOCOVERAGE=true TEST_SELECTION=DB ./autotest.sh mysql
     when:
@@ -292,7 +292,7 @@ pipeline:
         DB: mysqlmb4
         PHP: 7.1
   mysqlmb4-php7.2:
-    image: nextcloudci/php7.2:php7.2-11
+    image: nextcloudci/php7.2:php7.2-12
     commands:
       - NOCOVERAGE=true TEST_SELECTION=DB ./autotest.sh mysqlmb4
     when:

@jernst and may I ask for another favor? in config/sample.config.php update the comment about 'log_type' => 'file' to reflect the other options and also state that the php-systemd module is needed for the systemd type. Also update the doc about 'syslog_tag' if going for it as suggested ^

jernst · 2018-06-13T18:58:44Z

Hang on, this was done in a hurry, will fix when I have more time.

blizzz · 2018-06-13T21:14:28Z

lib/private/Log/Syslog.php

 	];

 	public function __construct(IConfig $config) {
-		openlog($config->getSystemValue('syslog_tag', 'ownCloud'), LOG_PID | LOG_CONS, LOG_USER);


This change should be mentioned in #7827 after merge.

Signed-off-by: Johannes Ernst <[email protected]>

Changed name of default system (not systemd) logger from ownCloud to Nextcloud, to be consistent Fixed license per nextcloud#9760 (comment) Pulled upstream updates Signed-off-by: Johannes Ernst <[email protected]>

jernst · 2018-06-18T20:18:46Z

This should be better. Manually tested to 1) actually log to the journal, 2) syslog_tag works with journal, 3) HintException is thrown if the PHP systemd module is not active. Also signed commit, and call the extension system not php-system (that's the name of the package) in the comments, as the php.ini file needs extension=systemd.so.

blizzz · 2018-06-18T21:15:28Z

@jernst This looks good! could you commit the patch in #9760 (comment) to make the tests pass?

Signed-off-by: Johannes Ernst <[email protected]>

jernst · 2018-06-28T21:01:59Z

This integration test failure seems unrelated to this PR?

blizzz · 2018-06-28T22:08:21Z

It is nothing that i have seen failing before. Locally I also get different results. I restart the tests, if it reoccurs, could you rebase? if it still reoccurs, it could be related… though it appears strange.

blizzz · 2018-06-28T22:32:13Z

@jernst So, locally I have RedisTest succeeding. Perhaps there was a glitch inbetween. Could you rebase to current master and force-push?

Changed name of default system (not systemd) logger from ownCloud to Nextcloud, to be consistent Fixed license per #9760 (comment) Pulled upstream updates Signed-off-by: Johannes Ernst <[email protected]>

Signed-off-by: Johannes Ernst <[email protected]>

blizzz · 2018-06-29T08:42:36Z

I put #10048 in place as today is feature freeze. If that runs through, we may merge it today.

blizzz · 2018-06-29T09:03:57Z

→ continue in #10048

Added a logger for systemd/journald

beeb942

Added a unit test Signed-off-by: Johannes Ernst <[email protected]>

jernst force-pushed the master branch from f2930b9 to beeb942 Compare June 6, 2018 20:42

rullzer requested review from ChristophWurst, MorrisJobke, blizzz, icewind1991 and rullzer June 11, 2018 20:11

rullzer assigned rullzer and unassigned rullzer Jun 11, 2018

rullzer added enhancement 3. to review Waiting for reviews feature: logging labels Jun 11, 2018

rullzer added this to the Nextcloud 14 milestone Jun 11, 2018

blizzz reviewed Jun 12, 2018

View reviewed changes

blizzz mentioned this pull request Jun 13, 2018

add php-systemd module nextcloud/docker-ci#96

Merged

MorrisJobke reviewed Jun 13, 2018

View reviewed changes

jernst added a commit to jernst/nextcloud-server that referenced this pull request Jun 13, 2018

Fix license per nextcloud#9760 (comment)

9d4f2c8

MorrisJobke added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Jun 13, 2018

blizzz reviewed Jun 13, 2018

View reviewed changes

Pull updates from https://github.com/nextcloud/server

ea494fa

Signed-off-by: Johannes Ernst <[email protected]>

Added Systemd.log documentation to config.sample.php

c2baae9

Changed name of default system (not systemd) logger from ownCloud to Nextcloud, to be consistent Fixed license per nextcloud#9760 (comment) Pulled upstream updates Signed-off-by: Johannes Ernst <[email protected]>

jernst force-pushed the master branch from e54ef53 to c2baae9 Compare June 18, 2018 20:13

Applied patch for CI to work per nextcloud#9760 (comment)

f3325cf

Signed-off-by: Johannes Ernst <[email protected]>

blizzz pushed a commit that referenced this pull request Jun 29, 2018

Applied patch for CI to work per #9760 (comment)

32647a4

Signed-off-by: Johannes Ernst <[email protected]>

blizzz mentioned this pull request Jun 29, 2018

Systemd Logger #10048

Merged

blizzz closed this Jun 29, 2018

Uh oh!

Comments

Conversation

jernst commented Jun 6, 2018

Uh oh!

jernst commented Jun 6, 2018

Uh oh!

go2sh commented Jun 6, 2018

Uh oh!

codecov bot commented Jun 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jernst commented Jun 6, 2018

Uh oh!

go2sh commented Jun 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

blizzz commented Jun 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jernst commented Jun 11, 2018

Uh oh!

jernst commented Jun 11, 2018

Uh oh!

blizzz Jun 12, 2018

Choose a reason for hiding this comment

Uh oh!

jernst Jun 12, 2018

Choose a reason for hiding this comment

Uh oh!

blizzz Jun 13, 2018

Choose a reason for hiding this comment

Uh oh!

blizzz commented Jun 12, 2018

Uh oh!

MorrisJobke Jun 13, 2018

Choose a reason for hiding this comment

Uh oh!

jernst Jun 13, 2018

Choose a reason for hiding this comment

Uh oh!

blizzz commented Jun 13, 2018

Uh oh!

jernst commented Jun 13, 2018

Uh oh!

blizzz Jun 13, 2018

Choose a reason for hiding this comment

Uh oh!

jernst commented Jun 18, 2018

Uh oh!

blizzz commented Jun 18, 2018

Uh oh!

jernst commented Jun 28, 2018

Uh oh!

blizzz commented Jun 28, 2018

Uh oh!

blizzz commented Jun 28, 2018

Uh oh!

blizzz commented Jun 29, 2018

Uh oh!

blizzz commented Jun 29, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov bot commented Jun 6, 2018 •

edited

Loading

go2sh commented Jun 7, 2018 •

edited

Loading

blizzz commented Jun 11, 2018 •

edited

Loading