Configure logging to log to Stackdriver #155

carise · 2017-03-25T07:28:49Z

Logger has a console handler (logging.StreamHandler) and a syslog handler (logging.handlers.SysLogHandler). On GCE instance we install the fluentd logging agent which will transparently pass the syslog entries to stackdriver

resolves #73

…google-api-client-python

…r script main()

mirons-google · 2017-03-28T15:53:26Z

google/cloud/security/common/data_access/dao.py

@@ -29,11 +29,8 @@
 from google.cloud.security.common.data_access.errors import NoResultsError
 from google.cloud.security.common.data_access.sql_queries import create_tables
 from google.cloud.security.common.data_access.sql_queries import select_data
-from google.cloud.security.common.util.log_util import LogUtil


We don't need to add back in logging support?

it wasn't used anywhere in this file.

mirons-google · 2017-03-28T15:53:35Z

google/cloud/security/common/data_access/csv_writer.py

@@ -18,11 +18,8 @@
 import tempfile

 from google.cloud.security.common.data_access.errors import CSVFileError
-from google.cloud.security.common.util.log_util import LogUtil


We don't need to add back in logging support?

ahoying

Make sure the imports are consistent, since you started cleaning them up in the PR.

We should be importing the module, not the class:
https://google.github.io/styleguide/pyguide.html#Imports

ahoying · 2017-03-28T15:32:37Z

google/cloud/security/common/gcp_api/compute.py

 from google.cloud.security.common.gcp_api._base_client import _BaseClient

+def is_compute_engine_instance():


It seems a bit out of place to put this here. I think a module under ../util would be better.

ahoying · 2017-03-28T15:35:05Z

google/cloud/security/common/third_party/transports/background_thread.py

@@ -0,0 +1,171 @@
+# Copyright 2016 Google Inc.


How will this stay in sync with the file in google-cloud-python?

ahoying · 2017-03-28T15:36:16Z

google/cloud/security/common/util/log_util.py

+FLAGS = flags.FLAGS
+flags.DEFINE_boolean('use_cloud_logging', False,
+                     'Use Cloud Logging, if available.')
+flags.DEFINE_boolean('nouse_cloud_logging', False, 'Do not use Cloud Logging.')


DEFINE_boolean automatically creates the no version of the flag, you don't have to do that yourself. The no version will set the flag value False.

And based on the code below, the default value should be None, not False.

ahoying · 2017-03-28T15:39:50Z

google/cloud/security/common/util/log_util.py

+
+    # Determine whether to use cloud logger based on environment.
+    if should_use_cloud_logger is None:
+        should_use_cloud_logger = is_gce


Can this be simplified to not require the should_use_cloud_logger var and just the following.

if FLAGS.use_cloud_logging or (FLAGS.use_cloud_logging is None and is_gce):
...

ahoying · 2017-03-28T15:45:56Z

google/cloud/security/common/util/log_util.py

+        An instance of the configured logger.
+    """
+    formatter = logging.Formatter(LOG_FORMAT)
+    console_handler = logging.StreamHandler()


Should we default to SysLogHandler instead? Or maybe use logging.config.fileConfig and include a default logging configuration that can be overridden by changing the configuration file rather than changing the code?

ahoying · 2017-03-28T15:53:59Z

google/cloud/security/common/gcp_api/cloud_logging.py

+import logging
+
+from google.cloud.security.common.gcp_api._base_client import _BaseClient
+# pylint: disable=line-too-long


You need to re-enable this check after the imports to catch the other places in this file where line is too long.

ahoying · 2017-03-28T15:55:14Z

google/cloud/security/common/gcp_api/cloud_logging.py

+    # pylint: disable=line-too-long
+    """Cloud Logging Handler.
+
+    See: https://github.com/GoogleCloudPlatform/google-cloud-python/blob/master/logging/google/cloud/logging/handlers/handlers.py


Is there an issue with just directly using google-cloud-python instead of importing pieces of it? How do we get patches / updates if there are bug fixes?

