-
-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
Created an additional logger that logs to the systemd journal #9760
Conversation
These failing tests seem unrelated to anything I did ... |
You need to sign of your commits. ;-) See CONTRIBUTING.md |
Added a unit test Signed-off-by: Johannes Ernst <jernst@indiecomputing.com>
Codecov Report
@@ 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
|
@go2sh Done. |
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 |
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? :( |
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 ... |
That php-systemd patch just got merged: systemd/php-systemd#6 |
lib/private/Log/Systemdlog.php
Outdated
public function write(string $app, $message, int $level) { | ||
$journal_level = $this->levels[$level]; | ||
sd_journal_send('PRIORITY='.$journal_level, | ||
'SYSLOG_IDENTIFIER=nextcloud', |
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.
instead of hardcoding nextcloud, it might be better to get the name from ThemingDefaults::getName() or have it configurable as syslog does.
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.
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.
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.
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.
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. |
lib/private/Log/Systemdlog.php
Outdated
* | ||
* @author Johannes Ernst <jernst@indiecomputing.com> | ||
* | ||
* @license AGPL-3.0 |
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.
Would be nice if this is licensed under GNU AGPL version 3 or any later version
- cc @schiessle
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.
Done.
@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 |
Hang on, this was done in a hurry, will fix when I have more time. |
@@ -39,7 +39,7 @@ class Syslog implements IWriter { | |||
]; | |||
|
|||
public function __construct(IConfig $config) { | |||
openlog($config->getSystemValue('syslog_tag', 'ownCloud'), LOG_PID | LOG_CONS, LOG_USER); |
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.
This change should be mentioned in #7827 after merge.
Signed-off-by: Johannes Ernst <jernst@indiecomputing.com>
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 <jernst@indiecomputing.com>
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 <jernst@indiecomputing.com>
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 <jernst@indiecomputing.com>
This should be better. Manually tested to 1) actually log to the journal, 2) |
@jernst This looks good! could you commit the patch in #9760 (comment) to make the tests pass? |
Signed-off-by: Johannes Ernst <jernst@indiecomputing.com>
This integration test failure seems unrelated to this PR? |
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. |
@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 <jernst@indiecomputing.com>
Signed-off-by: Johannes Ernst <jernst@indiecomputing.com>
I put #10048 in place as today is feature freeze. If that runs through, we may merge it today. |
→ continue in #10048 |
Usage: in
config.php
:and all messages sent to the logger will go to the journal, mapped similarly to the
syslog
logger. This requires thephp-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.