Adds new logging type SYSLOG which logs to the system log.

Adds new logging type SYSLOG which logs to the system log.

On Windows this type logs to the Event Log and on POSIX systems it logs to the messages log (or its equvalent).

As a side effect of adding the presubmit check for it this
CL fixes running the presumbit checks tests on Windows.

BUG=642115
Committed: https://crrev.com/89f7ee10f03d696276adf1cdc901c949a2e091d4
Cr-Commit-Position: refs/heads/master@{#419758}

[pastarmovj, 2016-09-01 13:08:42]

Description was changed from

==========
Adds new logging type SYSLOG which logs to the system log.

On Windows this type logs to the Event Log and on POSIX systems
it logs to the messages log (or its equvalent).

BUG=642115
==========

to

==========
Adds new logging type SYSLOG which logs to the system log.

On Windows this type logs to the Event Log and on POSIX systems it logs to the
messages log (or its equvalent).

As a side effect of adding the presubmit check for it this 
CL fixes running the presumbit checks tests on Windows.

BUG=642115
==========

[pastarmovj, 2016-09-01 14:59:58]

pastarmovj@chromium.org changed reviewers:
+ brettw@chromium.org

[pastarmovj, 2016-09-01 14:59:59]

Hi Brett,

can you please review this CL? The linked bug has the context of this change
pretty well explained. 

The presubmit checks were coordinated with the privacy team to facilitate
conscious use of this feature.

The further presubmit fixes I made to be able to run the presubmit tests on my
Windows dev machine.

Thanks,
Julian

[brettw, 2016-09-07 17:54:56]

How often do you expect this to be used? From reading the bug it seems like
maybe one place, or at most a few.

In my opinion base/logging is already way over-complicated. I'm reluctant to add
more complexity, especially since this has worked for so long.

It's also not clear to me that the users of these system logs actually want
Google-style log messages with source file names and line numbers. From the
stated use-case in the bug, I suspect the answer is "no".

I think all we really need is a simple function that takes a string and writes
it to the system event log. Can we do that instead?

[pastarmovj, 2016-09-07 21:01:41]

On 2016/09/07 17:54:56, brettw (ping on IM after 24h) wrote:
> How often do you expect this to be used? From reading the bug it seems like
> maybe one place, or at most a few.
> 
> In my opinion base/logging is already way over-complicated. I'm reluctant to
add
> more complexity, especially since this has worked for so long.
> 
> It's also not clear to me that the users of these system logs actually want
> Google-style log messages with source file names and line numbers. From the
> stated use-case in the bug, I suspect the answer is "no".
> 
> I think all we really need is a simple function that takes a string and writes
> it to the system event log. Can we do that instead?

Hi,
thanks for looking into this!

I am not opposed to making this a separate API call. I think the use cases
around the enterprise code will be many as this is a good channel to communicate
issues to the right person to see them. It certainly is not meant to be of the
scope of LOG/DLOG etc but I can imagine a few dozens invocations might appear
eventually. 

The reason I implemented it with the existing *LOG familiy logic was to also
make it write to the regular log file as a side effect in case we want this logs
interspersed with the general logging. Lastly the format is certainly not set in
stone but I think the existing format gives both usable information for the
admin directly but also actionable information shall they decide to raise a bug
from them.

I can still pull the logic in a separate file but keep it based on the *LOG
format. I can of course also completely move it to a special call but then I
think most places it will be used will be also backed by a mirror LOG line as
well.

WDYT?

[brettw, 2016-09-12 21:04:29]

I still prefer adding a new thing instead of changing the shared logging. I
think the new thing can have much lower complexity.

It seems reasonable to me that a new system call function call API call can just
do:
  "LOG(WARNING) << message;"
To cover the "chrome log wants the message too" use case. It seems for the
system logs to be useful for your case (third-parties looking at them), the text
will need to be super-descriptive so that the source file and line number in the
Chrome logs aren't actually very helpful.

[pastarmovj, 2016-09-14 00:25:33]

How about that - I moved all the new stuff in separate files. Under the hood
they still use the logging machinery because I believe that the stream based
formatting of messages will be of great value here instead of having to piece
together static strings first.

As for the contents of the logged data and the format. Eventually what we
believe the course of action on those logs will be, is that admins will try to
act upon them by checking affected machines and in many cases resort to opening
a bug on crbug.com. This is where the file name and line number comes in handy.
Even for the Windows event log (which is not intended as a single line text) for
ERROR and above the whole stack trace is in the log.

Out of the top of my head I can come up with at least a dozen cases where I will
use this new logging right away - anything that is policy application hitting an
error that is OS bound is a great candidate.

The exact text used does not have to be very verbose but rather easily
searchable. If you look at your random event log content this is usually the
case - errno + short error message and a stack trace (for example uncaught .net
exceptions). People will be searching those on the internet - stackoverflow,
crbug etc.

[brettw, 2016-09-14 22:33:48]

I can live with this design.

https://codereview.chromium.org/2296783002/diff/40001/base/syslog_logging.cc
File base/syslog_logging.cc (right):

https://codereview.chromium.org/2296783002/diff/40001/base/syslog_logging.cc#...
base/syslog_logging.cc:29: HANDLE event_log_handle = RegisterEventSourceA(NULL,
kEventSource);
I'm not familiar with this API, but MSDN says the name "must be a subkey of a
log under the Eventlog registry key". Is that true for the string "chrome" here?

https://codereview.chromium.org/2296783002/diff/40001/base/syslog_logging.cc#...
base/syslog_logging.cc:64: // we use the actual values which of course do no
match ours. See sys/syslog.h
"do no" -> "do not"

https://codereview.chromium.org/2296783002/diff/40001/base/syslog_logging.h
File base/syslog_logging.h (right):

https://codereview.chromium.org/2296783002/diff/40001/base/syslog_logging.h#n...
base/syslog_logging.h:1: // Copyright (c) 2012 The Chromium Authors. All rights
reserved.
The new copyright block has no "(c)" and "2016" :)