+1 on not importing pieces of code like this if we can avoid it. But the issue is different versions of oauth2client being used. Have other people done something similar to here?

mirons-google

Nice! Thanks for working through this one.

mirons-google · 2017-03-28T15:55:55Z

google/cloud/security/common/gcp_api/cloud_logging.py

+
+Most of this code has been lifted from:
+
+https://github.com/GoogleCloudPlatform/google-cloud-python/blob/master/logging/google/cloud/logging


Does licensing allow this? Should we just add to third_party?

mirons-google · 2017-03-28T15:56:30Z

google/cloud/security/common/gcp_api/cloud_logging.py

+    """Convert a timestamp to a string.
+
+    Args:
+        :type value: :class:`datetime.datetime`


nit: make pydoc match to the rest of the code-base. Fix throughout please.

mirons-google · 2017-03-28T15:59:14Z

google/cloud/security/common/gcp_api/compute.py

 from google.cloud.security.common.gcp_api._base_client import _BaseClient

+def is_compute_engine_instance():


Do callers need the error msg or can we simplify this by just returning a boolean?

mirons-google · 2017-03-28T16:09:23Z

google/cloud/security/common/third_party/transports/base.py

+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+"""Module containing base class for logging transport."""


Why don't we just pip install google-cloud-logging and use it as a dependency?

googleapiclient and google-cloud-python don't play together at all. installing both breaks the entire environment.

mirons-google · 2017-03-28T16:10:58Z

google/cloud/security/common/util/log_util.py

+    console_handler.setFormatter(formatter)
+    logger_instance = logging.getLogger(module_name)
+    logger_instance.addHandler(console_handler)
+    logger_instance.setLevel(logging.INFO)


We might want to look into making this an argument at execution.

mirons-google · 2017-03-28T16:11:37Z

google/cloud/security/common/util/log_util.py

+    Args:
+        module_name: The name of the module for which to configure logging.
+    """
+    (is_gce, _) = compute.is_compute_engine_instance()


See my comment above about not needing the error message. I suggest just returning the boolean result.

mirons-google · 2017-03-28T16:12:35Z

google/cloud/security/common/util/log_util.py

+        logger_instance = logging.getLogger(module_name)
+        logger_instance.addHandler(cloud_handler)
+
+        if FLAGS.debug:


Why isn't this done in get_logger? It could be confusing to debug the code to understand where this is getting set if we set verbosity in two different spots.

you can't use flags until they are parsed, which happens in app.run(). So the strategy is to attach the default log handler at the toplevel (e.g. module import) and then attach the stackdriver handler after app.run().

carise · 2017-03-31T05:03:16Z

I haven't cleaned up all the imports yet, mostly the ones that were nearby the log_util import.

Changed logger to use the SysLogHandler and removed the cloud logging related stuff. PTAL.

coveralls · 2017-04-05T01:35:54Z

Coverage increased (+0.2%) to 65.867% when pulling 06d2cb5 on configure-logging into 8e614ba on master.

carise · 2017-04-05T01:43:57Z

tests/inventory/pipelines/base_pipeline_test.py

@@ -87,11 +88,11 @@ def test_get_loaded_count(self):
    def test_error_is_handled_in_get_loaded_count(self):
        """Test error from get_loaded_count is handled."""

