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

Separate audit logs from server logs. #27443

Merged
merged 1 commit into from Mar 24, 2017

Conversation

@sharidas
Member

sharidas commented Mar 21, 2017

This change will help users to separate logs
for each apps based on conditional logging.

Signed-off-by: Sujith H sharidasan@owncloud.com

Description

This change will help users to separate the audit logs from the server log. So lets say for each apps we can have conditional logs having separate log files.

Related Issue

Motivation and Context

This will help users to have separate logs for each apps. Instead of searching the entire server log, it would be better to separate the logs.

How Has This Been Tested?

Made the modifications in the files_texteditor and gallary app as follows:

https://paste.fedoraproject.org/paste/MF7RzFfirSFFfKYciC1UYl5M1UNdIGYhyRLivL9gydE=
Modify the config.php as follows:

  'log.condition' => [
    [
      'users' => ['user1'],
      'apps' => ['files_texteditor'],
      'logfile' => '/tmp/test.log'
    ],
    [
      'users' => ['user1'],
      'apps' => ['gallery'],
      'logfile' => '/tmp/gallery.log'
    ],
  ], 

After this try to open a text file in the files app. User should be able to see the logs written in /tmp/test.log
Similarly try to open the gallery with photos. User should be able to see the logs written in /tmp/gallery.log

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
@mention-bot

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot Mar 21, 2017

@sharidas, thanks for your PR! By analyzing the history of the files in this pull request, we identified @DeepDiver1975, @bartv2 and @oparoz to be potential reviewers.

mention-bot commented Mar 21, 2017

@sharidas, thanks for your PR! By analyzing the history of the files in this pull request, we identified @DeepDiver1975, @bartv2 and @oparoz to be potential reviewers.

@mmattel

This comment has been minimized.

Show comment
Hide comment
@mmattel

mmattel Mar 21, 2017

Contributor

Pls do also a documentation in config.sample.php
Q: is this for owncloud log only or does that apply to the syslog method too?

Contributor

mmattel commented Mar 21, 2017

Pls do also a documentation in config.sample.php
Q: is this for owncloud log only or does that apply to the syslog method too?

@DeepDiver1975 DeepDiver1975 added this to the 10.0 milestone Mar 21, 2017

@DeepDiver1975

This comment has been minimized.

Show comment
Hide comment
@DeepDiver1975

DeepDiver1975 Mar 21, 2017

Member
Stacktrace

Test\LoggerTest::testAppCondition
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
-    0 => '1 Show info messages of files app'
-    1 => '2 Show warning messages of other apps'
+    0 => '2 Show warning messages of other apps'
 )
Member

DeepDiver1975 commented Mar 21, 2017

Stacktrace

Test\LoggerTest::testAppCondition
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
-    0 => '1 Show info messages of files app'
-    1 => '2 Show warning messages of other apps'
+    0 => '2 Show warning messages of other apps'
 )
@sharidas

This comment has been minimized.

Show comment
Hide comment
@sharidas

sharidas Mar 21, 2017

Member

@mmattel Sure I will update the config.sample.php too. This patch tries to separate the logs for apps separate from owncloud log.

Member

sharidas commented Mar 21, 2017

@mmattel Sure I will update the config.sample.php too. This patch tries to separate the logs for apps separate from owncloud log.

@jvillafanez

This comment has been minimized.

Show comment
Hide comment
@jvillafanez

jvillafanez Mar 21, 2017

Member