[pastarmovj, 2016-09-15 21:27:10]

Thanks! PTAL.

https://codereview.chromium.org/2296783002/diff/40001/base/syslog_logging.cc
File base/syslog_logging.cc (right):

https://codereview.chromium.org/2296783002/diff/40001/base/syslog_logging.cc#...
base/syslog_logging.cc:29: HANDLE event_log_handle = RegisterEventSourceA(NULL,
kEventSource);
On 2016/09/14 22:33:48, brettw (ping on IM after 24h) wrote:
> I'm not familiar with this API, but MSDN says the name "must be a subkey of a
> log under the Eventlog registry key". Is that true for the string "chrome"
here?

Actually this string appears in the log as the "source application" field. It
can be registered as a known log source or not (as is now) in which case it
always goes in the "Applications" event log.

Here is the remark about it: "If the source name cannot be found, the event
logging service uses the Application log. Although events will be reported , the
events will not include descriptions because there are no message and category
message files for looking up descriptions related to the event identifiers."

The TODO below refers to eventually register our events with the event log if we
ever decide to do more sophisticated logging. For now simple string reporting in
the logs is sufficient for the purpose it serves.

https://codereview.chromium.org/2296783002/diff/40001/base/syslog_logging.cc#...
base/syslog_logging.cc:64: // we use the actual values which of course do no
match ours. See sys/syslog.h
On 2016/09/14 22:33:48, brettw (ping on IM after 24h) wrote:
> "do no" -> "do not"

Done.

https://codereview.chromium.org/2296783002/diff/40001/base/syslog_logging.h
File base/syslog_logging.h (right):

https://codereview.chromium.org/2296783002/diff/40001/base/syslog_logging.h#n...
base/syslog_logging.h:1: // Copyright (c) 2012 The Chromium Authors. All rights
reserved.
On 2016/09/14 22:33:48, brettw (ping on IM after 24h) wrote:
> The new copyright block has no "(c)" and "2016" :)

Done.

[brettw, 2016-09-15 21:54:02]

thanks for clarifying, lgtm

[pastarmovj, 2016-09-20 09:57:20]

The CQ bit was checked by pastarmovj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-20 09:57:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-20 10:47:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-20 10:47:31]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)

[pastarmovj, 2016-09-20 12:27:49]