-        self.pipeline.logger = mock.create_autospec(
-            LogUtil).setup_logging('foo')
+        base_pipeline.LOGGER = mock.create_autospec(


@blueandgold FYI I tweaked your pipeline unit tests to reference the [MODULE_NAME].LOGGER property and made the pipeline.logger property a global "LOGGER" variable to be consistent across the codebase. Let me know if this looks ok; I'm not totally sure what's the best way here.

No problem if that works. 👍

coveralls · 2017-04-06T00:10:03Z

Coverage increased (+0.2%) to 66.167% when pulling f2876d7 on configure-logging into 7d4757d on master.

blueandgold · 2019-11-29T17:54:54Z

Adding cla: yes label, as the PR author is a googler at the time of this PR was created.

carise added 22 commits March 7, 2017 15:17

Initial

28267f6

tweak

3e8656a

google-cloud-logging and associated library doesn't work nicely with …

0ba7629

…google-api-client-python

Merge branch 'master' into configure-logging

e89562b

Merge branch 'master' into configure-logging

448333d

Merge branch 'master' into configure-logging

dac1774

Merge branch 'master' into configure-logging

213648b

Updates

2b36aa0

Merging changes

0c92ec9

revert the changes

cf4344e

get something working

2bae8c0

Tons of changes to clean up logger

4aa49f9

Always log to stdout and conditionally log to cloud

e0d50f3

lint fixes

6ccd88c

merge

fb779f2

make lint happier hopefully

3d25ade

allow globals because gflags makes me sad

44fc551

fix error

45609d3

Merge branch 'master' into configure-logging

dd4550d

Refactor log initialization to add the cloud logging handler in runne…

0a27ce4

…r script main()

Properly attach the cloud handler and fix lint

c6ed49f

lint

3137bb9

carise requested review from ahoying, blueandgold and mirons-google March 28, 2017 02:43

mirons-google reviewed Mar 28, 2017

View reviewed changes

ahoying reviewed Mar 28, 2017

View reviewed changes

mirons-google reviewed Mar 28, 2017

View reviewed changes

Merge branch 'master' into configure-logging

51c02ca

carise added 8 commits March 30, 2017 14:59

Logs

4b5a7e5

fixes

519d2aa

fixes

a9d1913

revert

10cb69f

remove stuff

4ca31e4

remove old stuff

8601676

fix

2982d37

fix lint

f1b3c57

blueandgold approved these changes Apr 4, 2017

View reviewed changes

carise added 2 commits April 4, 2017 18:19

fix merge

c054718

fix misc errors

06d2cb5

carise commented Apr 5, 2017

View reviewed changes

Merge branch 'master' into configure-logging

f2876d7

carise merged commit 009f57d into master Apr 6, 2017

carise deleted the configure-logging branch April 6, 2017 00:15

carise changed the title ~~Configure logging to log to Stackdriver (when flag is set)~~ Configure logging to log to Stackdriver Aug 31, 2017

blueandgold added the cla: yes label Nov 29, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Configure logging to log to Stackdriver #155

Configure logging to log to Stackdriver #155

carise commented Mar 25, 2017 •

edited

mirons-google Mar 28, 2017

carise Mar 28, 2017

mirons-google Mar 28, 2017

ahoying left a comment

ahoying Mar 28, 2017

ahoying Mar 28, 2017

ahoying Mar 28, 2017

ahoying Mar 28, 2017

ahoying Mar 28, 2017

ahoying Mar 28, 2017

ahoying Mar 28, 2017

blueandgold Mar 28, 2017

mirons-google left a comment

mirons-google Mar 28, 2017

mirons-google Mar 28, 2017

mirons-google Mar 28, 2017

mirons-google Mar 28, 2017

carise Mar 28, 2017

mirons-google Mar 28, 2017

mirons-google Mar 28, 2017

mirons-google Mar 28, 2017

carise Mar 28, 2017

carise commented Mar 31, 2017

coveralls commented Apr 5, 2017

carise Apr 5, 2017

blueandgold Apr 6, 2017

coveralls commented Apr 6, 2017

blueandgold commented Nov 29, 2019

		from google.cloud.security.common.gcp_api._base_client import _BaseClient

		def is_compute_engine_instance():


		Most of this code has been lifted from:

		https://github.com/GoogleCloudPlatform/google-cloud-python/blob/master/logging/google/cloud/logging

Configure logging to log to Stackdriver #155

Configure logging to log to Stackdriver #155

Conversation

carise commented Mar 25, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahoying left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mirons-google left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carise commented Mar 31, 2017

coveralls commented Apr 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Apr 6, 2017

blueandgold commented Nov 29, 2019

carise commented Mar 25, 2017 •

edited