🚫 Personally, I'm not willing to merge this piece of code. I think there are better and cleaner alternatives to achive the same objective.

  • We can't guarantee that the apps will follow the approach we want. This means that a lot of apps won't work with these changes (the app will work but it won't log the way we want)
  • I expect a huge logCondition if we want all the apps to log in different files. It doesn't seem a maintenable approach even with vanilla installations.
  • There is a mix in the responsabilities: either the app decides what to log and under what conditions (leaving out core for this), or it's core the one filtering the logs however it wants.
  • Apps shouldn't care at all about where they're logging.

First, I'd add support to use several log handlers at the same time. Then we can add a new log handler (AppBasedLog, for example) that would write any log entry to the corresponding file based on the app. Additional optional configuration could be added to the new handler: directory location for the logs, user filters, etc.

I'd keep this on hold for now.

Member

jvillafanez commented Mar 21, 2017

🚫 Personally, I'm not willing to merge this piece of code. I think there are better and cleaner alternatives to achive the same objective.

  • We can't guarantee that the apps will follow the approach we want. This means that a lot of apps won't work with these changes (the app will work but it won't log the way we want)
  • I expect a huge logCondition if we want all the apps to log in different files. It doesn't seem a maintenable approach even with vanilla installations.
  • There is a mix in the responsabilities: either the app decides what to log and under what conditions (leaving out core for this), or it's core the one filtering the logs however it wants.
  • Apps shouldn't care at all about where they're logging.

First, I'd add support to use several log handlers at the same time. Then we can add a new log handler (AppBasedLog, for example) that would write any log entry to the corresponding file based on the app. Additional optional configuration could be added to the new handler: directory location for the logs, user filters, etc.

I'd keep this on hold for now.

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Mar 21, 2017

Member

@jvillafanez while I agree that your proposed approach is significantly better, it will require rewriting / rewiring pieces of apps to make sure they are using the correct app-specific logger. And if apps can still use the same wrong logger we're back in a similar situation like we are now where apps need to specific for every message to which log it needs to go. (the difference in your approach being that there is only a single location where the app should pay attention which logger they get / use per DI while the current situation is per log entry).

I expect a huge logCondition if we want all the apps to log in different files. It doesn't seem a maintenable approach even with vanilla installations.

This is true. Might be possible to generate the config using a script but it's inelegant.

@jvillafanez do you have an estimate how much time it would take to go the proper route you proposed (and a quick plan) to compare with this PR's quick, inelegant but working approach.

Member

PVince81 commented Mar 21, 2017

@jvillafanez while I agree that your proposed approach is significantly better, it will require rewriting / rewiring pieces of apps to make sure they are using the correct app-specific logger. And if apps can still use the same wrong logger we're back in a similar situation like we are now where apps need to specific for every message to which log it needs to go. (the difference in your approach being that there is only a single location where the app should pay attention which logger they get / use per DI while the current situation is per log entry).

I expect a huge logCondition if we want all the apps to log in different files. It doesn't seem a maintenable approach even with vanilla installations.

This is true. Might be possible to generate the config using a script but it's inelegant.

@jvillafanez do you have an estimate how much time it would take to go the proper route you proposed (and a quick plan) to compare with this PR's quick, inelegant but working approach.

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Mar 21, 2017

Member

Would also be good to hear from @butonic about #27443 (comment) as he's the one who proposed more of the logging flexibility stuff.

cc @pmaier1

Member

PVince81 commented Mar 21, 2017

Would also be good to hear from @butonic about #27443 (comment) as he's the one who proposed more of the logging flexibility stuff.

cc @pmaier1

@butonic

This comment has been minimized.

Show comment
Hide comment
@butonic

butonic Mar 21, 2017

Member

@jvillafanez I don't fully understand:

We can't guarantee that the apps will follow the approach we want. This means that a lot of apps won't work with these changes (the app will work but it won't log the way we want)

Apps should follow our current logging interface and use the context parameter to set a value for 'app'. The current logging uses that and even logs 'no app in context' if it is forgotten. Without configuring any log.condition nothing changes from what is currently happening. everything lands in data/owncloud.log, which is what you want anyway if you are using a log aggregator like splunk or elk.

I expect a huge logCondition if we want all the apps to log in different files. It doesn't seem a maintenable approach even with vanilla installations.

Why would we want that? I think the most prominent use case is to log certain apps to a dedicated logfile, eg user_ldap or wnd because they add a lot of noise. Personally, I think splitting is bad because you might loose information if not all logs are sent to you. In any case the log.condition can be put in a separate logging.config.php

There is a mix in the responsabilities: either the app decides what to log and under what conditions (leaving out core for this), or it's core the one filtering the logs however it wants.

Coming from Java I personally like what most logging frameworks do there. Set a global log level plus set dedicated log levels per package and class. That is pretty flexible and allows to turn of logs that you are not interested it. It would be cool if you could split the log into two files, one with the stuff you think is relevant (the filtered log) and one with the full debug log ... but then again tail and grep are your friends.

Apps shouldn't care at all about where they're logging.
totally agree ... but I don't see why this PR is a problem for this?

IMO the bigger problem is that we sometimes concatenate a lot of strings that are then not logged. I'd like to use a closure to only evaluate the code when needed. but the PSR does not seem to allow that. We could probably add a closure to the context for complicated stuff and try to encourage developers to use only plain strings in the log message.

Member

butonic commented Mar 21, 2017

@jvillafanez I don't fully understand:

We can't guarantee that the apps will follow the approach we want. This means that a lot of apps won't work with these changes (the app will work but it won't log the way we want)

Apps should follow our current logging interface and use the context parameter to set a value for 'app'. The current logging uses that and even logs 'no app in context' if it is forgotten. Without configuring any log.condition nothing changes from what is currently happening. everything lands in data/owncloud.log, which is what you want anyway if you are using a log aggregator like splunk or elk.

I expect a huge logCondition if we want all the apps to log in different files. It doesn't seem a maintenable approach even with vanilla installations.

Why would we want that? I think the most prominent use case is to log certain apps to a dedicated logfile, eg user_ldap or wnd because they add a lot of noise. Personally, I think splitting is bad because you might loose information if not all logs are sent to you. In any case the log.condition can be put in a separate logging.config.php

There is a mix in the responsabilities: either the app decides what to log and under what conditions (leaving out core for this), or it's core the one filtering the logs however it wants.

Coming from Java I personally like what most logging frameworks do there. Set a global log level plus set dedicated log levels per package and class. That is pretty flexible and allows to turn of logs that you are not interested it. It would be cool if you could split the log into two files, one with the stuff you think is relevant (the filtered log) and one with the full debug log ... but then again tail and grep are your friends.

Apps shouldn't care at all about where they're logging.
totally agree ... but I don't see why this PR is a problem for this?

IMO the bigger problem is that we sometimes concatenate a lot of strings that are then not logged. I'd like to use a closure to only evaluate the code when needed. but the PSR does not seem to allow that. We could probably add a closure to the context for complicated stuff and try to encourage developers to use only plain strings in the log message.

@jvillafanez

This comment has been minimized.

Show comment
Hide comment
@jvillafanez

jvillafanez Mar 21, 2017

Member

We can't guarantee that the apps will follow the approach we want. This means that a lot of apps won't work with these changes (the app will work but it won't log the way we want)

Apps should follow our current logging interface and use the context parameter to set a value for 'app'. The current logging uses that and even logs 'no app in context' if it is forgotten. Without configuring any log.condition nothing changes from what is currently happening. everything lands in data/owncloud.log, which is what you want anyway if you are using a log aggregator like splunk or elk.

I got confused. I read "modifications in apps" and said "this is bad". If apps don't need any change to be able to use these changes, that's fine.

I expect a huge logCondition if we want all the apps to log in different files. It doesn't seem a maintenable approach even with vanilla installations.

Why would we want that? I think the most prominent use case is to log certain apps to a dedicated logfile, eg user_ldap or wnd because they add a lot of noise. Personally, I think splitting is bad because you might loose information if not all logs are sent to you. In any case the log.condition can be put in a separate logging.config.php

I'm sure there will be people who will split the files just because it's possible. I think it's fine as long as we also have the full log: for small things or first contact, it might be useful to check small files based on each component or just for specific components.

My main point is that we should allow admins to configure several log handlers. Right now they have the possiblity to use syslog or the ownCloud log, only one of them. By letting the admin choose several of them we can easily extend the logging capabilities.
The secondary point is to create a new handler to apply these changes. This handler will take care of filtering and creating the required log files if needed.

The advantages:

  • more flexibility for the admin to choose the log handlers he wants to use. This could include other file formats at some point. On the short term, the expected handlers would be the current ownCloud log and the new one with some filters. This way we can have the full log AND the filtered log.
  • less risk changing code. By adding a new handler the risk of a regression is greatly reduced because the old code won't be changed. If something breaks it's possible to fallback to the old one.

Coming from Java I personally like what most logging frameworks do there. Set a global log level plus set dedicated log levels per package and class. That is pretty flexible and allows to turn of logs that you are not interested it

That could be another log handler with those capabilities.

Member

jvillafanez commented Mar 21, 2017

We can't guarantee that the apps will follow the approach we want. This means that a lot of apps won't work with these changes (the app will work but it won't log the way we want)

Apps should follow our current logging interface and use the context parameter to set a value for 'app'. The current logging uses that and even logs 'no app in context' if it is forgotten. Without configuring any log.condition nothing changes from what is currently happening. everything lands in data/owncloud.log, which is what you want anyway if you are using a log aggregator like splunk or elk.

I got confused. I read "modifications in apps" and said "this is bad". If apps don't need any change to be able to use these changes, that's fine.

I expect a huge logCondition if we want all the apps to log in different files. It doesn't seem a maintenable approach even with vanilla installations.

Why would we want that? I think the most prominent use case is to log certain apps to a dedicated logfile, eg user_ldap or wnd because they add a lot of noise. Personally, I think splitting is bad because you might loose information if not all logs are sent to you. In any case the log.condition can be put in a separate logging.config.php

I'm sure there will be people who will split the files just because it's possible. I think it's fine as long as we also have the full log: for small things or first contact, it might be useful to check small files based on each component or just for specific components.

My main point is that we should allow admins to configure several log handlers. Right now they have the possiblity to use syslog or the ownCloud log, only one of them. By letting the admin choose several of them we can easily extend the logging capabilities.
The secondary point is to create a new handler to apply these changes. This handler will take care of filtering and creating the required log files if needed.

The advantages:

  • more flexibility for the admin to choose the log handlers he wants to use. This could include other file formats at some point. On the short term, the expected handlers would be the current ownCloud log and the new one with some filters. This way we can have the full log AND the filtered log.
  • less risk changing code. By adding a new handler the risk of a regression is greatly reduced because the old code won't be changed. If something breaks it's possible to fallback to the old one.

Coming from Java I personally like what most logging frameworks do there. Set a global log level plus set dedicated log levels per package and class. That is pretty flexible and allows to turn of logs that you are not interested it

That could be another log handler with those capabilities.

@jvillafanez

This comment has been minimized.

Show comment
Hide comment
@jvillafanez

jvillafanez Mar 21, 2017

Member

@jvillafanez do you have an estimate how much time it would take to go the proper route you proposed (and a quick plan) to compare with this PR's quick, inelegant but working approach.

I'd say 1-2 days to support multiple handlers (it might take a bit more time because we might need to verify that the whole code uses the logger configured by ownCloud) and another 1-2 to create the new handler with the changes (it should be mostly a copy & paste of the current code). I'm not sure if it will be possible to have unittest (1 day for some basic tests?), and maybe 1 day for QA.
5 days should be enough to have something working properly.

Member

jvillafanez commented Mar 21, 2017

@jvillafanez do you have an estimate how much time it would take to go the proper route you proposed (and a quick plan) to compare with this PR's quick, inelegant but working approach.

I'd say 1-2 days to support multiple handlers (it might take a bit more time because we might need to verify that the whole code uses the logger configured by ownCloud) and another 1-2 to create the new handler with the changes (it should be mostly a copy & paste of the current code). I'm not sure if it will be possible to have unittest (1 day for some basic tests?), and maybe 1 day for QA.
5 days should be enough to have something working properly.

Show outdated Hide outdated config/config.sample.php
@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Mar 21, 2017

Member

@pmaier1 see #27443 (comment)

As we're running out of time I suggest keeping this PR for the short term (it's working and close to completion) and considering @jvillafanez's proposal for later. And for that also have a proper design session.

The current approach in this PR is an extension of something that already exists, so a quick addition.

Member

PVince81 commented Mar 21, 2017

@pmaier1 see #27443 (comment)

As we're running out of time I suggest keeping this PR for the short term (it's working and close to completion) and considering @jvillafanez's proposal for later. And for that also have a proper design session.

The current approach in this PR is an extension of something that already exists, so a quick addition.

$handle = @fopen($conditionalLogFile, 'a');
@chmod($conditionalLogFile, 0640);
} else {
$handle = @fopen(self::$logFile, 'a');

This comment has been minimized.

@PVince81

PVince81 Mar 21, 2017

Member

wow, did we really do a fopen on the log file for every single entry ?

@PVince81

PVince81 Mar 21, 2017

Member

wow, did we really do a fopen on the log file for every single entry ?

This comment has been minimized.

@sharidas

sharidas Mar 22, 2017

Member

Yah.

@sharidas

This comment has been minimized.

@butonic

butonic Mar 22, 2017

Member

hm what would happen if we keep the handle open until the request ends? I can only guess how that behaves when multiple processes write log messages. hmmm monolog also uses the file handle for the whole request (unless you call close()): https://github.com/Seldaek/monolog/blob/master/src/Monolog/Handler/StreamHandler.php#L95-L111 It seems the underlying fwrite implementation is thread safe: http://stackoverflow.com/a/2220574

so we should be able to optimize our log writing ... @mrow4a ?

@butonic

butonic Mar 22, 2017

Member

hm what would happen if we keep the handle open until the request ends? I can only guess how that behaves when multiple processes write log messages. hmmm monolog also uses the file handle for the whole request (unless you call close()): https://github.com/Seldaek/monolog/blob/master/src/Monolog/Handler/StreamHandler.php#L95-L111 It seems the underlying fwrite implementation is thread safe: http://stackoverflow.com/a/2220574

so we should be able to optimize our log writing ... @mrow4a ?

This comment has been minimized.

@jvillafanez

jvillafanez Mar 22, 2017

Member

quoting official php documentation:

(http://php.net/manual/en/function.fwrite.php)

If handle was fopen()ed in append mode, fwrite()s are atomic (unless the size of string exceeds the filesystem's block size, on some platforms, and as long as the file is on a local filesystem). That is, there is no need to flock() a resource before calling fwrite(); all of the data will be written without interruption.

(http://php.net/manual/en/function.fseek.php)

If you have opened the file in append (a or a+) mode, any data you write to the file will always be appended, regardless of the file position, and the result of calling fseek() will be undefined.

I'd keep the file opened and lock the file when writing (we can't guarantee the log entry to have less than 1k). In addition I've read somewhere the writes over NFS aren't atomic, so that another reason to lock the file.

@jvillafanez

jvillafanez Mar 22, 2017

Member

quoting official php documentation:

(http://php.net/manual/en/function.fwrite.php)

If handle was fopen()ed in append mode, fwrite()s are atomic (unless the size of string exceeds the filesystem's block size, on some platforms, and as long as the file is on a local filesystem). That is, there is no need to flock() a resource before calling fwrite(); all of the data will be written without interruption.

(http://php.net/manual/en/function.fseek.php)

If you have opened the file in append (a or a+) mode, any data you write to the file will always be appended, regardless of the file position, and the result of calling fseek() will be undefined.

I'd keep the file opened and lock the file when writing (we can't guarantee the log entry to have less than 1k). In addition I've read somewhere the writes over NFS aren't atomic, so that another reason to lock the file.

This comment has been minimized.

@butonic

butonic Mar 22, 2017

Member

wouldn't that block other requests from writing log messages until the lock is released?

@butonic

butonic Mar 22, 2017

Member

wouldn't that block other requests from writing log messages until the lock is released?

This comment has been minimized.

@jvillafanez

jvillafanez Mar 22, 2017

Member

yes, but hopefully it won't be dramatic. The flow would be: open file to append and when we want to write a log entry, lock the file for writing - write - unlock. There is no need to open and close the file over and over.

We could add an option to use locks or not over the log file if there is a noticeable performance impact. I expect some overhead, but hopefully not much.

@jvillafanez

jvillafanez Mar 22, 2017

Member

yes, but hopefully it won't be dramatic. The flow would be: open file to append and when we want to write a log entry, lock the file for writing - write - unlock. There is no need to open and close the file over and over.

We could add an option to use locks or not over the log file if there is a noticeable performance impact. I expect some overhead, but hopefully not much.

This comment has been minimized.

@butonic

butonic Mar 22, 2017

Member

Hm I fear locking will kill performance. What does google say? The log4php guys have a benchmark ... it is from 2009, but we could rerun it on todays php to see the numbers: https://www.grobmeier.de/performance-ofnonblocking-write-to-files-via-php-21082009.html

running 4 scripts in parralel in the browser with 100000 loops in my vm with php 5.6 on debian produces:

Execution with NOT closing the log file took 5.9572031497955 seconds
Execution with CLOSING the log after each write file took 17.085966825485 seconds
Execution with file_put_content took 16.009223937988 seconds
Execution with leaving the file open, but LOCKING and UNLOCKING it took 5.5897269248962 seconds
Execution with nonblocking stream took 6.125638961792 seconds
Execution with the error_log method took 14.576776981354 seconds

so I think locking is the way to go ... in a different pr.

@sharidas sorry for highjacking ..

@butonic

butonic Mar 22, 2017

Member

Hm I fear locking will kill performance. What does google say? The log4php guys have a benchmark ... it is from 2009, but we could rerun it on todays php to see the numbers: https://www.grobmeier.de/performance-ofnonblocking-write-to-files-via-php-21082009.html

running 4 scripts in parralel in the browser with 100000 loops in my vm with php 5.6 on debian produces:

Execution with NOT closing the log file took 5.9572031497955 seconds
Execution with CLOSING the log after each write file took 17.085966825485 seconds
Execution with file_put_content took 16.009223937988 seconds
Execution with leaving the file open, but LOCKING and UNLOCKING it took 5.5897269248962 seconds
Execution with nonblocking stream took 6.125638961792 seconds
Execution with the error_log method took 14.576776981354 seconds

so I think locking is the way to go ... in a different pr.

@sharidas sorry for highjacking ..

This comment has been minimized.

@jvillafanez

jvillafanez Mar 22, 2017

Member

so I think locking is the way to go ... in a different pr.

Yes, it's outside of scope for this PR. No need to change it right now.

@jvillafanez

jvillafanez Mar 22, 2017

Member

so I think locking is the way to go ... in a different pr.

Yes, it's outside of scope for this PR. No need to change it right now.

@@ -297,7 +307,7 @@ public function log($level, $message, array $context = []) {
if ($level >= $minLevel) {
$logger = $this->logger;
call_user_func([$logger, 'write'], $app, $message, $level);
call_user_func([$logger, 'write'], $app, $message, $level, $logConditionFile);

This comment has been minimized.

@jvillafanez

jvillafanez Mar 22, 2017

Member

Does it make sense that the "global" logger (aka, this class) tells the specific loggers where they have to write? Is it useful for any other logger (syslog, for example)?

@jvillafanez

jvillafanez Mar 22, 2017

Member

Does it make sense that the "global" logger (aka, this class) tells the specific loggers where they have to write? Is it useful for any other logger (syslog, for example)?

This comment has been minimized.

@mmattel

mmattel Mar 22, 2017

Contributor

impact on syslog - this was one of my questions above: #27443 (comment)
Not to complain if so, but necessay to know and document.

@mmattel

mmattel Mar 22, 2017

Contributor

impact on syslog - this was one of my questions above: #27443 (comment)
Not to complain if so, but necessay to know and document.

This comment has been minimized.

@butonic

butonic Mar 22, 2017

Member

AFAICT the syslog logger will ignore the logfile condition and always log to syslog.

@butonic

butonic Mar 22, 2017

Member

AFAICT the syslog logger will ignore the logfile condition and always log to syslog.

This comment has been minimized.

@mmattel

mmattel Mar 22, 2017

Contributor

Raises the question: shall this be enabled for syslog too, would that make sense?

In any case it needs a note in config.sample.php and/or the documentation.
aka: Not applicapable when using syslog, or This also splits logs when using syslog

@mmattel

mmattel Mar 22, 2017

Contributor

Raises the question: shall this be enabled for syslog too, would that make sense?

In any case it needs a note in config.sample.php and/or the documentation.
aka: Not applicapable when using syslog, or This also splits logs when using syslog

This comment has been minimized.

@mmattel

mmattel Mar 22, 2017

Contributor

@butonic can you check if logging to syslog will ignore logfile conditions?
This is important regarding proper documentation.

@mmattel

mmattel Mar 22, 2017

Contributor

@butonic can you check if logging to syslog will ignore logfile conditions?
This is important regarding proper documentation.

This comment has been minimized.

@jvillafanez

jvillafanez Mar 22, 2017

Member

I'd move the checks for the different logs files directly into the ownCloud log and keep the same "interface" for all the loggers. Basically, the ownCloud logger would read the log.conditions configuration and initialize itself properly (if possible).

@jvillafanez

jvillafanez Mar 22, 2017

Member

I'd move the checks for the different logs files directly into the ownCloud log and keep the same "interface" for all the loggers. Basically, the ownCloud logger would read the log.conditions configuration and initialize itself properly (if possible).

Show outdated Hide outdated lib/private/Log/Owncloud.php
@pmaier1

This comment has been minimized.

Show comment
Hide comment
@pmaier1

pmaier1 Mar 22, 2017

Contributor

@pmaier1 see #27443 (comment)
As we're running out of time I suggest keeping this PR for the short term (it's working and close to completion) and considering @jvillafanez's proposal for later. And for that also have a proper design session.
The current approach in this PR is an extension of something that already exists, so a quick addition.

Yes, fine. Nevertheless we should not forget about the better approach. I will link the comment in the platform ticket.

Contributor

pmaier1 commented Mar 22, 2017

@pmaier1 see #27443 (comment)
As we're running out of time I suggest keeping this PR for the short term (it's working and close to completion) and considering @jvillafanez's proposal for later. And for that also have a proper design session.
The current approach in this PR is an extension of something that already exists, so a quick addition.

Yes, fine. Nevertheless we should not forget about the better approach. I will link the comment in the platform ticket.

@mmattel

This comment has been minimized.

Show comment
Hide comment
@mmattel

mmattel Mar 22, 2017

Contributor

Please also fix config.sample.php:

  • according the code, there is no more shared_secret parameter
  • describe what the new logfile parameter is used for
  • Based on @butonic response, see above: add: Not applicapable when using syslog
Contributor

mmattel commented Mar 22, 2017

Please also fix config.sample.php:

  • according the code, there is no more shared_secret parameter
  • describe what the new logfile parameter is used for
  • Based on @butonic response, see above: add: Not applicapable when using syslog
@sharidas

This comment has been minimized.

Show comment
Hide comment
@sharidas

sharidas Mar 23, 2017

Member

@mmattel By mistake the shared_secret parameter was deleted. I have tried to add the same in the updated PR. I have tried to provide a brief description of the logfile parameter used in the config. I have added as a Note in the logfile sections saying: Not applicapable when using syslog Let me know the new PR looks ok or requires changes.

Thanks all for the feedback.

Member

sharidas commented Mar 23, 2017

@mmattel By mistake the shared_secret parameter was deleted. I have tried to add the same in the updated PR. I have tried to provide a brief description of the logfile parameter used in the config. I have added as a Note in the logfile sections saying: Not applicapable when using syslog Let me know the new PR looks ok or requires changes.

Thanks all for the feedback.

@mmattel

This comment has been minimized.

Show comment
Hide comment
@mmattel

mmattel Mar 23, 2017

Contributor

Not tested, looks good from my POV 👍

Contributor

mmattel commented Mar 23, 2017

Not tested, looks good from my POV 👍

Show outdated Hide outdated lib/private/Log.php
Show outdated Hide outdated lib/private/Log.php
Show outdated Hide outdated lib/private/Log.php
Show outdated Hide outdated lib/private/Log/Owncloud.php
Show outdated Hide outdated lib/private/Log.php
Show outdated Hide outdated config/config.sample.php
@sharidas

This comment has been minimized.

Show comment
Hide comment
@sharidas

sharidas Mar 23, 2017

Member

@butonic Thanks for the review comments. I have updated the PR with the changes requested. I have added the breaks mentioned. I have also replaced the isset with !empty. As per the oc coding style i have provided space in the left and right of the =. Also added missing * in the config.sample.php.

Member

sharidas commented Mar 23, 2017

@butonic Thanks for the review comments. I have updated the PR with the changes requested. I have added the breaks mentioned. I have also replaced the isset with !empty. As per the oc coding style i have provided space in the left and right of the =. Also added missing * in the config.sample.php.

@butonic

This comment has been minimized.

Show comment
Hide comment
@butonic

butonic Mar 23, 2017

Member

@shadone cool, thx. Now Jenkins still complains as reported earlier:

Test\LoggerTest::testAppCondition
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
-    0 => '1 Show info messages of files app'
-    1 => '2 Show warning messages of other apps'
+    0 => '2 Show warning messages of other apps'
 )

/var/lib/jenkins/workspace/owncloud-core_core_PR-27443-VFE2WOKMDT4YDIDOZ3FLLTMZCW3UDIIKOVVAQZY6LBKX3ESO43XA/tests/lib/LoggerTest.php:59

Since you are wrapping the value in another array the depth changes ... migrate or fallback needed.

Member

butonic commented Mar 23, 2017

@shadone cool, thx. Now Jenkins still complains as reported earlier:

Test\LoggerTest::testAppCondition
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
-    0 => '1 Show info messages of files app'
-    1 => '2 Show warning messages of other apps'
+    0 => '2 Show warning messages of other apps'
 )

/var/lib/jenkins/workspace/owncloud-core_core_PR-27443-VFE2WOKMDT4YDIDOZ3FLLTMZCW3UDIIKOVVAQZY6LBKX3ESO43XA/tests/lib/LoggerTest.php:59

Since you are wrapping the value in another array the depth changes ... migrate or fallback needed.

@butonic

This comment has been minimized.

Show comment
Hide comment
@butonic

butonic Mar 23, 2017

Member

marked as breaking change

Member

butonic commented Mar 23, 2017

marked as breaking change

Show outdated Hide outdated config/config.sample.php
Show outdated Hide outdated config/config.sample.php
@sharidas

This comment has been minimized.

Show comment
Hide comment
@sharidas

sharidas Mar 23, 2017

Member

@butonic I have updated the PR. I am trying to update the tests/lib/LoggerTest.php to incorporate new feature.

Member

sharidas commented Mar 23, 2017

@butonic I have updated the PR. I am trying to update the tests/lib/LoggerTest.php to incorporate new feature.

Show outdated Hide outdated lib/private/Log.php
@sharidas

This comment has been minimized.

Show comment
Hide comment
@sharidas

sharidas Mar 23, 2017

Member

@mmattel @butonic I have updated the PR. Let me know if more changes are required.

Member

sharidas commented Mar 23, 2017

@mmattel @butonic I have updated the PR. Let me know if more changes are required.

@sharidas

This comment has been minimized.

Show comment
Hide comment
@sharidas

sharidas Mar 24, 2017

Member

@butonic I have updated the PR. Let me know if further refinements or changes are to be incorporated in this PR.

Member

sharidas commented Mar 24, 2017

@butonic I have updated the PR. Let me know if further refinements or changes are to be incorporated in this PR.

Show outdated Hide outdated lib/private/Log.php
Show outdated Hide outdated lib/private/Log.php
Show outdated Hide outdated tests/lib/LoggerTest.php
@sharidas

This comment has been minimized.

Show comment
Hide comment
@sharidas

sharidas Mar 24, 2017

Member

@butonic As per the review comments I have replaced the log.conditions in the LoggerTest.php file. I have also updated the PR to include the changes recommended:

  1. $logConditions[] = $this->config->getValue('log.condition', []);
  2. Removed the fallback code.
    Let me know if more changes are required.
Member

sharidas commented Mar 24, 2017

@butonic As per the review comments I have replaced the log.conditions in the LoggerTest.php file. I have also updated the PR to include the changes recommended:

  1. $logConditions[] = $this->config->getValue('log.condition', []);
  2. Removed the fallback code.
    Let me know if more changes are required.
@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Mar 24, 2017

Member

@sharidas please fix the failing unit test

Member

PVince81 commented Mar 24, 2017

@sharidas please fix the failing unit test

Separate audit logs from server logs.
This change will help users to separate logs
for each apps based on conditional logging.
Changed usage of log.condition to
log.conditions.

Signed-off-by: Sujith H <sharidasan@owncloud.com>
@sharidas

This comment has been minimized.

Show comment
Hide comment
@sharidas

sharidas Mar 24, 2017

Member

Updated the PR by fixing the failing unit test.

Member

sharidas commented Mar 24, 2017

Updated the PR by fixing the failing unit test.

@butonic

This comment has been minimized.

Show comment
Hide comment
@butonic

butonic Mar 24, 2017

Member

I think this is ok now. Jenkins is happy. 👍

Member

butonic commented Mar 24, 2017

I think this is ok now. Jenkins is happy. 👍

@PVince81 PVince81 merged commit 6f8bfca into master Mar 24, 2017

4 checks passed

Scrutinizer 1 new issues, 3 updated code elements
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details

@PVince81 PVince81 deleted the separate-audit-logs-sharidas branch Mar 24, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment