Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2530163002:
Make the name of the event source for SYSLOG configurable. (Closed)
|
Created:
4 years ago by pastarmovj Modified:
4 years ago CC:
chromium-reviews, vmpstr+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake the name of the event source for SYSLOG configurable.
In order to allow this code to properly operate for all Chromium and Chrome flavors (e.g. Stable, Beta, Canary etc.) it is important to allow for different source names for the Event Logger.
The default behavior of SYSLOG now is equivalent to a LOG unless
SetEventSourceName is called at which point the function can start
logging to the Event Log as well.
For Branded Chrome build the event source stays Chrome for the Stable channel and will have the proper appendix for the various other channels whereas for non-branded build it will be Chromium.
BUG=668397
TEST=none
Committed: https://crrev.com/a3eac43eeb3dc4bb862e6e27e9d12f7250428804
Cr-Commit-Position: refs/heads/master@{#436994}
Patch Set 1 #Patch Set 2 : Fix BUILD.gn #
Total comments: 4
Patch Set 3 : Change to per-channel naming. #
Total comments: 8
Patch Set 4 : Fix includes and nits. #
Total comments: 1
Patch Set 5 : Clean up. #Patch Set 6 : Rebase on ToT. #Patch Set 7 : Fix unit test. #
Total comments: 4
Patch Set 8 : Add not reached. #Patch Set 9 : Remove NOTREACHED and log suffix. #
Messages
Total messages: 40 (18 generated)
pastarmovj@chromium.org changed reviewers: + grt@chromium.org
Hi Greg, please review the way I pull the app name from install_static and if you are ok with it I will get some owners to review this CL as well. Thanks, Julian
https://codereview.chromium.org/2530163002/diff/20001/base/syslog_logging.cc File base/syslog_logging.cc (right): https://codereview.chromium.org/2530163002/diff/20001/base/syslog_logging.cc#... base/syslog_logging.cc:25: std::string g_event_source_name; non-POD globals are banned (https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables). you could use std::string* and leak it at exit. or use a Leaky LazyInstance if it's possible for the setter and getters to race. maybe you can avoid that if you dictate that the setter must be called during single-threaded initialization. https://codereview.chromium.org/2530163002/diff/20001/chrome/common/logging_c... File chrome/common/logging_chrome.cc (right): https://codereview.chromium.org/2530163002/diff/20001/chrome/common/logging_c... chrome/common/logging_chrome.cc:367: base::UTF16ToASCII(install_static::kProductPathName)); while this will be a brand-specific string (e.g., "Chromium" or "Chrome"), what we need here is a "mode"-specific string to account for a product being installed in multiple side-by-side locations (e.g., "Chrome SxS" or "Chrome Beta" vs "Chrome"). i propose that you add a new member to install_static::InstallConstants for the log event source name and add values for it to the kInstallModes definitions in chromium_install_modes.cc and google_chrome_install_modes.cc. you can then add a convenience getter to InstallDetails that you call here.
https://codereview.chromium.org/2530163002/diff/20001/base/syslog_logging.cc File base/syslog_logging.cc (right): https://codereview.chromium.org/2530163002/diff/20001/base/syslog_logging.cc#... base/syslog_logging.cc:25: std::string g_event_source_name; On 2016/11/25 15:25:34, grt (UTC plus 1) wrote: > non-POD globals are banned > (https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables). > you could use std::string* and leak it at exit. or use a Leaky LazyInstance if > it's possible for the setter and getters to race. maybe you can avoid that if > you dictate that the setter must be called during single-threaded > initialization. Done. https://codereview.chromium.org/2530163002/diff/20001/chrome/common/logging_c... File chrome/common/logging_chrome.cc (right): https://codereview.chromium.org/2530163002/diff/20001/chrome/common/logging_c... chrome/common/logging_chrome.cc:367: base::UTF16ToASCII(install_static::kProductPathName)); On 2016/11/25 15:25:35, grt (UTC plus 1) wrote: > while this will be a brand-specific string (e.g., "Chromium" or "Chrome"), what > we need here is a "mode"-specific string to account for a product being > installed in multiple side-by-side locations (e.g., "Chrome SxS" or "Chrome > Beta" vs "Chrome"). i propose that you add a new member to > install_static::InstallConstants for the log event source name and add values > for it to the kInstallModes definitions in chromium_install_modes.cc and > google_chrome_install_modes.cc. you can then add a convenience getter to > InstallDetails that you call here. I am not convinced that this particular use case deserves splitting up at such fine granularity. But the code needed to achieve it is not much and we can rather do this now than later. I decided to go for a more generic install_full_name() rather than a specific to this case name. WDYT?
https://codereview.chromium.org/2530163002/diff/40001/base/syslog_logging.cc File base/syslog_logging.cc (right): https://codereview.chromium.org/2530163002/diff/40001/base/syslog_logging.cc#... base/syslog_logging.cc:29: DCHECK(g_event_source_name == nullptr); nit: DCHECK_EQ(g_event_source_name, nullptr); https://codereview.chromium.org/2530163002/diff/40001/base/syslog_logging.cc#... base/syslog_logging.cc:56: nit: consider base::ScopedClosureRunner auto_deregister(base::Bind(base::IgnoreResult(&DeregisterEventSource), event_log_handle)); (or something like that) and remove line 80 to avoid a possible future handle leak when someone adds a return statement in the middle of this. https://codereview.chromium.org/2530163002/diff/40001/chrome/install_static/i... File chrome/install_static/install_details.h (right): https://codereview.chromium.org/2530163002/diff/40001/chrome/install_static/i... chrome/install_static/install_details.h:17: // Re-declare the constants from install_modes.h to avoid cyclical dependency. should install_modes.h really be including install_constants.h rather than install_details.h? if so, does fixing that remove the need for this? https://codereview.chromium.org/2530163002/diff/40001/chrome/install_static/i... chrome/install_static/install_details.h:81: std::wstring install_full_name() const { i'd be inclined to make this GetInstallFullName and de-inline it to get rid of the ugly declarations above if changing install_modes.h doesn't allow for it
ptal. Thanks! https://codereview.chromium.org/2530163002/diff/40001/base/syslog_logging.cc File base/syslog_logging.cc (right): https://codereview.chromium.org/2530163002/diff/40001/base/syslog_logging.cc#... base/syslog_logging.cc:29: DCHECK(g_event_source_name == nullptr); On 2016/11/28 12:49:52, grt (UTC plus 1) wrote: > nit: DCHECK_EQ(g_event_source_name, nullptr); Done. https://codereview.chromium.org/2530163002/diff/40001/base/syslog_logging.cc#... base/syslog_logging.cc:56: On 2016/11/28 12:49:52, grt (UTC plus 1) wrote: > nit: consider > base::ScopedClosureRunner > auto_deregister(base::Bind(base::IgnoreResult(&DeregisterEventSource), > event_log_handle)); > (or something like that) > and remove line 80 to avoid a possible future handle leak when someone adds a > return statement in the middle of this. Done. https://codereview.chromium.org/2530163002/diff/40001/chrome/install_static/i... File chrome/install_static/install_details.h (right): https://codereview.chromium.org/2530163002/diff/40001/chrome/install_static/i... chrome/install_static/install_details.h:17: // Re-declare the constants from install_modes.h to avoid cyclical dependency. On 2016/11/28 12:49:52, grt (UTC plus 1) wrote: > should install_modes.h really be including install_constants.h rather than > install_details.h? if so, does fixing that remove the need for this? Fixed includes globally. https://codereview.chromium.org/2530163002/diff/40001/chrome/install_static/i... chrome/install_static/install_details.h:81: std::wstring install_full_name() const { On 2016/11/28 12:49:52, grt (UTC plus 1) wrote: > i'd be inclined to make this GetInstallFullName and de-inline it to get rid of > the ugly declarations above if changing install_modes.h doesn't allow for it Acknowledged.
lgtm with a nit. thanks! https://codereview.chromium.org/2530163002/diff/60001/chrome/install_static/i... File chrome/install_static/install_details.h (right): https://codereview.chromium.org/2530163002/diff/60001/chrome/install_static/i... chrome/install_static/install_details.h:18: // Re-declare the constants from install_modes.h to avoid cyclical dependency. nit: remove this now? :-)
pastarmovj@chromium.org changed reviewers: + thakis@chromium.org
Thanks Greg! Nico, please review the CL for OWNER approval. For context the plan is to make this base code more easily reusable by other projects based on the Chrome codebase and also allow to distinguish logging events from different Chrome channels https://codereview.chromium.org/2507753002/. Cheers, Julian
The CQ bit was checked by grt@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
The CQ bit was checked by pastarmovj@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
That's an explicit non-goal in general. tried understanding what you want to do, but the CL description you link to is fairly sparse and links to a Fixed bug. In what context do you want to use base outside of chrome? On Tue, Nov 29, 2016 at 7:52 AM, <pastarmovj@chromium.org> wrote: > Thanks Greg! > > Nico, please review the CL for OWNER approval. > > For context the plan is to make this base code more easily reusable by > other > projects based on the Chrome codebase and also allow to distinguish logging > events from different Chrome channels > https://codereview.chromium.org/2507753002/. > > Cheers, > Julian > > > > > https://codereview.chromium.org/2530163002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/11/29 16:20:18, Nico wrote: > That's an explicit non-goal in general. tried understanding what you want > to do, but the CL description you link to is fairly sparse and links to a > Fixed bug. In what context do you want to use base outside of chrome? For me, the big issue is not supporting use of base outside of Chrome, but rather avoiding conflicts with multiple Chrome installs. Without this change, the following sequence will be problematic: Install stable Chrome Install beta Chrome (in the near future when we support side-by-side beta on Windows) Uninstall beta Chrome The last step would remove registration of the event provider, so stable Chrome's events in the event log would lack formatting. This could lead to bug reports that we'd waste time investigating. Additionally, I would argue that making sure that other users of base don't interfere with Chrome should be an explicit goal of ours.
Description was changed from ========== Make the name of the event source for SYSLOG configurable. In order to allow this code to be shared by other projects using the Chromium codebase it is important to allow for different sources for the Event Logger. The default behavior of SYSLOG now is equivalent to LOG unless SetEventSourceName is called at which point the function can start logging to the Event Log as well. For Branded Chrome build the event source stays Chrome and for non-branded build it will be Chromium. BUG=668397 TEST=none ========== to ========== Make the name of the event source for SYSLOG configurable. In order to allow this code to properly operate for all Chromium and Chrome flavors (e.g. Stable, Beta, Canary etc.) it is important to allow for different source names for the Event Logger. The default behavior of SYSLOG now is equivalent to LOG unless SetEventSourceName is called at which point the function can start logging to the Event Log as well. For Branded Chrome build the event source stays Chrome for the Stable channel and will have the proper appendix for the various other channels whereas for non-branded build it will be Chromium. BUG=668397 TEST=none ==========
What Greg said, I was a little vague about what I wanted to say there but I was also thinking for all other Chromium forks out there which we most certainly would rather not have report to the event log as "Chrome".
On 2016/11/29 21:56:18, pastarmovj wrote: > What Greg said, I was a little vague about what I wanted to say there but I was > also thinking for all other Chromium forks out there which we most certainly > would rather not have report to the event log as "Chrome". gentle ping?
Thanks, doing this for side-by-side makes sense to me. The bug this links to also says "other projects sharing the codebase" though, and that really doesn't sound like other channels. What does it refer to? https://codereview.chromium.org/2530163002/diff/120001/base/syslog_logging.cc File base/syslog_logging.cc (right): https://codereview.chromium.org/2530163002/diff/120001/base/syslog_logging.cc... base/syslog_logging.cc:45: // degrade gracefully to regular LOG with an extra error suffix. If you see Is this useful? Why wouldn't this CHECK(false) instead? The CL description currently says """ The default behavior of SYSLOG now is equivalent to LOG unless SetEventSourceName is called at which point the function can start logging to the Event Log as well. """ If every output is prefixed with " !!EVENTLOG SOURCE NAME NOT SET!!" that doesn't seem equivalent to LOG. If this is a problem we should CHECK(false). If it isn't, we shouldn't include the noise.
Description was changed from ========== Make the name of the event source for SYSLOG configurable. In order to allow this code to properly operate for all Chromium and Chrome flavors (e.g. Stable, Beta, Canary etc.) it is important to allow for different source names for the Event Logger. The default behavior of SYSLOG now is equivalent to LOG unless SetEventSourceName is called at which point the function can start logging to the Event Log as well. For Branded Chrome build the event source stays Chrome for the Stable channel and will have the proper appendix for the various other channels whereas for non-branded build it will be Chromium. BUG=668397 TEST=none ========== to ========== Make the name of the event source for SYSLOG configurable. In order to allow this code to properly operate for all Chromium and Chrome flavors (e.g. Stable, Beta, Canary etc.) it is important to allow for different source names for the Event Logger. The default behavior of SYSLOG now is equivalent to a grumpy version of LOG unless SetEventSourceName is called at which point the function can start logging to the Event Log as well. For Branded Chrome build the event source stays Chrome for the Stable channel and will have the proper appendix for the various other channels whereas for non-branded build it will be Chromium. BUG=668397 TEST=none ==========
I changes the description a bit to not lie for 100% equivalency with LOG, and made it a DCHECK to not have a proper initialization. https://codereview.chromium.org/2530163002/diff/120001/base/syslog_logging.cc File base/syslog_logging.cc (right): https://codereview.chromium.org/2530163002/diff/120001/base/syslog_logging.cc... base/syslog_logging.cc:45: // degrade gracefully to regular LOG with an extra error suffix. If you see On 2016/12/02 03:08:41, Nico wrote: > Is this useful? Why wouldn't this CHECK(false) instead? > > The CL description currently says > > """ > The default behavior of SYSLOG now is equivalent to LOG unless > SetEventSourceName is called at which point the function can start > logging to the Event Log as well. > """ > > If every output is prefixed with " !!EVENTLOG SOURCE NAME NOT SET!!" that > doesn't seem equivalent to LOG. If this is a problem we should CHECK(false). If > it isn't, we shouldn't include the noise. I see this as a debugging facility for the debigging facility. :) At best I see a NOTREACHED here but nothing that will run in release builds because it be a shame to crash even once instance of chrome over this.
Thanks. https://codereview.chromium.org/2530163002/diff/120001/base/syslog_logging.cc File base/syslog_logging.cc (right): https://codereview.chromium.org/2530163002/diff/120001/base/syslog_logging.cc... base/syslog_logging.cc:45: // degrade gracefully to regular LOG with an extra error suffix. If you see On 2016/12/02 19:30:59, pastarmovj wrote: > On 2016/12/02 03:08:41, Nico wrote: > > Is this useful? Why wouldn't this CHECK(false) instead? > > > > The CL description currently says > > > > """ > > The default behavior of SYSLOG now is equivalent to LOG unless > > SetEventSourceName is called at which point the function can start > > logging to the Event Log as well. > > """ > > > > If every output is prefixed with " !!EVENTLOG SOURCE NAME NOT SET!!" that > > doesn't seem equivalent to LOG. If this is a problem we should CHECK(false). > If > > it isn't, we shouldn't include the noise. > > I see this as a debugging facility for the debigging facility. :) > At best I see a NOTREACHED here but nothing that will run in release builds > because it be a shame to crash even once instance of chrome over this. History shows that nobody looks at log spam. Either it's a bug when this is not set, then this should be a (D)CHECK, or it's ok, then it shouldn't spam. See also the section on CHECKs in https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md (which says "if you DCHECK/NOTREACHED, don't try to recover")
https://codereview.chromium.org/2530163002/diff/120001/base/syslog_logging.cc File base/syslog_logging.cc (right): https://codereview.chromium.org/2530163002/diff/120001/base/syslog_logging.cc... base/syslog_logging.cc:45: // degrade gracefully to regular LOG with an extra error suffix. If you see On 2016/12/02 20:28:58, Nico wrote: > On 2016/12/02 19:30:59, pastarmovj wrote: > > On 2016/12/02 03:08:41, Nico wrote: > > > Is this useful? Why wouldn't this CHECK(false) instead? > > > > > > The CL description currently says > > > > > > """ > > > The default behavior of SYSLOG now is equivalent to LOG unless > > > SetEventSourceName is called at which point the function can start > > > logging to the Event Log as well. > > > """ > > > > > > If every output is prefixed with " !!EVENTLOG SOURCE NAME NOT SET!!" that > > > doesn't seem equivalent to LOG. If this is a problem we should CHECK(false). > > If > > > it isn't, we shouldn't include the noise. > > > > I see this as a debugging facility for the debigging facility. :) > > At best I see a NOTREACHED here but nothing that will run in release builds > > because it be a shame to crash even once instance of chrome over this. > > History shows that nobody looks at log spam. Either it's a bug when this is not > set, then this should be a (D)CHECK, or it's ok, then it shouldn't spam. See > also the section on CHECKs in > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md > (which says "if you DCHECK/NOTREACHED, don't try to recover") Done.
Description was changed from ========== Make the name of the event source for SYSLOG configurable. In order to allow this code to properly operate for all Chromium and Chrome flavors (e.g. Stable, Beta, Canary etc.) it is important to allow for different source names for the Event Logger. The default behavior of SYSLOG now is equivalent to a grumpy version of LOG unless SetEventSourceName is called at which point the function can start logging to the Event Log as well. For Branded Chrome build the event source stays Chrome for the Stable channel and will have the proper appendix for the various other channels whereas for non-branded build it will be Chromium. BUG=668397 TEST=none ========== to ========== Make the name of the event source for SYSLOG configurable. In order to allow this code to properly operate for all Chromium and Chrome flavors (e.g. Stable, Beta, Canary etc.) it is important to allow for different source names for the Event Logger. The default behavior of SYSLOG now is equivalent to a LOG unless SetEventSourceName is called at which point the function can start logging to the Event Log as well. For Branded Chrome build the event source stays Chrome for the Stable channel and will have the proper appendix for the various other channels whereas for non-branded build it will be Chromium. BUG=668397 TEST=none ==========
Gentle ping.
"""Thanks, doing this for side-by-side makes sense to me. The bug this links to also says "other projects sharing the codebase" though, and that really doesn't sound like other channels. What does it refer to?""" remains unanswered as far as I can tell.
On 2016/12/07 15:55:33, Nico wrote: > """Thanks, doing this for side-by-side makes sense to me. The bug this links to > also says "other projects sharing the codebase" though, and that really doesn't > sound like other channels. What does it refer to?""" > > remains unanswered as far as I can tell. Sorry I thought this got clarified in the course of discussion. The bug text was hastily typed by me just like the description of this CL. I was enivisioning Chromium vs. Chrome and as external products all other Chromium clones out there. We certainly would rather not have those claim to be Chrome in the event log. But for the sake of brevity let's say it is all channels of Chrome and Chromium.
lgtm
The CQ bit was checked by pastarmovj@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from grt@chromium.org Link to the patchset: https://codereview.chromium.org/2530163002/#ps160001 (title: "Remove NOTREACHED and log suffix.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1481128326190720,
"parent_rev": "276fe0b7a512529a5a61f0e2b29d3d1feca21265", "commit_rev":
"6b9e908c32425b054049016736b158ae39945908"}
Message was sent while issue was closed.
Description was changed from ========== Make the name of the event source for SYSLOG configurable. In order to allow this code to properly operate for all Chromium and Chrome flavors (e.g. Stable, Beta, Canary etc.) it is important to allow for different source names for the Event Logger. The default behavior of SYSLOG now is equivalent to a LOG unless SetEventSourceName is called at which point the function can start logging to the Event Log as well. For Branded Chrome build the event source stays Chrome for the Stable channel and will have the proper appendix for the various other channels whereas for non-branded build it will be Chromium. BUG=668397 TEST=none ========== to ========== Make the name of the event source for SYSLOG configurable. In order to allow this code to properly operate for all Chromium and Chrome flavors (e.g. Stable, Beta, Canary etc.) it is important to allow for different source names for the Event Logger. The default behavior of SYSLOG now is equivalent to a LOG unless SetEventSourceName is called at which point the function can start logging to the Event Log as well. For Branded Chrome build the event source stays Chrome for the Stable channel and will have the proper appendix for the various other channels whereas for non-branded build it will be Chromium. BUG=668397 TEST=none ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Make the name of the event source for SYSLOG configurable. In order to allow this code to properly operate for all Chromium and Chrome flavors (e.g. Stable, Beta, Canary etc.) it is important to allow for different source names for the Event Logger. The default behavior of SYSLOG now is equivalent to a LOG unless SetEventSourceName is called at which point the function can start logging to the Event Log as well. For Branded Chrome build the event source stays Chrome for the Stable channel and will have the proper appendix for the various other channels whereas for non-branded build it will be Chromium. BUG=668397 TEST=none ========== to ========== Make the name of the event source for SYSLOG configurable. In order to allow this code to properly operate for all Chromium and Chrome flavors (e.g. Stable, Beta, Canary etc.) it is important to allow for different source names for the Event Logger. The default behavior of SYSLOG now is equivalent to a LOG unless SetEventSourceName is called at which point the function can start logging to the Event Log as well. For Branded Chrome build the event source stays Chrome for the Stable channel and will have the proper appendix for the various other channels whereas for non-branded build it will be Chromium. BUG=668397 TEST=none Committed: https://crrev.com/a3eac43eeb3dc4bb862e6e27e9d12f7250428804 Cr-Commit-Position: refs/heads/master@{#436994} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/a3eac43eeb3dc4bb862e6e27e9d12f7250428804 Cr-Commit-Position: refs/heads/master@{#436994} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