The CQ bit was checked by pastarmovj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-20 12:28:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-20 12:39:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-20 12:39:58]

Dry run: Try jobs failed on following builders:
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[pastarmovj, 2016-09-20 12:44:10]

The CQ bit was checked by pastarmovj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-20 12:44:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[pastarmovj, 2016-09-20 12:54:18]

Patchset #7 (id:120001) has been deleted

[pastarmovj, 2016-09-20 12:55:21]

The CQ bit was checked by pastarmovj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-20 12:55:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-20 14:13:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-20 14:13:32]

Dry run: This issue passed the CQ dry run.

[pastarmovj, 2016-09-20 14:52:56]

The CQ bit was checked by pastarmovj@chromium.org

[pastarmovj, 2016-09-20 14:52:57]

The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org
Link to the patchset: https://codereview.chromium.org/2296783002/#ps140001
(title: "Copy/paste error.")

[commit-bot: I haz the power, 2016-09-20 14:53:14]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-20 14:58:31]

Description was changed from

==========
Adds new logging type SYSLOG which logs to the system log.

On Windows this type logs to the Event Log and on POSIX systems it logs to the
messages log (or its equvalent).

As a side effect of adding the presubmit check for it this 
CL fixes running the presumbit checks tests on Windows.

BUG=642115
==========

to

==========
Adds new logging type SYSLOG which logs to the system log.

On Windows this type logs to the Event Log and on POSIX systems it logs to the
messages log (or its equvalent).

As a side effect of adding the presubmit check for it this 
CL fixes running the presumbit checks tests on Windows.

BUG=642115
==========

[commit-bot: I haz the power, 2016-09-20 14:58:32]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2016-09-20 14:59:54]

Description was changed from

==========
Adds new logging type SYSLOG which logs to the system log.

On Windows this type logs to the Event Log and on POSIX systems it logs to the
messages log (or its equvalent).

As a side effect of adding the presubmit check for it this 
CL fixes running the presumbit checks tests on Windows.

BUG=642115
==========

to

==========
Adds new logging type SYSLOG which logs to the system log.

On Windows this type logs to the Event Log and on POSIX systems it logs to the
messages log (or its equvalent).

As a side effect of adding the presubmit check for it this
CL fixes running the presumbit checks tests on Windows.

BUG=642115

Committed: https://crrev.com/89f7ee10f03d696276adf1cdc901c949a2e091d4
Cr-Commit-Position: refs/heads/master@{#419758}
==========

[commit-bot: I haz the power, 2016-09-20 14:59:55]

Patchset 7 (id:??) landed as
https://crrev.com/89f7ee10f03d696276adf1cdc901c949a2e091d4
Cr-Commit-Position: refs/heads/master@{#419758}

[Evan Stade, 2017-01-12 01:56:41]

estade@chromium.org changed reviewers:
+ estade@chromium.org

[Evan Stade, 2017-01-12 01:56:42]

https://codereview.chromium.org/2296783002/diff/140001/PRESUBMIT.py
File PRESUBMIT.py (right):

https://codereview.chromium.org/2296783002/diff/140001/PRESUBMIT.py#newcode2223
PRESUBMIT.py:2223: if 'SYSLOG' in input_api.ReadFile(f, 'rb'):
is there a particular reason to read the whole file and not changed contents?
Every time I edit PRESUBMIT.py and run presubmit, it flags the SYSLOG on this
line.

[pastarmovj, 2017-01-12 10:35:21]

Hi Evan,
sorry for that. I changed the presubmit check to only check changed lines.
Cheers,
Julian

https://codereview.chromium.org/2296783002/diff/140001/PRESUBMIT.py
File PRESUBMIT.py (right):

https://codereview.chromium.org/2296783002/diff/140001/PRESUBMIT.py#newcode2223
PRESUBMIT.py:2223: if 'SYSLOG' in input_api.ReadFile(f, 'rb'):
On 2017/01/12 01:56:42, Evan Stade wrote:
> is there a particular reason to read the whole file and not changed contents?
> Every time I edit PRESUBMIT.py and run presubmit, it flags the SYSLOG on this
> line.

Problem solved :) https://codereview.chromium.org/2622083006/

Sorry for the inconvenience.