From 9a20be984352489804230ed3a91aa9b459405a45 Mon Sep 17 00:00:00 2001 From: Angela Li Date: Mon, 22 May 2017 16:44:45 -0700 Subject: [PATCH 01/47] Send trace context with logs from web applications --- .../google/cloud/logging/handlers/_helpers.py | 39 ++++++++++++++++++ .../cloud/logging/handlers/app_engine.py | 18 ++++++++- .../google/cloud/logging/handlers/handlers.py | 12 +++++- .../logging/handlers/middleware/__init__.py | 17 ++++++++ .../logging/handlers/middleware/request.py | 40 +++++++++++++++++++ .../handlers/transports/background_thread.py | 13 ++++-- .../cloud/logging/handlers/transports/base.py | 5 ++- .../cloud/logging/handlers/transports/sync.py | 11 ++++- 8 files changed, 146 insertions(+), 9 deletions(-) create mode 100644 logging/google/cloud/logging/handlers/middleware/__init__.py create mode 100644 logging/google/cloud/logging/handlers/middleware/request.py diff --git a/logging/google/cloud/logging/handlers/_helpers.py b/logging/google/cloud/logging/handlers/_helpers.py index 81adcf0eb545..97b44d53ec19 100644 --- a/logging/google/cloud/logging/handlers/_helpers.py +++ b/logging/google/cloud/logging/handlers/_helpers.py @@ -17,6 +17,23 @@ import math import json +try: + import django +except ImportError: + _USE_DJANGO = False +else: + _USE_DJANGO = True + +try: + import flask +except ImportError: + _USE_FLASK = False + flask = None +else: + _USE_FLASK = True + +from google.cloud.logging.handlers.middleware.request import get_request + def format_stackdriver_json(record, message): """Helper to format a LogRecord in in Stackdriver fluentd format. @@ -37,3 +54,25 @@ def format_stackdriver_json(record, message): } return json.dumps(payload) + + +def get_trace_id_from_request_header(): + """Helper to get trace_id from web application request header. + + :rtype: str + :returns: Trace_id in HTTP request headers. + """ + if _USE_FLASK: + try: + trace_id = flask.request.headers['X_CLOUD_TRACE_CONTEXT'].split('/')[0] + except Exception: + trace_id = None + elif _USE_DJANGO: + try: + request = get_request() + trace_id = request.META['HTTP_X_CLOUD_TRACE_CONTEXT'].split('/')[0] + except Exception: + trace_id = None + else: + trace_id = None + return trace_id diff --git a/logging/google/cloud/logging/handlers/app_engine.py b/logging/google/cloud/logging/handlers/app_engine.py index c7394f32262d..6860060e071f 100644 --- a/logging/google/cloud/logging/handlers/app_engine.py +++ b/logging/google/cloud/logging/handlers/app_engine.py @@ -20,6 +20,7 @@ import os +from google.cloud.logging.handlers._helpers import get_trace_id_from_request_header from google.cloud.logging.handlers.handlers import CloudLoggingHandler from google.cloud.logging.handlers.transports import BackgroundThreadTransport from google.cloud.logging.resource import Resource @@ -50,7 +51,8 @@ def __init__(self, client, client, name=_DEFAULT_GAE_LOGGER_NAME, transport=transport, - resource=self.get_gae_resource()) + resource=self.get_gae_resource(), + labels=self.get_gae_labels()) def get_gae_resource(self): """Return the GAE resource using the environment variables. @@ -67,3 +69,17 @@ def get_gae_resource(self): }, ) return gae_resource + + def get_gae_labels(self): + """Return the labels for GAE app which includes trace_id. + + :rtype: dict + :returns: Labels for GAE app. + """ + trace_id = get_trace_id_from_request_header() + if trace_id is None: + trace_id = 'unknown' + gae_labels = { + 'appengine.googleapis.com/trace_id': trace_id, + } + return gae_labels \ No newline at end of file diff --git a/logging/google/cloud/logging/handlers/handlers.py b/logging/google/cloud/logging/handlers/handlers.py index 2269c2858f33..b45b4f593bc2 100644 --- a/logging/google/cloud/logging/handlers/handlers.py +++ b/logging/google/cloud/logging/handlers/handlers.py @@ -57,6 +57,9 @@ class CloudLoggingHandler(logging.StreamHandler): :param resource: (Optional) Monitored resource of the entry, defaults to the global resource type. + :type labels: dict + :param labels: (Optional) Mapping of labels for the entry. + Example: .. code-block:: python @@ -79,12 +82,14 @@ class CloudLoggingHandler(logging.StreamHandler): def __init__(self, client, name=DEFAULT_LOGGER_NAME, transport=BackgroundThreadTransport, - resource=_GLOBAL_RESOURCE): + resource=_GLOBAL_RESOURCE, + labels=None): super(CloudLoggingHandler, self).__init__() self.name = name self.client = client self.transport = transport(client, name) self.resource = resource + self.labels = labels def emit(self, record): """Actually log the specified logging record. @@ -97,7 +102,10 @@ def emit(self, record): :param record: The record to be logged. """ message = super(CloudLoggingHandler, self).format(record) - self.transport.send(record, message, resource=self.resource) + self.transport.send(record, + message, + resource=self.resource, + labels=self.labels) def setup_logging(handler, excluded_loggers=EXCLUDED_LOGGER_DEFAULTS, diff --git a/logging/google/cloud/logging/handlers/middleware/__init__.py b/logging/google/cloud/logging/handlers/middleware/__init__.py new file mode 100644 index 000000000000..79c3abee7894 --- /dev/null +++ b/logging/google/cloud/logging/handlers/middleware/__init__.py @@ -0,0 +1,17 @@ +# Copyright 2016 Google Inc. All Rights Reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +from google.cloud.logging.handlers.middleware.request import RequestMiddleware + +__all__ = ['RequestMiddleware'] diff --git a/logging/google/cloud/logging/handlers/middleware/request.py b/logging/google/cloud/logging/handlers/middleware/request.py new file mode 100644 index 000000000000..0c6d1f46c5cd --- /dev/null +++ b/logging/google/cloud/logging/handlers/middleware/request.py @@ -0,0 +1,40 @@ +# Copyright 2016 Google Inc. All Rights Reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +try: + from threading import local +except ImportError: + from django.utils._threading_local import local + +_thread_locals = local() + + +def get_request(): + """Get Django request from thread local. + + :rtype: str + :returns: Django request. + """ + return getattr(_thread_locals, 'request', None) + +class RequestMiddleware(object): + """Saves the request in thread local""" + + def process_request(self, request): + """Called on each request, before Django decides which view to execute. + + :type request: :class:`~django.http.request.HttpRequest` + :param request: Django http request. + """ + _thread_locals.request = request diff --git a/logging/google/cloud/logging/handlers/transports/background_thread.py b/logging/google/cloud/logging/handlers/transports/background_thread.py index 010c06b36bc9..d889bed62626 100644 --- a/logging/google/cloud/logging/handlers/transports/background_thread.py +++ b/logging/google/cloud/logging/handlers/transports/background_thread.py @@ -203,7 +203,7 @@ def _main_thread_terminated(self): else: print('Failed to send %d pending logs.' % (self._queue.qsize(),)) - def enqueue(self, record, message, resource=None): + def enqueue(self, record, message, resource=None, labels=None): """Queues a log entry to be written by the background thread. :type record: :class:`logging.LogRecord` @@ -215,6 +215,9 @@ def enqueue(self, record, message, resource=None): :type resource: :class:`~google.cloud.logging.resource.Resource` :param resource: (Optional) Monitored resource of the entry + + :type labels: dict + :param labels: (Optional) Mapping of labels for the entry. """ self._queue.put_nowait({ 'info': { @@ -223,6 +226,7 @@ def enqueue(self, record, message, resource=None): }, 'severity': record.levelname, 'resource': resource, + 'labels': labels, }) def flush(self): @@ -257,7 +261,7 @@ def __init__(self, client, name, grace_period=_DEFAULT_GRACE_PERIOD, self.worker = _Worker(logger) self.worker.start() - def send(self, record, message, resource=None): + def send(self, record, message, resource=None, labels=None): """Overrides Transport.send(). :type record: :class:`logging.LogRecord` @@ -269,8 +273,11 @@ def send(self, record, message, resource=None): :type resource: :class:`~google.cloud.logging.resource.Resource` :param resource: (Optional) Monitored resource of the entry. + + :type labels: dict + :param labels: (Optional) Mapping of labels for the entry. """ - self.worker.enqueue(record, message, resource=resource) + self.worker.enqueue(record, message, resource=resource, labels=labels) def flush(self): """Submit any pending log records.""" diff --git a/logging/google/cloud/logging/handlers/transports/base.py b/logging/google/cloud/logging/handlers/transports/base.py index 21957021793f..7829201b1c98 100644 --- a/logging/google/cloud/logging/handlers/transports/base.py +++ b/logging/google/cloud/logging/handlers/transports/base.py @@ -22,7 +22,7 @@ class Transport(object): client and name object, and must override :meth:`send`. """ - def send(self, record, message, resource=None): + def send(self, record, message, resource=None, labels=None): """Transport send to be implemented by subclasses. :type record: :class:`logging.LogRecord` @@ -34,6 +34,9 @@ def send(self, record, message, resource=None): :type resource: :class:`~google.cloud.logging.resource.Resource` :param resource: (Optional) Monitored resource of the entry. + + :type labels: dict + :param labels: (Optional) Mapping of labels for the entry. """ raise NotImplementedError diff --git a/logging/google/cloud/logging/handlers/transports/sync.py b/logging/google/cloud/logging/handlers/transports/sync.py index 0dd6e0bd7e24..be70e60a14e1 100644 --- a/logging/google/cloud/logging/handlers/transports/sync.py +++ b/logging/google/cloud/logging/handlers/transports/sync.py @@ -29,7 +29,7 @@ class SyncTransport(Transport): def __init__(self, client, name): self.logger = client.logger(name) - def send(self, record, message, resource=None): + def send(self, record, message, resource=None, labels=None): """Overrides transport.send(). :type record: :class:`logging.LogRecord` @@ -38,8 +38,15 @@ def send(self, record, message, resource=None): :type message: str :param message: The message from the ``LogRecord`` after being formatted by the associated log formatters. + + :type resource: :class:`~google.cloud.logging.resource.Resource` + :param resource: (Optional) Monitored resource of the entry. + + :type labels: dict + :param labels: (Optional) Mapping of labels for the entry. """ info = {'message': message, 'python_logger': record.name} self.logger.log_struct(info, severity=record.levelname, - resource=resource) + resource=resource, + labels=labels) From cf283c4e4983ea6077506c5d8abe928a8b7bb1d1 Mon Sep 17 00:00:00 2001 From: Angela Li Date: Mon, 22 May 2017 16:58:43 -0700 Subject: [PATCH 02/47] Fix style --- logging/google/cloud/logging/handlers/app_engine.py | 2 +- logging/google/cloud/logging/handlers/middleware/request.py | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/logging/google/cloud/logging/handlers/app_engine.py b/logging/google/cloud/logging/handlers/app_engine.py index 6860060e071f..ea4cc9245282 100644 --- a/logging/google/cloud/logging/handlers/app_engine.py +++ b/logging/google/cloud/logging/handlers/app_engine.py @@ -82,4 +82,4 @@ def get_gae_labels(self): gae_labels = { 'appengine.googleapis.com/trace_id': trace_id, } - return gae_labels \ No newline at end of file + return gae_labels diff --git a/logging/google/cloud/logging/handlers/middleware/request.py b/logging/google/cloud/logging/handlers/middleware/request.py index 0c6d1f46c5cd..a37a6af47035 100644 --- a/logging/google/cloud/logging/handlers/middleware/request.py +++ b/logging/google/cloud/logging/handlers/middleware/request.py @@ -28,6 +28,7 @@ def get_request(): """ return getattr(_thread_locals, 'request', None) + class RequestMiddleware(object): """Saves the request in thread local""" From 583d076f53d1c10eba3633e24c83a00e4e612598 Mon Sep 17 00:00:00 2001 From: Angela Li Date: Tue, 23 May 2017 14:23:32 -0700 Subject: [PATCH 03/47] Improved code for web framework detection --- .../google/cloud/logging/handlers/_helpers.py | 37 ++++++++++++++----- 1 file changed, 27 insertions(+), 10 deletions(-) diff --git a/logging/google/cloud/logging/handlers/_helpers.py b/logging/google/cloud/logging/handlers/_helpers.py index 97b44d53ec19..a1c697ba404c 100644 --- a/logging/google/cloud/logging/handlers/_helpers.py +++ b/logging/google/cloud/logging/handlers/_helpers.py @@ -16,24 +16,18 @@ import math import json - -try: - import django -except ImportError: - _USE_DJANGO = False -else: - _USE_DJANGO = True +import sys try: import flask except ImportError: - _USE_FLASK = False flask = None -else: - _USE_FLASK = True from google.cloud.logging.handlers.middleware.request import get_request +_USE_FLASK = False +_USE_DJANGO = False + def format_stackdriver_json(record, message): """Helper to format a LogRecord in in Stackdriver fluentd format. @@ -56,12 +50,35 @@ def format_stackdriver_json(record, message): return json.dumps(payload) +def detect_web_framework(): + """Detect web framework used in this environment. + + Detect which web framework is used by looking at the sys.modules. + If multiple or no supported web frameworks detected in the modules, then + print a warning message. + Set _USE_FLASK or _USE_DJANGO to True if one of them are detected. + """ + modules = sys.modules + global _USE_FLASK, _USE_DJANGO + + if 'flask' in modules and 'django' in modules: + print('Cannot determine, found multiple web frameworks.') + elif 'flask' in modules: + _USE_FLASK = True + elif 'django' in modules: + _USE_DJANGO = True + else: + print('No supported web framework found in the modules.') + + def get_trace_id_from_request_header(): """Helper to get trace_id from web application request header. :rtype: str :returns: Trace_id in HTTP request headers. """ + detect_web_framework() + if _USE_FLASK: try: trace_id = flask.request.headers['X_CLOUD_TRACE_CONTEXT'].split('/')[0] From 5be368e1410dbff2a2f4009f3887ffbdfb06c139 Mon Sep 17 00:00:00 2001 From: Angela Li Date: Tue, 23 May 2017 14:28:01 -0700 Subject: [PATCH 04/47] Fix year --- logging/google/cloud/logging/handlers/middleware/__init__.py | 2 +- logging/google/cloud/logging/handlers/middleware/request.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/logging/google/cloud/logging/handlers/middleware/__init__.py b/logging/google/cloud/logging/handlers/middleware/__init__.py index 79c3abee7894..c340235b8bdd 100644 --- a/logging/google/cloud/logging/handlers/middleware/__init__.py +++ b/logging/google/cloud/logging/handlers/middleware/__init__.py @@ -1,4 +1,4 @@ -# Copyright 2016 Google Inc. All Rights Reserved. +# Copyright 2017 Google Inc. All Rights Reserved. # # Licensed under the Apache License, Version 2.0 (the "License"); # you may not use this file except in compliance with the License. diff --git a/logging/google/cloud/logging/handlers/middleware/request.py b/logging/google/cloud/logging/handlers/middleware/request.py index a37a6af47035..be5102487cc6 100644 --- a/logging/google/cloud/logging/handlers/middleware/request.py +++ b/logging/google/cloud/logging/handlers/middleware/request.py @@ -1,4 +1,4 @@ -# Copyright 2016 Google Inc. All Rights Reserved. +# Copyright 2017 Google Inc. All Rights Reserved. # # Licensed under the Apache License, Version 2.0 (the "License"); # you may not use this file except in compliance with the License. From 28e3a6c119b1b2ddaaf7cac45d39ab6e5c21a2b7 Mon Sep 17 00:00:00 2001 From: Angela Li Date: Tue, 23 May 2017 14:45:28 -0700 Subject: [PATCH 05/47] Drop module level variables --- .../google/cloud/logging/handlers/_helpers.py | 22 ++++++++++--------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/logging/google/cloud/logging/handlers/_helpers.py b/logging/google/cloud/logging/handlers/_helpers.py index a1c697ba404c..6bf0edf79d54 100644 --- a/logging/google/cloud/logging/handlers/_helpers.py +++ b/logging/google/cloud/logging/handlers/_helpers.py @@ -25,9 +25,6 @@ from google.cloud.logging.handlers.middleware.request import get_request -_USE_FLASK = False -_USE_DJANGO = False - def format_stackdriver_json(record, message): """Helper to format a LogRecord in in Stackdriver fluentd format. @@ -56,19 +53,24 @@ def detect_web_framework(): Detect which web framework is used by looking at the sys.modules. If multiple or no supported web frameworks detected in the modules, then print a warning message. - Set _USE_FLASK or _USE_DJANGO to True if one of them are detected. + Return the name of web framework detected if flask or django is found. + Return unknown if cannot determine. + + :rtype: str + :returns: Web framework detected in this environment. """ modules = sys.modules - global _USE_FLASK, _USE_DJANGO + web_framework = 'unknown' if 'flask' in modules and 'django' in modules: print('Cannot determine, found multiple web frameworks.') elif 'flask' in modules: - _USE_FLASK = True + web_framework = 'flask' elif 'django' in modules: - _USE_DJANGO = True + web_framework = 'django' else: print('No supported web framework found in the modules.') + return web_framework def get_trace_id_from_request_header(): @@ -77,14 +79,14 @@ def get_trace_id_from_request_header(): :rtype: str :returns: Trace_id in HTTP request headers. """ - detect_web_framework() + web_framework = detect_web_framework() - if _USE_FLASK: + if web_framework is 'flask': try: trace_id = flask.request.headers['X_CLOUD_TRACE_CONTEXT'].split('/')[0] except Exception: trace_id = None - elif _USE_DJANGO: + elif web_framework is 'django': try: request = get_request() trace_id = request.META['HTTP_X_CLOUD_TRACE_CONTEXT'].split('/')[0] From cdc96b4fcc6668a56af66ea46d482bf47942e4a4 Mon Sep 17 00:00:00 2001 From: Angela Li Date: Thu, 25 May 2017 10:07:25 -0700 Subject: [PATCH 06/47] Address Jon's comments and add some unit tests (not complete yet) --- .../google/cloud/logging/handlers/_helpers.py | 66 ++++++++----------- .../cloud/logging/handlers/app_engine.py | 4 +- .../google/cloud/logging/handlers/handlers.py | 9 +-- .../logging/handlers/middleware/request.py | 6 +- .../tests/unit/handlers/test_app_engine.py | 11 +++- logging/tests/unit/handlers/test_handlers.py | 6 +- .../transports/test_background_thread.py | 8 +-- .../unit/handlers/transports/test_sync.py | 6 +- 8 files changed, 56 insertions(+), 60 deletions(-) diff --git a/logging/google/cloud/logging/handlers/_helpers.py b/logging/google/cloud/logging/handlers/_helpers.py index 6bf0edf79d54..0af7a53de476 100644 --- a/logging/google/cloud/logging/handlers/_helpers.py +++ b/logging/google/cloud/logging/handlers/_helpers.py @@ -16,7 +16,6 @@ import math import json -import sys try: import flask @@ -47,51 +46,44 @@ def format_stackdriver_json(record, message): return json.dumps(payload) -def detect_web_framework(): - """Detect web framework used in this environment. - - Detect which web framework is used by looking at the sys.modules. - If multiple or no supported web frameworks detected in the modules, then - print a warning message. - Return the name of web framework detected if flask or django is found. - Return unknown if cannot determine. +def get_trace_id_from_flask(): + """Get trace_id from flask request headers. :rtype: str - :returns: Web framework detected in this environment. + :return: Trace_id in HTTP request headers. """ - modules = sys.modules - web_framework = 'unknown' + try: + trace_id = flask.request.headers['X_CLOUD_TRACE_CONTEXT'].split('/')[0] + except Exception: + trace_id = None + return trace_id - if 'flask' in modules and 'django' in modules: - print('Cannot determine, found multiple web frameworks.') - elif 'flask' in modules: - web_framework = 'flask' - elif 'django' in modules: - web_framework = 'django' - else: - print('No supported web framework found in the modules.') - return web_framework + +def get_trace_id_from_django(): + """Get trace_id from django request headers. + + :rtype: str + :return: Trace_id in HTTP request headers. + """ + try: + request = get_request() + trace_id = request.META['HTTP_X_CLOUD_TRACE_CONTEXT'].split('/')[0] + except Exception: + trace_id = None + return trace_id -def get_trace_id_from_request_header(): +def get_trace_id(): """Helper to get trace_id from web application request header. :rtype: str :returns: Trace_id in HTTP request headers. """ - web_framework = detect_web_framework() - - if web_framework is 'flask': - try: - trace_id = flask.request.headers['X_CLOUD_TRACE_CONTEXT'].split('/')[0] - except Exception: - trace_id = None - elif web_framework is 'django': - try: - request = get_request() - trace_id = request.META['HTTP_X_CLOUD_TRACE_CONTEXT'].split('/')[0] - except Exception: - trace_id = None - else: - trace_id = None + checkers = [get_trace_id_from_django, get_trace_id_from_flask] + + for checker in checkers: + trace_id = checker() + if trace_id is not None: + return trace_id + return trace_id diff --git a/logging/google/cloud/logging/handlers/app_engine.py b/logging/google/cloud/logging/handlers/app_engine.py index ea4cc9245282..7bf4790e0603 100644 --- a/logging/google/cloud/logging/handlers/app_engine.py +++ b/logging/google/cloud/logging/handlers/app_engine.py @@ -20,7 +20,7 @@ import os -from google.cloud.logging.handlers._helpers import get_trace_id_from_request_header +from google.cloud.logging.handlers._helpers import get_trace_id from google.cloud.logging.handlers.handlers import CloudLoggingHandler from google.cloud.logging.handlers.transports import BackgroundThreadTransport from google.cloud.logging.resource import Resource @@ -76,7 +76,7 @@ def get_gae_labels(self): :rtype: dict :returns: Labels for GAE app. """ - trace_id = get_trace_id_from_request_header() + trace_id = get_trace_id() if trace_id is None: trace_id = 'unknown' gae_labels = { diff --git a/logging/google/cloud/logging/handlers/handlers.py b/logging/google/cloud/logging/handlers/handlers.py index b45b4f593bc2..26343ec9f9c5 100644 --- a/logging/google/cloud/logging/handlers/handlers.py +++ b/logging/google/cloud/logging/handlers/handlers.py @@ -102,10 +102,11 @@ def emit(self, record): :param record: The record to be logged. """ message = super(CloudLoggingHandler, self).format(record) - self.transport.send(record, - message, - resource=self.resource, - labels=self.labels) + self.transport.send( + record, + message, + resource=self.resource, + labels=self.labels) def setup_logging(handler, excluded_loggers=EXCLUDED_LOGGER_DEFAULTS, diff --git a/logging/google/cloud/logging/handlers/middleware/request.py b/logging/google/cloud/logging/handlers/middleware/request.py index be5102487cc6..92bfd4369b00 100644 --- a/logging/google/cloud/logging/handlers/middleware/request.py +++ b/logging/google/cloud/logging/handlers/middleware/request.py @@ -12,10 +12,8 @@ # See the License for the specific language governing permissions and # limitations under the License. -try: - from threading import local -except ImportError: - from django.utils._threading_local import local +from threading import local + _thread_locals = local() diff --git a/logging/tests/unit/handlers/test_app_engine.py b/logging/tests/unit/handlers/test_app_engine.py index c39328593f7a..99f5ea93625a 100644 --- a/logging/tests/unit/handlers/test_app_engine.py +++ b/logging/tests/unit/handlers/test_app_engine.py @@ -34,6 +34,7 @@ def test_constructor(self): from google.cloud.logging.handlers.app_engine import _GAE_VERSION_ENV client = mock.Mock(project=self.PROJECT, spec=['project']) + with mock.patch('os.environ', new={_GAE_PROJECT_ENV: 'test_project', _GAE_SERVICE_ENV: 'test_service', _GAE_VERSION_ENV: 'test_version'}): @@ -43,6 +44,7 @@ def test_constructor(self): self.assertEqual(handler.resource.labels['project_id'], 'test_project') self.assertEqual(handler.resource.labels['module_id'], 'test_service') self.assertEqual(handler.resource.labels['version_id'], 'test_version') + self.assertEqual(handler.labels['appengine.googleapis.com/trace_id'], 'unknown') def test_emit(self): import mock @@ -50,6 +52,7 @@ def test_emit(self): client = mock.Mock(project=self.PROJECT, spec=['project']) handler = self._make_one(client, transport=_Transport) gae_resource = handler.get_gae_resource() + gae_labels = handler.get_gae_labels() logname = 'app' message = 'hello world' record = logging.LogRecord(logname, logging, None, None, message, @@ -58,7 +61,9 @@ def test_emit(self): self.assertIs(handler.transport.client, client) self.assertEqual(handler.transport.name, logname) - self.assertEqual(handler.transport.send_called_with, (record, message, gae_resource)) + self.assertEqual( + handler.transport.send_called_with, + (record, message, gae_resource, gae_labels)) class _Transport(object): @@ -67,5 +72,5 @@ def __init__(self, client, name): self.client = client self.name = name - def send(self, record, message, resource): - self.send_called_with = (record, message, resource) + def send(self, record, message, resource, labels): + self.send_called_with = (record, message, resource, labels) diff --git a/logging/tests/unit/handlers/test_handlers.py b/logging/tests/unit/handlers/test_handlers.py index 05dc87631478..5459a0c10b96 100644 --- a/logging/tests/unit/handlers/test_handlers.py +++ b/logging/tests/unit/handlers/test_handlers.py @@ -45,7 +45,7 @@ def test_emit(self): None, None) handler.emit(record) - self.assertEqual(handler.transport.send_called_with, (record, message, _GLOBAL_RESOURCE)) + self.assertEqual(handler.transport.send_called_with, (record, message, _GLOBAL_RESOURCE, None)) class TestSetupLogging(unittest.TestCase): @@ -110,5 +110,5 @@ class _Transport(object): def __init__(self, client, name): pass - def send(self, record, message, resource): - self.send_called_with = (record, message, resource) + def send(self, record, message, resource, labels=None): + self.send_called_with = (record, message, resource, labels) diff --git a/logging/tests/unit/handlers/transports/test_background_thread.py b/logging/tests/unit/handlers/transports/test_background_thread.py index 3e3378dcd361..b9449ce56685 100644 --- a/logging/tests/unit/handlers/transports/test_background_thread.py +++ b/logging/tests/unit/handlers/transports/test_background_thread.py @@ -61,9 +61,9 @@ def test_send(self): python_logger_name, logging.INFO, None, None, message, None, None) - transport.send(record, message, _GLOBAL_RESOURCE) + transport.send(record, message, _GLOBAL_RESOURCE, None) - transport.worker.enqueue.assert_called_once_with(record, message, _GLOBAL_RESOURCE) + transport.worker.enqueue.assert_called_once_with(record, message, _GLOBAL_RESOURCE, None) def test_flush(self): client = _Client(self.PROJECT) @@ -287,13 +287,13 @@ def __init__(self): self.commit_called = False self.commit_count = None - def log_struct(self, info, severity=logging.INFO, resource=None): + def log_struct(self, info, severity=logging.INFO, resource=None, labels=None): from google.cloud.logging.logger import _GLOBAL_RESOURCE assert resource is None resource = _GLOBAL_RESOURCE - self.log_struct_called_with = (info, severity, resource) + self.log_struct_called_with = (info, severity, resource, labels) self.entries.append(info) def commit(self): diff --git a/logging/tests/unit/handlers/transports/test_sync.py b/logging/tests/unit/handlers/transports/test_sync.py index 475ecc9c6a71..e18920ce2167 100644 --- a/logging/tests/unit/handlers/transports/test_sync.py +++ b/logging/tests/unit/handlers/transports/test_sync.py @@ -52,7 +52,7 @@ def test_send(self): 'message': message, 'python_logger': python_logger_name, } - EXPECTED_SENT = (EXPECTED_STRUCT, 'INFO', _GLOBAL_RESOURCE) + EXPECTED_SENT = (EXPECTED_STRUCT, 'INFO', _GLOBAL_RESOURCE, None) self.assertEqual( transport.logger.log_struct_called_with, EXPECTED_SENT) @@ -63,8 +63,8 @@ class _Logger(object): def __init__(self, name): self.name = name - def log_struct(self, message, severity=None, resource=_GLOBAL_RESOURCE): - self.log_struct_called_with = (message, severity, resource) + def log_struct(self, message, severity=None, resource=_GLOBAL_RESOURCE, labels=None): + self.log_struct_called_with = (message, severity, resource, labels) class _Client(object): From fb83ab6e9cf556bd2c7a0063cc38a3ed5c4d933c Mon Sep 17 00:00:00 2001 From: Angela Li Date: Thu, 25 May 2017 10:45:36 -0700 Subject: [PATCH 07/47] Fix stuff --- .../google/cloud/logging/handlers/_helpers.py | 35 ++++++++++++++----- .../logging/handlers/middleware/request.py | 17 +++++---- .../tests/unit/handlers/test_app_engine.py | 4 ++- 3 files changed, 37 insertions(+), 19 deletions(-) diff --git a/logging/google/cloud/logging/handlers/_helpers.py b/logging/google/cloud/logging/handlers/_helpers.py index 0af7a53de476..ca94e17ebb92 100644 --- a/logging/google/cloud/logging/handlers/_helpers.py +++ b/logging/google/cloud/logging/handlers/_helpers.py @@ -22,7 +22,10 @@ except ImportError: flask = None -from google.cloud.logging.handlers.middleware.request import get_request +from google.cloud.logging.handlers.middleware.request import RequestMiddleware + +_FLASK_TRACE_HEADER = 'X_CLOUD_TRACE_CONTEXT' +_DJANGO_TRACE_HEADER = 'HTTP_X_CLOUD_TRACE_CONTEXT' def format_stackdriver_json(record, message): @@ -52,10 +55,16 @@ def get_trace_id_from_flask(): :rtype: str :return: Trace_id in HTTP request headers. """ - try: - trace_id = flask.request.headers['X_CLOUD_TRACE_CONTEXT'].split('/')[0] - except Exception: - trace_id = None + if not flask or not flask.request: + return None + + header = flask.request.headers.get(_FLASK_TRACE_HEADER) + + if not header: + return None + + trace_id = header.split('/')[0] + return trace_id @@ -65,11 +74,19 @@ def get_trace_id_from_django(): :rtype: str :return: Trace_id in HTTP request headers. """ + request_middleware = RequestMiddleware() + request = request_middleware.get_request() + + if not request: + return None + try: - request = get_request() - trace_id = request.META['HTTP_X_CLOUD_TRACE_CONTEXT'].split('/')[0] - except Exception: - trace_id = None + header = request.META[_DJANGO_TRACE_HEADER] + except KeyError: + return None + + trace_id = header.split('/')[0] + return trace_id diff --git a/logging/google/cloud/logging/handlers/middleware/request.py b/logging/google/cloud/logging/handlers/middleware/request.py index 92bfd4369b00..7e0e89f6aa39 100644 --- a/logging/google/cloud/logging/handlers/middleware/request.py +++ b/logging/google/cloud/logging/handlers/middleware/request.py @@ -18,15 +18,6 @@ _thread_locals = local() -def get_request(): - """Get Django request from thread local. - - :rtype: str - :returns: Django request. - """ - return getattr(_thread_locals, 'request', None) - - class RequestMiddleware(object): """Saves the request in thread local""" @@ -37,3 +28,11 @@ def process_request(self, request): :param request: Django http request. """ _thread_locals.request = request + + def get_request(self): + """Get Django request from thread local. + + :rtype: str + :returns: Django request. + """ + return getattr(_thread_locals, 'request', None) diff --git a/logging/tests/unit/handlers/test_app_engine.py b/logging/tests/unit/handlers/test_app_engine.py index 99f5ea93625a..0f8ace3a95e7 100644 --- a/logging/tests/unit/handlers/test_app_engine.py +++ b/logging/tests/unit/handlers/test_app_engine.py @@ -33,6 +33,8 @@ def test_constructor(self): from google.cloud.logging.handlers.app_engine import _GAE_SERVICE_ENV from google.cloud.logging.handlers.app_engine import _GAE_VERSION_ENV + TRACE_ID_LABEL = 'appengine.googleapis.com/trace_id' + client = mock.Mock(project=self.PROJECT, spec=['project']) with mock.patch('os.environ', new={_GAE_PROJECT_ENV: 'test_project', @@ -44,7 +46,7 @@ def test_constructor(self): self.assertEqual(handler.resource.labels['project_id'], 'test_project') self.assertEqual(handler.resource.labels['module_id'], 'test_service') self.assertEqual(handler.resource.labels['version_id'], 'test_version') - self.assertEqual(handler.labels['appengine.googleapis.com/trace_id'], 'unknown') + self.assertEqual(handler.labels[TRACE_ID_LABEL], 'unknown') def test_emit(self): import mock From 9a20c4e013b7f8c03f83447fd604bdf5ce5bc9e6 Mon Sep 17 00:00:00 2001 From: Angela Li Date: Thu, 25 May 2017 14:54:23 -0700 Subject: [PATCH 08/47] Add unit test for flask and some refactor --- .../google/cloud/logging/handlers/_helpers.py | 12 ++-- .../cloud/logging/handlers/app_engine.py | 2 - logging/nox.py | 2 +- logging/tests/unit/handlers/test__helpers.py | 57 +++++++++++++++++++ .../tests/unit/handlers/test_app_engine.py | 3 +- 5 files changed, 67 insertions(+), 9 deletions(-) create mode 100644 logging/tests/unit/handlers/test__helpers.py diff --git a/logging/google/cloud/logging/handlers/_helpers.py b/logging/google/cloud/logging/handlers/_helpers.py index ca94e17ebb92..17caf76e4aa9 100644 --- a/logging/google/cloud/logging/handlers/_helpers.py +++ b/logging/google/cloud/logging/handlers/_helpers.py @@ -27,6 +27,8 @@ _FLASK_TRACE_HEADER = 'X_CLOUD_TRACE_CONTEXT' _DJANGO_TRACE_HEADER = 'HTTP_X_CLOUD_TRACE_CONTEXT' +_EMPTY_TRACE_ID = 'None' + def format_stackdriver_json(record, message): """Helper to format a LogRecord in in Stackdriver fluentd format. @@ -56,12 +58,12 @@ def get_trace_id_from_flask(): :return: Trace_id in HTTP request headers. """ if not flask or not flask.request: - return None + return _EMPTY_TRACE_ID header = flask.request.headers.get(_FLASK_TRACE_HEADER) if not header: - return None + return _EMPTY_TRACE_ID trace_id = header.split('/')[0] @@ -78,12 +80,12 @@ def get_trace_id_from_django(): request = request_middleware.get_request() if not request: - return None + return _EMPTY_TRACE_ID try: header = request.META[_DJANGO_TRACE_HEADER] except KeyError: - return None + return _EMPTY_TRACE_ID trace_id = header.split('/')[0] @@ -100,7 +102,7 @@ def get_trace_id(): for checker in checkers: trace_id = checker() - if trace_id is not None: + if trace_id is not _EMPTY_TRACE_ID: return trace_id return trace_id diff --git a/logging/google/cloud/logging/handlers/app_engine.py b/logging/google/cloud/logging/handlers/app_engine.py index 7bf4790e0603..daa48132e246 100644 --- a/logging/google/cloud/logging/handlers/app_engine.py +++ b/logging/google/cloud/logging/handlers/app_engine.py @@ -77,8 +77,6 @@ def get_gae_labels(self): :returns: Labels for GAE app. """ trace_id = get_trace_id() - if trace_id is None: - trace_id = 'unknown' gae_labels = { 'appengine.googleapis.com/trace_id': trace_id, } diff --git a/logging/nox.py b/logging/nox.py index 5d4751a955a5..11d484b232e7 100644 --- a/logging/nox.py +++ b/logging/nox.py @@ -31,7 +31,7 @@ def unit_tests(session, python_version): session.interpreter = 'python{}'.format(python_version) # Install all test dependencies, then install this package in-place. - session.install('mock', 'pytest', 'pytest-cov', *LOCAL_DEPS) + session.install('mock', 'pytest', 'pytest-cov', 'flask', *LOCAL_DEPS) session.install('-e', '.') # Run py.test against the unit tests. diff --git a/logging/tests/unit/handlers/test__helpers.py b/logging/tests/unit/handlers/test__helpers.py new file mode 100644 index 000000000000..db9d51d27920 --- /dev/null +++ b/logging/tests/unit/handlers/test__helpers.py @@ -0,0 +1,57 @@ +# Copyright 2017 Google Inc. All Rights Reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +import unittest + + +class TestFlaskTrace(unittest.TestCase): + + def create_app(self): + import flask + + app = flask.Flask(__name__) + + @app.route('/') + def index(): + return 'test flask trace' + + return app + + def setUp(self): + self.app = self.create_app() + + def test_trace_id_no_context_header(self): + from google.cloud.logging.handlers._helpers import get_trace_id_from_flask + from google.cloud.logging.handlers._helpers import _EMPTY_TRACE_ID + + with self.app.test_request_context( + path='/', + headers={}): + trace_id = get_trace_id_from_flask() + + self.assertEqual(trace_id, _EMPTY_TRACE_ID) + + def test_trace_id_valid_context_header(self): + from google.cloud.logging.handlers._helpers import get_trace_id_from_flask + + FLASK_TRACE_HEADER = 'X_CLOUD_TRACE_CONTEXT' + FLASK_TRACE_ID = 'testtraceid/testspanid' + + with self.app.test_request_context( + path='/', + headers={FLASK_TRACE_HEADER:FLASK_TRACE_ID}): + trace_id = get_trace_id_from_flask() + + EXPECTED_TRACE_ID = 'testtraceid' + self.assertEqual(trace_id, EXPECTED_TRACE_ID) diff --git a/logging/tests/unit/handlers/test_app_engine.py b/logging/tests/unit/handlers/test_app_engine.py index 0f8ace3a95e7..886dbbbb5584 100644 --- a/logging/tests/unit/handlers/test_app_engine.py +++ b/logging/tests/unit/handlers/test_app_engine.py @@ -32,6 +32,7 @@ def test_constructor(self): from google.cloud.logging.handlers.app_engine import _GAE_PROJECT_ENV from google.cloud.logging.handlers.app_engine import _GAE_SERVICE_ENV from google.cloud.logging.handlers.app_engine import _GAE_VERSION_ENV + from google.cloud.logging.handlers._helpers import _EMPTY_TRACE_ID TRACE_ID_LABEL = 'appengine.googleapis.com/trace_id' @@ -46,7 +47,7 @@ def test_constructor(self): self.assertEqual(handler.resource.labels['project_id'], 'test_project') self.assertEqual(handler.resource.labels['module_id'], 'test_service') self.assertEqual(handler.resource.labels['version_id'], 'test_version') - self.assertEqual(handler.labels[TRACE_ID_LABEL], 'unknown') + self.assertEqual(handler.labels[TRACE_ID_LABEL], _EMPTY_TRACE_ID) def test_emit(self): import mock From f8b7e65acb7e1229252588264f16445f859011af Mon Sep 17 00:00:00 2001 From: Angela Li Date: Fri, 26 May 2017 10:20:56 -0700 Subject: [PATCH 09/47] Add unit test for middleware --- logging/nox.py | 2 +- .../unit/handlers/middleware/test_request.py | 39 +++++++++++++++++++ 2 files changed, 40 insertions(+), 1 deletion(-) create mode 100644 logging/tests/unit/handlers/middleware/test_request.py diff --git a/logging/nox.py b/logging/nox.py index 11d484b232e7..de975600fd04 100644 --- a/logging/nox.py +++ b/logging/nox.py @@ -31,7 +31,7 @@ def unit_tests(session, python_version): session.interpreter = 'python{}'.format(python_version) # Install all test dependencies, then install this package in-place. - session.install('mock', 'pytest', 'pytest-cov', 'flask', *LOCAL_DEPS) + session.install('mock', 'pytest', 'pytest-cov', 'flask', 'django', *LOCAL_DEPS) session.install('-e', '.') # Run py.test against the unit tests. diff --git a/logging/tests/unit/handlers/middleware/test_request.py b/logging/tests/unit/handlers/middleware/test_request.py new file mode 100644 index 000000000000..ad784b45c48e --- /dev/null +++ b/logging/tests/unit/handlers/middleware/test_request.py @@ -0,0 +1,39 @@ +# Copyright 2017 Google Inc. All Rights Reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +import unittest + + +class TestRequestMiddleware(unittest.TestCase): + + def _get_target_class(self): + from google.cloud.logging.handlers.middleware.request import RequestMiddleware + + return RequestMiddleware + + def _make_one(self, *args, **kw): + return self._get_target_class()(*args, **kw) + + def test_get_request(self): + from django.conf import settings + from django.test.utils import setup_test_environment + from django.test import RequestFactory + + settings.configure() + setup_test_environment() + + self.middleware = self._make_one() + self.request = RequestFactory().get('/') + self.middleware.process_request(self.request) + self.assertEqual(self.middleware.get_request(), self.request) From 65efdcd79fbcae278c0ad925c367d902b408f9ab Mon Sep 17 00:00:00 2001 From: Angela Li Date: Fri, 26 May 2017 11:25:41 -0700 Subject: [PATCH 10/47] Add unit test for django --- .../unit/handlers/middleware/test_request.py | 15 ++++- logging/tests/unit/handlers/test__helpers.py | 67 ++++++++++++++++++- 2 files changed, 77 insertions(+), 5 deletions(-) diff --git a/logging/tests/unit/handlers/middleware/test_request.py b/logging/tests/unit/handlers/middleware/test_request.py index ad784b45c48e..5b6dfca8dc67 100644 --- a/logging/tests/unit/handlers/middleware/test_request.py +++ b/logging/tests/unit/handlers/middleware/test_request.py @@ -25,15 +25,24 @@ def _get_target_class(self): def _make_one(self, *args, **kw): return self._get_target_class()(*args, **kw) - def test_get_request(self): + def setUp(self): from django.conf import settings from django.test.utils import setup_test_environment - from django.test import RequestFactory - settings.configure() + if not settings.configured: + settings.configure() setup_test_environment() + def test_get_request(self): + from django.test import RequestFactory + self.middleware = self._make_one() self.request = RequestFactory().get('/') self.middleware.process_request(self.request) self.assertEqual(self.middleware.get_request(), self.request) + + + def tearDown(self): + from django.test.utils import teardown_test_environment + + teardown_test_environment() diff --git a/logging/tests/unit/handlers/test__helpers.py b/logging/tests/unit/handlers/test__helpers.py index db9d51d27920..3d023dd8f9cf 100644 --- a/logging/tests/unit/handlers/test__helpers.py +++ b/logging/tests/unit/handlers/test__helpers.py @@ -33,25 +33,88 @@ def setUp(self): def test_trace_id_no_context_header(self): from google.cloud.logging.handlers._helpers import get_trace_id_from_flask + from google.cloud.logging.handlers._helpers import get_trace_id from google.cloud.logging.handlers._helpers import _EMPTY_TRACE_ID with self.app.test_request_context( path='/', headers={}): trace_id = get_trace_id_from_flask() + trace_id_returned = get_trace_id() self.assertEqual(trace_id, _EMPTY_TRACE_ID) + self.assertEqual(trace_id_returned, _EMPTY_TRACE_ID) def test_trace_id_valid_context_header(self): from google.cloud.logging.handlers._helpers import get_trace_id_from_flask + from google.cloud.logging.handlers._helpers import get_trace_id FLASK_TRACE_HEADER = 'X_CLOUD_TRACE_CONTEXT' - FLASK_TRACE_ID = 'testtraceid/testspanid' + FLASK_TRACE_ID = 'testtraceidflask/testspanid' with self.app.test_request_context( path='/', headers={FLASK_TRACE_HEADER:FLASK_TRACE_ID}): trace_id = get_trace_id_from_flask() + trace_id_returned = get_trace_id() - EXPECTED_TRACE_ID = 'testtraceid' + EXPECTED_TRACE_ID = 'testtraceidflask' self.assertEqual(trace_id, EXPECTED_TRACE_ID) + self.assertEqual(trace_id, trace_id_returned) + + +class TestDjangoTrace(unittest.TestCase): + + def setUp(self): + from django.conf import settings + from django.test.utils import setup_test_environment + + if not settings.configured: + settings.configure() + setup_test_environment() + + def test_trace_id_no_context_header(self): + import mock + from django.test import RequestFactory + from google.cloud.logging.handlers._helpers import get_trace_id_from_django + from google.cloud.logging.handlers._helpers import get_trace_id + from google.cloud.logging.handlers._helpers import _EMPTY_TRACE_ID + + request = RequestFactory().get('/') + + with mock.patch( + 'google.cloud.logging.handlers.middleware.RequestMiddleware.get_request', + return_value=request): + trace_id = get_trace_id_from_django() + trace_id_returned = get_trace_id() + + self.assertEqual(trace_id, _EMPTY_TRACE_ID) + self.assertEqual(trace_id_returned, _EMPTY_TRACE_ID) + + def test_trace_id_valid_context_header(self): + import mock + from django.test import RequestFactory + from google.cloud.logging.handlers._helpers import get_trace_id_from_django + from google.cloud.logging.handlers._helpers import get_trace_id + + DJANGO_TRACE_HEADER = 'HTTP_X_CLOUD_TRACE_CONTEXT' + DJANGO_TRACE_ID = 'testtraceiddjango/testspanid' + + request = RequestFactory().get( + '/', + **{DJANGO_TRACE_HEADER:DJANGO_TRACE_ID}) + + with mock.patch( + 'google.cloud.logging.handlers.middleware.RequestMiddleware.get_request', + return_value=request): + trace_id = get_trace_id_from_django() + trace_id_returned = get_trace_id() + + EXPECTED_TRACE_ID = 'testtraceiddjango' + self.assertEqual(trace_id, EXPECTED_TRACE_ID) + self.assertEqual(trace_id, trace_id_returned) + + def tearDown(self): + from django.test.utils import teardown_test_environment + + teardown_test_environment() From f78cc2e91905f41d86fe6251ce8c340cad3c74b1 Mon Sep 17 00:00:00 2001 From: Angela Li Date: Fri, 26 May 2017 11:29:54 -0700 Subject: [PATCH 11/47] Fix style --- logging/tests/unit/handlers/middleware/test_request.py | 1 - 1 file changed, 1 deletion(-) diff --git a/logging/tests/unit/handlers/middleware/test_request.py b/logging/tests/unit/handlers/middleware/test_request.py index 5b6dfca8dc67..0aa950263f5d 100644 --- a/logging/tests/unit/handlers/middleware/test_request.py +++ b/logging/tests/unit/handlers/middleware/test_request.py @@ -41,7 +41,6 @@ def test_get_request(self): self.middleware.process_request(self.request) self.assertEqual(self.middleware.get_request(), self.request) - def tearDown(self): from django.test.utils import teardown_test_environment From 33b92dd16640b574e956dc0a070a081bb041ff54 Mon Sep 17 00:00:00 2001 From: Angela Li Date: Fri, 26 May 2017 14:38:45 -0700 Subject: [PATCH 12/47] Address jon's comments --- .../google/cloud/logging/handlers/_helpers.py | 17 ++--- .../cloud/logging/handlers/app_engine.py | 10 ++- .../logging/handlers/middleware/request.py | 17 ++--- .../unit/handlers/middleware/test_request.py | 13 ++-- logging/tests/unit/handlers/test__helpers.py | 66 ++++++++++--------- .../tests/unit/handlers/test_app_engine.py | 6 +- 6 files changed, 66 insertions(+), 63 deletions(-) diff --git a/logging/google/cloud/logging/handlers/_helpers.py b/logging/google/cloud/logging/handlers/_helpers.py index 17caf76e4aa9..2c88f427f00b 100644 --- a/logging/google/cloud/logging/handlers/_helpers.py +++ b/logging/google/cloud/logging/handlers/_helpers.py @@ -22,13 +22,11 @@ except ImportError: flask = None -from google.cloud.logging.handlers.middleware.request import RequestMiddleware +from google.cloud.logging.handlers.middleware.request import _get_request _FLASK_TRACE_HEADER = 'X_CLOUD_TRACE_CONTEXT' _DJANGO_TRACE_HEADER = 'HTTP_X_CLOUD_TRACE_CONTEXT' -_EMPTY_TRACE_ID = 'None' - def format_stackdriver_json(record, message): """Helper to format a LogRecord in in Stackdriver fluentd format. @@ -58,12 +56,12 @@ def get_trace_id_from_flask(): :return: Trace_id in HTTP request headers. """ if not flask or not flask.request: - return _EMPTY_TRACE_ID + return None header = flask.request.headers.get(_FLASK_TRACE_HEADER) if not header: - return _EMPTY_TRACE_ID + return None trace_id = header.split('/')[0] @@ -76,16 +74,15 @@ def get_trace_id_from_django(): :rtype: str :return: Trace_id in HTTP request headers. """ - request_middleware = RequestMiddleware() - request = request_middleware.get_request() + request = _get_request() if not request: - return _EMPTY_TRACE_ID + return None try: header = request.META[_DJANGO_TRACE_HEADER] except KeyError: - return _EMPTY_TRACE_ID + return None trace_id = header.split('/')[0] @@ -102,7 +99,7 @@ def get_trace_id(): for checker in checkers: trace_id = checker() - if trace_id is not _EMPTY_TRACE_ID: + if trace_id is not None: return trace_id return trace_id diff --git a/logging/google/cloud/logging/handlers/app_engine.py b/logging/google/cloud/logging/handlers/app_engine.py index daa48132e246..f8b12c44a42e 100644 --- a/logging/google/cloud/logging/handlers/app_engine.py +++ b/logging/google/cloud/logging/handlers/app_engine.py @@ -31,6 +31,8 @@ _GAE_SERVICE_ENV = 'GAE_SERVICE' _GAE_VERSION_ENV = 'GAE_VERSION' +_TRACE_ID_LABEL = 'appengine.googleapis.com/trace_id' + class AppEngineHandler(CloudLoggingHandler): """A logging handler that sends App Engine-formatted logs to Stackdriver. @@ -76,8 +78,10 @@ def get_gae_labels(self): :rtype: dict :returns: Labels for GAE app. """ + gae_labels = {} + trace_id = get_trace_id() - gae_labels = { - 'appengine.googleapis.com/trace_id': trace_id, - } + if trace_id is not None: + gae_labels[_TRACE_ID_LABEL] = trace_id + return gae_labels diff --git a/logging/google/cloud/logging/handlers/middleware/request.py b/logging/google/cloud/logging/handlers/middleware/request.py index 7e0e89f6aa39..9baf11f4dbbe 100644 --- a/logging/google/cloud/logging/handlers/middleware/request.py +++ b/logging/google/cloud/logging/handlers/middleware/request.py @@ -18,6 +18,15 @@ _thread_locals = local() +def _get_request(): + """Get Django request from thread local. + + :rtype: str + :returns: Django request. + """ + return getattr(_thread_locals, 'request', None) + + class RequestMiddleware(object): """Saves the request in thread local""" @@ -28,11 +37,3 @@ def process_request(self, request): :param request: Django http request. """ _thread_locals.request = request - - def get_request(self): - """Get Django request from thread local. - - :rtype: str - :returns: Django request. - """ - return getattr(_thread_locals, 'request', None) diff --git a/logging/tests/unit/handlers/middleware/test_request.py b/logging/tests/unit/handlers/middleware/test_request.py index 0aa950263f5d..349247805104 100644 --- a/logging/tests/unit/handlers/middleware/test_request.py +++ b/logging/tests/unit/handlers/middleware/test_request.py @@ -33,15 +33,16 @@ def setUp(self): settings.configure() setup_test_environment() + def tearDown(self): + from django.test.utils import teardown_test_environment + + teardown_test_environment() + def test_get_request(self): from django.test import RequestFactory + from google.cloud.logging.handlers.middleware.request import _get_request self.middleware = self._make_one() self.request = RequestFactory().get('/') self.middleware.process_request(self.request) - self.assertEqual(self.middleware.get_request(), self.request) - - def tearDown(self): - from django.test.utils import teardown_test_environment - - teardown_test_environment() + self.assertEqual(_get_request(), self.request) diff --git a/logging/tests/unit/handlers/test__helpers.py b/logging/tests/unit/handlers/test__helpers.py index 3d023dd8f9cf..634ceb26e8a6 100644 --- a/logging/tests/unit/handlers/test__helpers.py +++ b/logging/tests/unit/handlers/test__helpers.py @@ -34,7 +34,6 @@ def setUp(self): def test_trace_id_no_context_header(self): from google.cloud.logging.handlers._helpers import get_trace_id_from_flask from google.cloud.logging.handlers._helpers import get_trace_id - from google.cloud.logging.handlers._helpers import _EMPTY_TRACE_ID with self.app.test_request_context( path='/', @@ -42,24 +41,24 @@ def test_trace_id_no_context_header(self): trace_id = get_trace_id_from_flask() trace_id_returned = get_trace_id() - self.assertEqual(trace_id, _EMPTY_TRACE_ID) - self.assertEqual(trace_id_returned, _EMPTY_TRACE_ID) + self.assertEqual(trace_id, None) + self.assertEqual(trace_id_returned, None) def test_trace_id_valid_context_header(self): from google.cloud.logging.handlers._helpers import get_trace_id_from_flask from google.cloud.logging.handlers._helpers import get_trace_id - FLASK_TRACE_HEADER = 'X_CLOUD_TRACE_CONTEXT' - FLASK_TRACE_ID = 'testtraceidflask/testspanid' + flask_trace_header = 'X_CLOUD_TRACE_CONTEXT' + flask_trace_id = 'testtraceidflask/testspanid' with self.app.test_request_context( path='/', - headers={FLASK_TRACE_HEADER:FLASK_TRACE_ID}): + headers={flask_trace_header:flask_trace_id}): trace_id = get_trace_id_from_flask() trace_id_returned = get_trace_id() - EXPECTED_TRACE_ID = 'testtraceidflask' - self.assertEqual(trace_id, EXPECTED_TRACE_ID) + expected_trace_id = 'testtraceidflask' + self.assertEqual(trace_id, expected_trace_id) self.assertEqual(trace_id, trace_id_returned) @@ -73,48 +72,51 @@ def setUp(self): settings.configure() setup_test_environment() + def tearDown(self): + from django.test.utils import teardown_test_environment + + teardown_test_environment() + def test_trace_id_no_context_header(self): - import mock from django.test import RequestFactory from google.cloud.logging.handlers._helpers import get_trace_id_from_django from google.cloud.logging.handlers._helpers import get_trace_id - from google.cloud.logging.handlers._helpers import _EMPTY_TRACE_ID + from google.cloud.logging.handlers.middleware.request import RequestMiddleware + from google.cloud.logging.handlers.middleware.request import _thread_locals request = RequestFactory().get('/') - with mock.patch( - 'google.cloud.logging.handlers.middleware.RequestMiddleware.get_request', - return_value=request): - trace_id = get_trace_id_from_django() - trace_id_returned = get_trace_id() + middleware = RequestMiddleware() + middleware.process_request(request) + trace_id = get_trace_id_from_django() + trace_id_returned = get_trace_id() - self.assertEqual(trace_id, _EMPTY_TRACE_ID) - self.assertEqual(trace_id_returned, _EMPTY_TRACE_ID) + self.assertEqual(trace_id, None) + self.assertEqual(trace_id_returned, None) + + _thread_locals.__dict__.clear() def test_trace_id_valid_context_header(self): - import mock from django.test import RequestFactory from google.cloud.logging.handlers._helpers import get_trace_id_from_django from google.cloud.logging.handlers._helpers import get_trace_id + from google.cloud.logging.handlers.middleware.request import RequestMiddleware + from google.cloud.logging.handlers.middleware.request import _thread_locals - DJANGO_TRACE_HEADER = 'HTTP_X_CLOUD_TRACE_CONTEXT' - DJANGO_TRACE_ID = 'testtraceiddjango/testspanid' + django_trace_header = 'HTTP_X_CLOUD_TRACE_CONTEXT' + django_trace_id = 'testtraceiddjango/testspanid' request = RequestFactory().get( '/', - **{DJANGO_TRACE_HEADER:DJANGO_TRACE_ID}) + **{django_trace_header:django_trace_id}) - with mock.patch( - 'google.cloud.logging.handlers.middleware.RequestMiddleware.get_request', - return_value=request): - trace_id = get_trace_id_from_django() - trace_id_returned = get_trace_id() + middleware = RequestMiddleware() + middleware.process_request(request) + trace_id = get_trace_id_from_django() + trace_id_returned = get_trace_id() - EXPECTED_TRACE_ID = 'testtraceiddjango' - self.assertEqual(trace_id, EXPECTED_TRACE_ID) + expected_trace_id = 'testtraceiddjango' + self.assertEqual(trace_id, expected_trace_id) self.assertEqual(trace_id, trace_id_returned) - def tearDown(self): - from django.test.utils import teardown_test_environment - - teardown_test_environment() + _thread_locals.__dict__.clear() diff --git a/logging/tests/unit/handlers/test_app_engine.py b/logging/tests/unit/handlers/test_app_engine.py index 886dbbbb5584..ccdaf2992335 100644 --- a/logging/tests/unit/handlers/test_app_engine.py +++ b/logging/tests/unit/handlers/test_app_engine.py @@ -32,9 +32,7 @@ def test_constructor(self): from google.cloud.logging.handlers.app_engine import _GAE_PROJECT_ENV from google.cloud.logging.handlers.app_engine import _GAE_SERVICE_ENV from google.cloud.logging.handlers.app_engine import _GAE_VERSION_ENV - from google.cloud.logging.handlers._helpers import _EMPTY_TRACE_ID - - TRACE_ID_LABEL = 'appengine.googleapis.com/trace_id' + from google.cloud.logging.handlers.app_engine import _TRACE_ID_LABEL client = mock.Mock(project=self.PROJECT, spec=['project']) @@ -47,7 +45,7 @@ def test_constructor(self): self.assertEqual(handler.resource.labels['project_id'], 'test_project') self.assertEqual(handler.resource.labels['module_id'], 'test_service') self.assertEqual(handler.resource.labels['version_id'], 'test_version') - self.assertEqual(handler.labels[TRACE_ID_LABEL], _EMPTY_TRACE_ID) + self.assertEqual(handler.labels, {}) def test_emit(self): import mock From 5cc798ffaf220a2b25b83020701cdfc82370eba2 Mon Sep 17 00:00:00 2001 From: Angela Li Date: Tue, 30 May 2017 12:28:30 -0700 Subject: [PATCH 13/47] Address jon's comments --- logging/google/cloud/logging/handlers/_helpers.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/logging/google/cloud/logging/handlers/_helpers.py b/logging/google/cloud/logging/handlers/_helpers.py index 2c88f427f00b..00135d0fe243 100644 --- a/logging/google/cloud/logging/handlers/_helpers.py +++ b/logging/google/cloud/logging/handlers/_helpers.py @@ -80,10 +80,12 @@ def get_trace_id_from_django(): return None try: - header = request.META[_DJANGO_TRACE_HEADER] + header = request.META.get(_DJANGO_TRACE_HEADER) except KeyError: return None + if not header: + return None trace_id = header.split('/')[0] return trace_id @@ -102,4 +104,4 @@ def get_trace_id(): if trace_id is not None: return trace_id - return trace_id + return None From 6ef3f49a10a139d6e773214c8f0bf940660713a0 Mon Sep 17 00:00:00 2001 From: Angela Li Date: Fri, 2 Jun 2017 00:28:10 -0700 Subject: [PATCH 14/47] Address all comments --- .../google/cloud/logging/handlers/_helpers.py | 14 ++--- .../logging/handlers/middleware/request.py | 6 +- logging/nox.py | 4 +- .../unit/handlers/middleware/test_request.py | 18 +++--- logging/tests/unit/handlers/test__helpers.py | 56 +++++++++---------- logging/tests/unit/handlers/test_handlers.py | 4 +- .../transports/test_background_thread.py | 3 +- .../unit/handlers/transports/test_sync.py | 3 +- 8 files changed, 58 insertions(+), 50 deletions(-) diff --git a/logging/google/cloud/logging/handlers/_helpers.py b/logging/google/cloud/logging/handlers/_helpers.py index 00135d0fe243..e5bdabe2e388 100644 --- a/logging/google/cloud/logging/handlers/_helpers.py +++ b/logging/google/cloud/logging/handlers/_helpers.py @@ -22,7 +22,7 @@ except ImportError: flask = None -from google.cloud.logging.handlers.middleware.request import _get_request +from google.cloud.logging.handlers.middleware.request import _get_django_request _FLASK_TRACE_HEADER = 'X_CLOUD_TRACE_CONTEXT' _DJANGO_TRACE_HEADER = 'HTTP_X_CLOUD_TRACE_CONTEXT' @@ -55,12 +55,12 @@ def get_trace_id_from_flask(): :rtype: str :return: Trace_id in HTTP request headers. """ - if not flask or not flask.request: + if flask is None or not flask.request: return None header = flask.request.headers.get(_FLASK_TRACE_HEADER) - if not header: + if header is None: return None trace_id = header.split('/')[0] @@ -74,9 +74,9 @@ def get_trace_id_from_django(): :rtype: str :return: Trace_id in HTTP request headers. """ - request = _get_request() + request = _get_django_request() - if not request: + if request is None: return None try: @@ -84,7 +84,7 @@ def get_trace_id_from_django(): except KeyError: return None - if not header: + if header is None: return None trace_id = header.split('/')[0] @@ -97,7 +97,7 @@ def get_trace_id(): :rtype: str :returns: Trace_id in HTTP request headers. """ - checkers = [get_trace_id_from_django, get_trace_id_from_flask] + checkers = (get_trace_id_from_django, get_trace_id_from_flask) for checker in checkers: trace_id = checker() diff --git a/logging/google/cloud/logging/handlers/middleware/request.py b/logging/google/cloud/logging/handlers/middleware/request.py index 9baf11f4dbbe..1acf7ffd9961 100644 --- a/logging/google/cloud/logging/handlers/middleware/request.py +++ b/logging/google/cloud/logging/handlers/middleware/request.py @@ -12,13 +12,13 @@ # See the License for the specific language governing permissions and # limitations under the License. -from threading import local +import threading -_thread_locals = local() +_thread_locals = threading.local() -def _get_request(): +def _get_django_request(): """Get Django request from thread local. :rtype: str diff --git a/logging/nox.py b/logging/nox.py index de975600fd04..fbbbec1958c1 100644 --- a/logging/nox.py +++ b/logging/nox.py @@ -31,7 +31,9 @@ def unit_tests(session, python_version): session.interpreter = 'python{}'.format(python_version) # Install all test dependencies, then install this package in-place. - session.install('mock', 'pytest', 'pytest-cov', 'flask', 'django', *LOCAL_DEPS) + session.install( + 'mock', 'pytest', 'pytest-cov', + 'flask', 'django', *LOCAL_DEPS) session.install('-e', '.') # Run py.test against the unit tests. diff --git a/logging/tests/unit/handlers/middleware/test_request.py b/logging/tests/unit/handlers/middleware/test_request.py index 349247805104..ab8c18d1aa0e 100644 --- a/logging/tests/unit/handlers/middleware/test_request.py +++ b/logging/tests/unit/handlers/middleware/test_request.py @@ -25,7 +25,8 @@ def _get_target_class(self): def _make_one(self, *args, **kw): return self._get_target_class()(*args, **kw) - def setUp(self): + @classmethod + def setUpClass(cls): from django.conf import settings from django.test.utils import setup_test_environment @@ -33,16 +34,17 @@ def setUp(self): settings.configure() setup_test_environment() - def tearDown(self): + @classmethod + def tearDownClass(cls): from django.test.utils import teardown_test_environment teardown_test_environment() - def test_get_request(self): + def test_get_django_request(self): from django.test import RequestFactory - from google.cloud.logging.handlers.middleware.request import _get_request + from google.cloud.logging.handlers.middleware.request import _get_django_request - self.middleware = self._make_one() - self.request = RequestFactory().get('/') - self.middleware.process_request(self.request) - self.assertEqual(_get_request(), self.request) + middleware = self._make_one() + request = RequestFactory().get('/') + middleware.process_request(request) + self.assertEqual(_get_django_request(), request) diff --git a/logging/tests/unit/handlers/test__helpers.py b/logging/tests/unit/handlers/test__helpers.py index 634ceb26e8a6..59e56f34524b 100644 --- a/logging/tests/unit/handlers/test__helpers.py +++ b/logging/tests/unit/handlers/test__helpers.py @@ -15,9 +15,10 @@ import unittest -class TestFlaskTrace(unittest.TestCase): +class Test_get_trace_id_from_flask(unittest.TestCase): - def create_app(self): + @staticmethod + def create_app(): import flask app = flask.Flask(__name__) @@ -29,40 +30,41 @@ def index(): return app def setUp(self): - self.app = self.create_app() + self.app = Test_get_trace_id_from_flask.create_app() def test_trace_id_no_context_header(self): - from google.cloud.logging.handlers._helpers import get_trace_id_from_flask - from google.cloud.logging.handlers._helpers import get_trace_id + from google.cloud.logging.handlers import _helpers with self.app.test_request_context( path='/', headers={}): - trace_id = get_trace_id_from_flask() - trace_id_returned = get_trace_id() + trace_id = _helpers.get_trace_id_from_flask() + trace_id_returned = _helpers.get_trace_id() - self.assertEqual(trace_id, None) - self.assertEqual(trace_id_returned, None) + self.assertIsNone(trace_id) + self.assertIsNone(trace_id_returned) def test_trace_id_valid_context_header(self): from google.cloud.logging.handlers._helpers import get_trace_id_from_flask from google.cloud.logging.handlers._helpers import get_trace_id flask_trace_header = 'X_CLOUD_TRACE_CONTEXT' - flask_trace_id = 'testtraceidflask/testspanid' + expected_trace_id = 'testtraceidflask' + flask_trace_id = expected_trace_id + '/testspanid' - with self.app.test_request_context( - path='/', - headers={flask_trace_header:flask_trace_id}): + context = self.app.test_request_context( + path='/', + headers={flask_trace_header: flask_trace_id}) + + with context: trace_id = get_trace_id_from_flask() trace_id_returned = get_trace_id() - expected_trace_id = 'testtraceidflask' self.assertEqual(trace_id, expected_trace_id) self.assertEqual(trace_id, trace_id_returned) -class TestDjangoTrace(unittest.TestCase): +class Test_get_trace_id_from_django(unittest.TestCase): def setUp(self): from django.conf import settings @@ -79,22 +81,20 @@ def tearDown(self): def test_trace_id_no_context_header(self): from django.test import RequestFactory - from google.cloud.logging.handlers._helpers import get_trace_id_from_django - from google.cloud.logging.handlers._helpers import get_trace_id - from google.cloud.logging.handlers.middleware.request import RequestMiddleware - from google.cloud.logging.handlers.middleware.request import _thread_locals + from google.cloud.logging.handlers import _helpers + from google.cloud.logging.handlers.middleware import request - request = RequestFactory().get('/') + django_request = RequestFactory().get('/') - middleware = RequestMiddleware() - middleware.process_request(request) - trace_id = get_trace_id_from_django() - trace_id_returned = get_trace_id() + middleware = request.RequestMiddleware() + middleware.process_request(django_request) + trace_id = _helpers.get_trace_id_from_django() + trace_id_returned = _helpers.get_trace_id() - self.assertEqual(trace_id, None) - self.assertEqual(trace_id_returned, None) + self.assertIsNone(trace_id) + self.assertIsNone(trace_id_returned) - _thread_locals.__dict__.clear() + request._thread_locals.__dict__.clear() def test_trace_id_valid_context_header(self): from django.test import RequestFactory @@ -108,7 +108,7 @@ def test_trace_id_valid_context_header(self): request = RequestFactory().get( '/', - **{django_trace_header:django_trace_id}) + **{django_trace_header: django_trace_id}) middleware = RequestMiddleware() middleware.process_request(request) diff --git a/logging/tests/unit/handlers/test_handlers.py b/logging/tests/unit/handlers/test_handlers.py index 5459a0c10b96..96823b2e906d 100644 --- a/logging/tests/unit/handlers/test_handlers.py +++ b/logging/tests/unit/handlers/test_handlers.py @@ -45,7 +45,9 @@ def test_emit(self): None, None) handler.emit(record) - self.assertEqual(handler.transport.send_called_with, (record, message, _GLOBAL_RESOURCE, None)) + self.assertEqual( + handler.transport.send_called_with, + (record, message, _GLOBAL_RESOURCE, None)) class TestSetupLogging(unittest.TestCase): diff --git a/logging/tests/unit/handlers/transports/test_background_thread.py b/logging/tests/unit/handlers/transports/test_background_thread.py index b9449ce56685..f6671273b53d 100644 --- a/logging/tests/unit/handlers/transports/test_background_thread.py +++ b/logging/tests/unit/handlers/transports/test_background_thread.py @@ -63,7 +63,8 @@ def test_send(self): transport.send(record, message, _GLOBAL_RESOURCE, None) - transport.worker.enqueue.assert_called_once_with(record, message, _GLOBAL_RESOURCE, None) + transport.worker.enqueue.assert_called_once_with( + record, message, _GLOBAL_RESOURCE, None) def test_flush(self): client = _Client(self.PROJECT) diff --git a/logging/tests/unit/handlers/transports/test_sync.py b/logging/tests/unit/handlers/transports/test_sync.py index e18920ce2167..01c15240f3b7 100644 --- a/logging/tests/unit/handlers/transports/test_sync.py +++ b/logging/tests/unit/handlers/transports/test_sync.py @@ -63,7 +63,8 @@ class _Logger(object): def __init__(self, name): self.name = name - def log_struct(self, message, severity=None, resource=_GLOBAL_RESOURCE, labels=None): + def log_struct(self, message, severity=None, + resource=_GLOBAL_RESOURCE, labels=None): self.log_struct_called_with = (message, severity, resource, labels) From 14c2545bc94fab6505305d6d4ccb4808c912a54d Mon Sep 17 00:00:00 2001 From: Angela Li Date: Fri, 2 Jun 2017 00:34:48 -0700 Subject: [PATCH 15/47] fix style --- logging/tests/unit/handlers/test__helpers.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/logging/tests/unit/handlers/test__helpers.py b/logging/tests/unit/handlers/test__helpers.py index 59e56f34524b..c50ce0044e85 100644 --- a/logging/tests/unit/handlers/test__helpers.py +++ b/logging/tests/unit/handlers/test__helpers.py @@ -104,7 +104,8 @@ def test_trace_id_valid_context_header(self): from google.cloud.logging.handlers.middleware.request import _thread_locals django_trace_header = 'HTTP_X_CLOUD_TRACE_CONTEXT' - django_trace_id = 'testtraceiddjango/testspanid' + expected_trace_id = 'testtraceiddjango' + django_trace_id = expected_trace_id + '/testspanid' request = RequestFactory().get( '/', @@ -115,7 +116,6 @@ def test_trace_id_valid_context_header(self): trace_id = get_trace_id_from_django() trace_id_returned = get_trace_id() - expected_trace_id = 'testtraceiddjango' self.assertEqual(trace_id, expected_trace_id) self.assertEqual(trace_id, trace_id_returned) From 199ada005c406f39924eb42e0329e66d5c023e42 Mon Sep 17 00:00:00 2001 From: Angela Li Date: Mon, 22 May 2017 16:44:45 -0700 Subject: [PATCH 16/47] Send trace context with logs from web applications --- .../google/cloud/logging/handlers/_helpers.py | 39 ++++++++++++++++++ .../cloud/logging/handlers/app_engine.py | 18 ++++++++- .../google/cloud/logging/handlers/handlers.py | 12 +++++- .../logging/handlers/middleware/__init__.py | 17 ++++++++ .../logging/handlers/middleware/request.py | 40 +++++++++++++++++++ .../handlers/transports/background_thread.py | 13 ++++-- .../cloud/logging/handlers/transports/base.py | 5 ++- .../cloud/logging/handlers/transports/sync.py | 11 ++++- 8 files changed, 146 insertions(+), 9 deletions(-) create mode 100644 logging/google/cloud/logging/handlers/middleware/__init__.py create mode 100644 logging/google/cloud/logging/handlers/middleware/request.py diff --git a/logging/google/cloud/logging/handlers/_helpers.py b/logging/google/cloud/logging/handlers/_helpers.py index 81adcf0eb545..97b44d53ec19 100644 --- a/logging/google/cloud/logging/handlers/_helpers.py +++ b/logging/google/cloud/logging/handlers/_helpers.py @@ -17,6 +17,23 @@ import math import json +try: + import django +except ImportError: + _USE_DJANGO = False +else: + _USE_DJANGO = True + +try: + import flask +except ImportError: + _USE_FLASK = False + flask = None +else: + _USE_FLASK = True + +from google.cloud.logging.handlers.middleware.request import get_request + def format_stackdriver_json(record, message): """Helper to format a LogRecord in in Stackdriver fluentd format. @@ -37,3 +54,25 @@ def format_stackdriver_json(record, message): } return json.dumps(payload) + + +def get_trace_id_from_request_header(): + """Helper to get trace_id from web application request header. + + :rtype: str + :returns: Trace_id in HTTP request headers. + """ + if _USE_FLASK: + try: + trace_id = flask.request.headers['X_CLOUD_TRACE_CONTEXT'].split('/')[0] + except Exception: + trace_id = None + elif _USE_DJANGO: + try: + request = get_request() + trace_id = request.META['HTTP_X_CLOUD_TRACE_CONTEXT'].split('/')[0] + except Exception: + trace_id = None + else: + trace_id = None + return trace_id diff --git a/logging/google/cloud/logging/handlers/app_engine.py b/logging/google/cloud/logging/handlers/app_engine.py index c7394f32262d..6860060e071f 100644 --- a/logging/google/cloud/logging/handlers/app_engine.py +++ b/logging/google/cloud/logging/handlers/app_engine.py @@ -20,6 +20,7 @@ import os +from google.cloud.logging.handlers._helpers import get_trace_id_from_request_header from google.cloud.logging.handlers.handlers import CloudLoggingHandler from google.cloud.logging.handlers.transports import BackgroundThreadTransport from google.cloud.logging.resource import Resource @@ -50,7 +51,8 @@ def __init__(self, client, client, name=_DEFAULT_GAE_LOGGER_NAME, transport=transport, - resource=self.get_gae_resource()) + resource=self.get_gae_resource(), + labels=self.get_gae_labels()) def get_gae_resource(self): """Return the GAE resource using the environment variables. @@ -67,3 +69,17 @@ def get_gae_resource(self): }, ) return gae_resource + + def get_gae_labels(self): + """Return the labels for GAE app which includes trace_id. + + :rtype: dict + :returns: Labels for GAE app. + """ + trace_id = get_trace_id_from_request_header() + if trace_id is None: + trace_id = 'unknown' + gae_labels = { + 'appengine.googleapis.com/trace_id': trace_id, + } + return gae_labels \ No newline at end of file diff --git a/logging/google/cloud/logging/handlers/handlers.py b/logging/google/cloud/logging/handlers/handlers.py index 2269c2858f33..b45b4f593bc2 100644 --- a/logging/google/cloud/logging/handlers/handlers.py +++ b/logging/google/cloud/logging/handlers/handlers.py @@ -57,6 +57,9 @@ class CloudLoggingHandler(logging.StreamHandler): :param resource: (Optional) Monitored resource of the entry, defaults to the global resource type. + :type labels: dict + :param labels: (Optional) Mapping of labels for the entry. + Example: .. code-block:: python @@ -79,12 +82,14 @@ class CloudLoggingHandler(logging.StreamHandler): def __init__(self, client, name=DEFAULT_LOGGER_NAME, transport=BackgroundThreadTransport, - resource=_GLOBAL_RESOURCE): + resource=_GLOBAL_RESOURCE, + labels=None): super(CloudLoggingHandler, self).__init__() self.name = name self.client = client self.transport = transport(client, name) self.resource = resource + self.labels = labels def emit(self, record): """Actually log the specified logging record. @@ -97,7 +102,10 @@ def emit(self, record): :param record: The record to be logged. """ message = super(CloudLoggingHandler, self).format(record) - self.transport.send(record, message, resource=self.resource) + self.transport.send(record, + message, + resource=self.resource, + labels=self.labels) def setup_logging(handler, excluded_loggers=EXCLUDED_LOGGER_DEFAULTS, diff --git a/logging/google/cloud/logging/handlers/middleware/__init__.py b/logging/google/cloud/logging/handlers/middleware/__init__.py new file mode 100644 index 000000000000..79c3abee7894 --- /dev/null +++ b/logging/google/cloud/logging/handlers/middleware/__init__.py @@ -0,0 +1,17 @@ +# Copyright 2016 Google Inc. All Rights Reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +from google.cloud.logging.handlers.middleware.request import RequestMiddleware + +__all__ = ['RequestMiddleware'] diff --git a/logging/google/cloud/logging/handlers/middleware/request.py b/logging/google/cloud/logging/handlers/middleware/request.py new file mode 100644 index 000000000000..0c6d1f46c5cd --- /dev/null +++ b/logging/google/cloud/logging/handlers/middleware/request.py @@ -0,0 +1,40 @@ +# Copyright 2016 Google Inc. All Rights Reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +try: + from threading import local +except ImportError: + from django.utils._threading_local import local + +_thread_locals = local() + + +def get_request(): + """Get Django request from thread local. + + :rtype: str + :returns: Django request. + """ + return getattr(_thread_locals, 'request', None) + +class RequestMiddleware(object): + """Saves the request in thread local""" + + def process_request(self, request): + """Called on each request, before Django decides which view to execute. + + :type request: :class:`~django.http.request.HttpRequest` + :param request: Django http request. + """ + _thread_locals.request = request diff --git a/logging/google/cloud/logging/handlers/transports/background_thread.py b/logging/google/cloud/logging/handlers/transports/background_thread.py index 010c06b36bc9..d889bed62626 100644 --- a/logging/google/cloud/logging/handlers/transports/background_thread.py +++ b/logging/google/cloud/logging/handlers/transports/background_thread.py @@ -203,7 +203,7 @@ def _main_thread_terminated(self): else: print('Failed to send %d pending logs.' % (self._queue.qsize(),)) - def enqueue(self, record, message, resource=None): + def enqueue(self, record, message, resource=None, labels=None): """Queues a log entry to be written by the background thread. :type record: :class:`logging.LogRecord` @@ -215,6 +215,9 @@ def enqueue(self, record, message, resource=None): :type resource: :class:`~google.cloud.logging.resource.Resource` :param resource: (Optional) Monitored resource of the entry + + :type labels: dict + :param labels: (Optional) Mapping of labels for the entry. """ self._queue.put_nowait({ 'info': { @@ -223,6 +226,7 @@ def enqueue(self, record, message, resource=None): }, 'severity': record.levelname, 'resource': resource, + 'labels': labels, }) def flush(self): @@ -257,7 +261,7 @@ def __init__(self, client, name, grace_period=_DEFAULT_GRACE_PERIOD, self.worker = _Worker(logger) self.worker.start() - def send(self, record, message, resource=None): + def send(self, record, message, resource=None, labels=None): """Overrides Transport.send(). :type record: :class:`logging.LogRecord` @@ -269,8 +273,11 @@ def send(self, record, message, resource=None): :type resource: :class:`~google.cloud.logging.resource.Resource` :param resource: (Optional) Monitored resource of the entry. + + :type labels: dict + :param labels: (Optional) Mapping of labels for the entry. """ - self.worker.enqueue(record, message, resource=resource) + self.worker.enqueue(record, message, resource=resource, labels=labels) def flush(self): """Submit any pending log records.""" diff --git a/logging/google/cloud/logging/handlers/transports/base.py b/logging/google/cloud/logging/handlers/transports/base.py index 21957021793f..7829201b1c98 100644 --- a/logging/google/cloud/logging/handlers/transports/base.py +++ b/logging/google/cloud/logging/handlers/transports/base.py @@ -22,7 +22,7 @@ class Transport(object): client and name object, and must override :meth:`send`. """ - def send(self, record, message, resource=None): + def send(self, record, message, resource=None, labels=None): """Transport send to be implemented by subclasses. :type record: :class:`logging.LogRecord` @@ -34,6 +34,9 @@ def send(self, record, message, resource=None): :type resource: :class:`~google.cloud.logging.resource.Resource` :param resource: (Optional) Monitored resource of the entry. + + :type labels: dict + :param labels: (Optional) Mapping of labels for the entry. """ raise NotImplementedError diff --git a/logging/google/cloud/logging/handlers/transports/sync.py b/logging/google/cloud/logging/handlers/transports/sync.py index 0dd6e0bd7e24..be70e60a14e1 100644 --- a/logging/google/cloud/logging/handlers/transports/sync.py +++ b/logging/google/cloud/logging/handlers/transports/sync.py @@ -29,7 +29,7 @@ class SyncTransport(Transport): def __init__(self, client, name): self.logger = client.logger(name) - def send(self, record, message, resource=None): + def send(self, record, message, resource=None, labels=None): """Overrides transport.send(). :type record: :class:`logging.LogRecord` @@ -38,8 +38,15 @@ def send(self, record, message, resource=None): :type message: str :param message: The message from the ``LogRecord`` after being formatted by the associated log formatters. + + :type resource: :class:`~google.cloud.logging.resource.Resource` + :param resource: (Optional) Monitored resource of the entry. + + :type labels: dict + :param labels: (Optional) Mapping of labels for the entry. """ info = {'message': message, 'python_logger': record.name} self.logger.log_struct(info, severity=record.levelname, - resource=resource) + resource=resource, + labels=labels) From 425c8ef46ce7b255be3433218ec6c42021c32afc Mon Sep 17 00:00:00 2001 From: Angela Li Date: Mon, 22 May 2017 16:58:43 -0700 Subject: [PATCH 17/47] Fix style --- logging/google/cloud/logging/handlers/app_engine.py | 2 +- logging/google/cloud/logging/handlers/middleware/request.py | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/logging/google/cloud/logging/handlers/app_engine.py b/logging/google/cloud/logging/handlers/app_engine.py index 6860060e071f..ea4cc9245282 100644 --- a/logging/google/cloud/logging/handlers/app_engine.py +++ b/logging/google/cloud/logging/handlers/app_engine.py @@ -82,4 +82,4 @@ def get_gae_labels(self): gae_labels = { 'appengine.googleapis.com/trace_id': trace_id, } - return gae_labels \ No newline at end of file + return gae_labels diff --git a/logging/google/cloud/logging/handlers/middleware/request.py b/logging/google/cloud/logging/handlers/middleware/request.py index 0c6d1f46c5cd..a37a6af47035 100644 --- a/logging/google/cloud/logging/handlers/middleware/request.py +++ b/logging/google/cloud/logging/handlers/middleware/request.py @@ -28,6 +28,7 @@ def get_request(): """ return getattr(_thread_locals, 'request', None) + class RequestMiddleware(object): """Saves the request in thread local""" From 1047fd5612756a03c8bce4e9bb3229a729a4641d Mon Sep 17 00:00:00 2001 From: Angela Li Date: Tue, 23 May 2017 14:23:32 -0700 Subject: [PATCH 18/47] Improved code for web framework detection --- .../google/cloud/logging/handlers/_helpers.py | 37 ++++++++++++++----- 1 file changed, 27 insertions(+), 10 deletions(-) diff --git a/logging/google/cloud/logging/handlers/_helpers.py b/logging/google/cloud/logging/handlers/_helpers.py index 97b44d53ec19..a1c697ba404c 100644 --- a/logging/google/cloud/logging/handlers/_helpers.py +++ b/logging/google/cloud/logging/handlers/_helpers.py @@ -16,24 +16,18 @@ import math import json - -try: - import django -except ImportError: - _USE_DJANGO = False -else: - _USE_DJANGO = True +import sys try: import flask except ImportError: - _USE_FLASK = False flask = None -else: - _USE_FLASK = True from google.cloud.logging.handlers.middleware.request import get_request +_USE_FLASK = False +_USE_DJANGO = False + def format_stackdriver_json(record, message): """Helper to format a LogRecord in in Stackdriver fluentd format. @@ -56,12 +50,35 @@ def format_stackdriver_json(record, message): return json.dumps(payload) +def detect_web_framework(): + """Detect web framework used in this environment. + + Detect which web framework is used by looking at the sys.modules. + If multiple or no supported web frameworks detected in the modules, then + print a warning message. + Set _USE_FLASK or _USE_DJANGO to True if one of them are detected. + """ + modules = sys.modules + global _USE_FLASK, _USE_DJANGO + + if 'flask' in modules and 'django' in modules: + print('Cannot determine, found multiple web frameworks.') + elif 'flask' in modules: + _USE_FLASK = True + elif 'django' in modules: + _USE_DJANGO = True + else: + print('No supported web framework found in the modules.') + + def get_trace_id_from_request_header(): """Helper to get trace_id from web application request header. :rtype: str :returns: Trace_id in HTTP request headers. """ + detect_web_framework() + if _USE_FLASK: try: trace_id = flask.request.headers['X_CLOUD_TRACE_CONTEXT'].split('/')[0] From 46012ebb2eb56306392fcfe84afbf8713588ec90 Mon Sep 17 00:00:00 2001 From: Angela Li Date: Tue, 23 May 2017 14:28:01 -0700 Subject: [PATCH 19/47] Fix year --- logging/google/cloud/logging/handlers/middleware/__init__.py | 2 +- logging/google/cloud/logging/handlers/middleware/request.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/logging/google/cloud/logging/handlers/middleware/__init__.py b/logging/google/cloud/logging/handlers/middleware/__init__.py index 79c3abee7894..c340235b8bdd 100644 --- a/logging/google/cloud/logging/handlers/middleware/__init__.py +++ b/logging/google/cloud/logging/handlers/middleware/__init__.py @@ -1,4 +1,4 @@ -# Copyright 2016 Google Inc. All Rights Reserved. +# Copyright 2017 Google Inc. All Rights Reserved. # # Licensed under the Apache License, Version 2.0 (the "License"); # you may not use this file except in compliance with the License. diff --git a/logging/google/cloud/logging/handlers/middleware/request.py b/logging/google/cloud/logging/handlers/middleware/request.py index a37a6af47035..be5102487cc6 100644 --- a/logging/google/cloud/logging/handlers/middleware/request.py +++ b/logging/google/cloud/logging/handlers/middleware/request.py @@ -1,4 +1,4 @@ -# Copyright 2016 Google Inc. All Rights Reserved. +# Copyright 2017 Google Inc. All Rights Reserved. # # Licensed under the Apache License, Version 2.0 (the "License"); # you may not use this file except in compliance with the License. From 43148bf40cd5bbae418f777d0e35c008e9d9834a Mon Sep 17 00:00:00 2001 From: Angela Li Date: Tue, 23 May 2017 14:45:28 -0700 Subject: [PATCH 20/47] Drop module level variables --- .../google/cloud/logging/handlers/_helpers.py | 22 ++++++++++--------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/logging/google/cloud/logging/handlers/_helpers.py b/logging/google/cloud/logging/handlers/_helpers.py index a1c697ba404c..6bf0edf79d54 100644 --- a/logging/google/cloud/logging/handlers/_helpers.py +++ b/logging/google/cloud/logging/handlers/_helpers.py @@ -25,9 +25,6 @@ from google.cloud.logging.handlers.middleware.request import get_request -_USE_FLASK = False -_USE_DJANGO = False - def format_stackdriver_json(record, message): """Helper to format a LogRecord in in Stackdriver fluentd format. @@ -56,19 +53,24 @@ def detect_web_framework(): Detect which web framework is used by looking at the sys.modules. If multiple or no supported web frameworks detected in the modules, then print a warning message. - Set _USE_FLASK or _USE_DJANGO to True if one of them are detected. + Return the name of web framework detected if flask or django is found. + Return unknown if cannot determine. + + :rtype: str + :returns: Web framework detected in this environment. """ modules = sys.modules - global _USE_FLASK, _USE_DJANGO + web_framework = 'unknown' if 'flask' in modules and 'django' in modules: print('Cannot determine, found multiple web frameworks.') elif 'flask' in modules: - _USE_FLASK = True + web_framework = 'flask' elif 'django' in modules: - _USE_DJANGO = True + web_framework = 'django' else: print('No supported web framework found in the modules.') + return web_framework def get_trace_id_from_request_header(): @@ -77,14 +79,14 @@ def get_trace_id_from_request_header(): :rtype: str :returns: Trace_id in HTTP request headers. """ - detect_web_framework() + web_framework = detect_web_framework() - if _USE_FLASK: + if web_framework is 'flask': try: trace_id = flask.request.headers['X_CLOUD_TRACE_CONTEXT'].split('/')[0] except Exception: trace_id = None - elif _USE_DJANGO: + elif web_framework is 'django': try: request = get_request() trace_id = request.META['HTTP_X_CLOUD_TRACE_CONTEXT'].split('/')[0] From de02a1de9cfad85ba35592935638eae816ec6b5d Mon Sep 17 00:00:00 2001 From: Angela Li Date: Thu, 25 May 2017 10:07:25 -0700 Subject: [PATCH 21/47] Address Jon's comments and add some unit tests (not complete yet) --- .../google/cloud/logging/handlers/_helpers.py | 66 ++++++++----------- .../cloud/logging/handlers/app_engine.py | 4 +- .../google/cloud/logging/handlers/handlers.py | 9 +-- .../logging/handlers/middleware/request.py | 6 +- .../tests/unit/handlers/test_app_engine.py | 11 +++- logging/tests/unit/handlers/test_handlers.py | 6 +- .../transports/test_background_thread.py | 8 +-- .../unit/handlers/transports/test_sync.py | 6 +- 8 files changed, 56 insertions(+), 60 deletions(-) diff --git a/logging/google/cloud/logging/handlers/_helpers.py b/logging/google/cloud/logging/handlers/_helpers.py index 6bf0edf79d54..0af7a53de476 100644 --- a/logging/google/cloud/logging/handlers/_helpers.py +++ b/logging/google/cloud/logging/handlers/_helpers.py @@ -16,7 +16,6 @@ import math import json -import sys try: import flask @@ -47,51 +46,44 @@ def format_stackdriver_json(record, message): return json.dumps(payload) -def detect_web_framework(): - """Detect web framework used in this environment. - - Detect which web framework is used by looking at the sys.modules. - If multiple or no supported web frameworks detected in the modules, then - print a warning message. - Return the name of web framework detected if flask or django is found. - Return unknown if cannot determine. +def get_trace_id_from_flask(): + """Get trace_id from flask request headers. :rtype: str - :returns: Web framework detected in this environment. + :return: Trace_id in HTTP request headers. """ - modules = sys.modules - web_framework = 'unknown' + try: + trace_id = flask.request.headers['X_CLOUD_TRACE_CONTEXT'].split('/')[0] + except Exception: + trace_id = None + return trace_id - if 'flask' in modules and 'django' in modules: - print('Cannot determine, found multiple web frameworks.') - elif 'flask' in modules: - web_framework = 'flask' - elif 'django' in modules: - web_framework = 'django' - else: - print('No supported web framework found in the modules.') - return web_framework + +def get_trace_id_from_django(): + """Get trace_id from django request headers. + + :rtype: str + :return: Trace_id in HTTP request headers. + """ + try: + request = get_request() + trace_id = request.META['HTTP_X_CLOUD_TRACE_CONTEXT'].split('/')[0] + except Exception: + trace_id = None + return trace_id -def get_trace_id_from_request_header(): +def get_trace_id(): """Helper to get trace_id from web application request header. :rtype: str :returns: Trace_id in HTTP request headers. """ - web_framework = detect_web_framework() - - if web_framework is 'flask': - try: - trace_id = flask.request.headers['X_CLOUD_TRACE_CONTEXT'].split('/')[0] - except Exception: - trace_id = None - elif web_framework is 'django': - try: - request = get_request() - trace_id = request.META['HTTP_X_CLOUD_TRACE_CONTEXT'].split('/')[0] - except Exception: - trace_id = None - else: - trace_id = None + checkers = [get_trace_id_from_django, get_trace_id_from_flask] + + for checker in checkers: + trace_id = checker() + if trace_id is not None: + return trace_id + return trace_id diff --git a/logging/google/cloud/logging/handlers/app_engine.py b/logging/google/cloud/logging/handlers/app_engine.py index ea4cc9245282..7bf4790e0603 100644 --- a/logging/google/cloud/logging/handlers/app_engine.py +++ b/logging/google/cloud/logging/handlers/app_engine.py @@ -20,7 +20,7 @@ import os -from google.cloud.logging.handlers._helpers import get_trace_id_from_request_header +from google.cloud.logging.handlers._helpers import get_trace_id from google.cloud.logging.handlers.handlers import CloudLoggingHandler from google.cloud.logging.handlers.transports import BackgroundThreadTransport from google.cloud.logging.resource import Resource @@ -76,7 +76,7 @@ def get_gae_labels(self): :rtype: dict :returns: Labels for GAE app. """ - trace_id = get_trace_id_from_request_header() + trace_id = get_trace_id() if trace_id is None: trace_id = 'unknown' gae_labels = { diff --git a/logging/google/cloud/logging/handlers/handlers.py b/logging/google/cloud/logging/handlers/handlers.py index b45b4f593bc2..26343ec9f9c5 100644 --- a/logging/google/cloud/logging/handlers/handlers.py +++ b/logging/google/cloud/logging/handlers/handlers.py @@ -102,10 +102,11 @@ def emit(self, record): :param record: The record to be logged. """ message = super(CloudLoggingHandler, self).format(record) - self.transport.send(record, - message, - resource=self.resource, - labels=self.labels) + self.transport.send( + record, + message, + resource=self.resource, + labels=self.labels) def setup_logging(handler, excluded_loggers=EXCLUDED_LOGGER_DEFAULTS, diff --git a/logging/google/cloud/logging/handlers/middleware/request.py b/logging/google/cloud/logging/handlers/middleware/request.py index be5102487cc6..92bfd4369b00 100644 --- a/logging/google/cloud/logging/handlers/middleware/request.py +++ b/logging/google/cloud/logging/handlers/middleware/request.py @@ -12,10 +12,8 @@ # See the License for the specific language governing permissions and # limitations under the License. -try: - from threading import local -except ImportError: - from django.utils._threading_local import local +from threading import local + _thread_locals = local() diff --git a/logging/tests/unit/handlers/test_app_engine.py b/logging/tests/unit/handlers/test_app_engine.py index c39328593f7a..99f5ea93625a 100644 --- a/logging/tests/unit/handlers/test_app_engine.py +++ b/logging/tests/unit/handlers/test_app_engine.py @@ -34,6 +34,7 @@ def test_constructor(self): from google.cloud.logging.handlers.app_engine import _GAE_VERSION_ENV client = mock.Mock(project=self.PROJECT, spec=['project']) + with mock.patch('os.environ', new={_GAE_PROJECT_ENV: 'test_project', _GAE_SERVICE_ENV: 'test_service', _GAE_VERSION_ENV: 'test_version'}): @@ -43,6 +44,7 @@ def test_constructor(self): self.assertEqual(handler.resource.labels['project_id'], 'test_project') self.assertEqual(handler.resource.labels['module_id'], 'test_service') self.assertEqual(handler.resource.labels['version_id'], 'test_version') + self.assertEqual(handler.labels['appengine.googleapis.com/trace_id'], 'unknown') def test_emit(self): import mock @@ -50,6 +52,7 @@ def test_emit(self): client = mock.Mock(project=self.PROJECT, spec=['project']) handler = self._make_one(client, transport=_Transport) gae_resource = handler.get_gae_resource() + gae_labels = handler.get_gae_labels() logname = 'app' message = 'hello world' record = logging.LogRecord(logname, logging, None, None, message, @@ -58,7 +61,9 @@ def test_emit(self): self.assertIs(handler.transport.client, client) self.assertEqual(handler.transport.name, logname) - self.assertEqual(handler.transport.send_called_with, (record, message, gae_resource)) + self.assertEqual( + handler.transport.send_called_with, + (record, message, gae_resource, gae_labels)) class _Transport(object): @@ -67,5 +72,5 @@ def __init__(self, client, name): self.client = client self.name = name - def send(self, record, message, resource): - self.send_called_with = (record, message, resource) + def send(self, record, message, resource, labels): + self.send_called_with = (record, message, resource, labels) diff --git a/logging/tests/unit/handlers/test_handlers.py b/logging/tests/unit/handlers/test_handlers.py index 05dc87631478..5459a0c10b96 100644 --- a/logging/tests/unit/handlers/test_handlers.py +++ b/logging/tests/unit/handlers/test_handlers.py @@ -45,7 +45,7 @@ def test_emit(self): None, None) handler.emit(record) - self.assertEqual(handler.transport.send_called_with, (record, message, _GLOBAL_RESOURCE)) + self.assertEqual(handler.transport.send_called_with, (record, message, _GLOBAL_RESOURCE, None)) class TestSetupLogging(unittest.TestCase): @@ -110,5 +110,5 @@ class _Transport(object): def __init__(self, client, name): pass - def send(self, record, message, resource): - self.send_called_with = (record, message, resource) + def send(self, record, message, resource, labels=None): + self.send_called_with = (record, message, resource, labels) diff --git a/logging/tests/unit/handlers/transports/test_background_thread.py b/logging/tests/unit/handlers/transports/test_background_thread.py index 3e3378dcd361..b9449ce56685 100644 --- a/logging/tests/unit/handlers/transports/test_background_thread.py +++ b/logging/tests/unit/handlers/transports/test_background_thread.py @@ -61,9 +61,9 @@ def test_send(self): python_logger_name, logging.INFO, None, None, message, None, None) - transport.send(record, message, _GLOBAL_RESOURCE) + transport.send(record, message, _GLOBAL_RESOURCE, None) - transport.worker.enqueue.assert_called_once_with(record, message, _GLOBAL_RESOURCE) + transport.worker.enqueue.assert_called_once_with(record, message, _GLOBAL_RESOURCE, None) def test_flush(self): client = _Client(self.PROJECT) @@ -287,13 +287,13 @@ def __init__(self): self.commit_called = False self.commit_count = None - def log_struct(self, info, severity=logging.INFO, resource=None): + def log_struct(self, info, severity=logging.INFO, resource=None, labels=None): from google.cloud.logging.logger import _GLOBAL_RESOURCE assert resource is None resource = _GLOBAL_RESOURCE - self.log_struct_called_with = (info, severity, resource) + self.log_struct_called_with = (info, severity, resource, labels) self.entries.append(info) def commit(self): diff --git a/logging/tests/unit/handlers/transports/test_sync.py b/logging/tests/unit/handlers/transports/test_sync.py index 475ecc9c6a71..e18920ce2167 100644 --- a/logging/tests/unit/handlers/transports/test_sync.py +++ b/logging/tests/unit/handlers/transports/test_sync.py @@ -52,7 +52,7 @@ def test_send(self): 'message': message, 'python_logger': python_logger_name, } - EXPECTED_SENT = (EXPECTED_STRUCT, 'INFO', _GLOBAL_RESOURCE) + EXPECTED_SENT = (EXPECTED_STRUCT, 'INFO', _GLOBAL_RESOURCE, None) self.assertEqual( transport.logger.log_struct_called_with, EXPECTED_SENT) @@ -63,8 +63,8 @@ class _Logger(object): def __init__(self, name): self.name = name - def log_struct(self, message, severity=None, resource=_GLOBAL_RESOURCE): - self.log_struct_called_with = (message, severity, resource) + def log_struct(self, message, severity=None, resource=_GLOBAL_RESOURCE, labels=None): + self.log_struct_called_with = (message, severity, resource, labels) class _Client(object): From ffb57c40a1e4426ca3f8001d6bc2d393321eec73 Mon Sep 17 00:00:00 2001 From: Angela Li Date: Thu, 25 May 2017 10:45:36 -0700 Subject: [PATCH 22/47] Fix stuff --- .../google/cloud/logging/handlers/_helpers.py | 35 ++++++++++++++----- .../logging/handlers/middleware/request.py | 17 +++++---- .../tests/unit/handlers/test_app_engine.py | 4 ++- 3 files changed, 37 insertions(+), 19 deletions(-) diff --git a/logging/google/cloud/logging/handlers/_helpers.py b/logging/google/cloud/logging/handlers/_helpers.py index 0af7a53de476..ca94e17ebb92 100644 --- a/logging/google/cloud/logging/handlers/_helpers.py +++ b/logging/google/cloud/logging/handlers/_helpers.py @@ -22,7 +22,10 @@ except ImportError: flask = None -from google.cloud.logging.handlers.middleware.request import get_request +from google.cloud.logging.handlers.middleware.request import RequestMiddleware + +_FLASK_TRACE_HEADER = 'X_CLOUD_TRACE_CONTEXT' +_DJANGO_TRACE_HEADER = 'HTTP_X_CLOUD_TRACE_CONTEXT' def format_stackdriver_json(record, message): @@ -52,10 +55,16 @@ def get_trace_id_from_flask(): :rtype: str :return: Trace_id in HTTP request headers. """ - try: - trace_id = flask.request.headers['X_CLOUD_TRACE_CONTEXT'].split('/')[0] - except Exception: - trace_id = None + if not flask or not flask.request: + return None + + header = flask.request.headers.get(_FLASK_TRACE_HEADER) + + if not header: + return None + + trace_id = header.split('/')[0] + return trace_id @@ -65,11 +74,19 @@ def get_trace_id_from_django(): :rtype: str :return: Trace_id in HTTP request headers. """ + request_middleware = RequestMiddleware() + request = request_middleware.get_request() + + if not request: + return None + try: - request = get_request() - trace_id = request.META['HTTP_X_CLOUD_TRACE_CONTEXT'].split('/')[0] - except Exception: - trace_id = None + header = request.META[_DJANGO_TRACE_HEADER] + except KeyError: + return None + + trace_id = header.split('/')[0] + return trace_id diff --git a/logging/google/cloud/logging/handlers/middleware/request.py b/logging/google/cloud/logging/handlers/middleware/request.py index 92bfd4369b00..7e0e89f6aa39 100644 --- a/logging/google/cloud/logging/handlers/middleware/request.py +++ b/logging/google/cloud/logging/handlers/middleware/request.py @@ -18,15 +18,6 @@ _thread_locals = local() -def get_request(): - """Get Django request from thread local. - - :rtype: str - :returns: Django request. - """ - return getattr(_thread_locals, 'request', None) - - class RequestMiddleware(object): """Saves the request in thread local""" @@ -37,3 +28,11 @@ def process_request(self, request): :param request: Django http request. """ _thread_locals.request = request + + def get_request(self): + """Get Django request from thread local. + + :rtype: str + :returns: Django request. + """ + return getattr(_thread_locals, 'request', None) diff --git a/logging/tests/unit/handlers/test_app_engine.py b/logging/tests/unit/handlers/test_app_engine.py index 99f5ea93625a..0f8ace3a95e7 100644 --- a/logging/tests/unit/handlers/test_app_engine.py +++ b/logging/tests/unit/handlers/test_app_engine.py @@ -33,6 +33,8 @@ def test_constructor(self): from google.cloud.logging.handlers.app_engine import _GAE_SERVICE_ENV from google.cloud.logging.handlers.app_engine import _GAE_VERSION_ENV + TRACE_ID_LABEL = 'appengine.googleapis.com/trace_id' + client = mock.Mock(project=self.PROJECT, spec=['project']) with mock.patch('os.environ', new={_GAE_PROJECT_ENV: 'test_project', @@ -44,7 +46,7 @@ def test_constructor(self): self.assertEqual(handler.resource.labels['project_id'], 'test_project') self.assertEqual(handler.resource.labels['module_id'], 'test_service') self.assertEqual(handler.resource.labels['version_id'], 'test_version') - self.assertEqual(handler.labels['appengine.googleapis.com/trace_id'], 'unknown') + self.assertEqual(handler.labels[TRACE_ID_LABEL], 'unknown') def test_emit(self): import mock From 8fd8f316fa29d26eb097642a94d1905db5373745 Mon Sep 17 00:00:00 2001 From: Angela Li Date: Thu, 25 May 2017 14:54:23 -0700 Subject: [PATCH 23/47] Add unit test for flask and some refactor --- .../google/cloud/logging/handlers/_helpers.py | 12 ++-- .../cloud/logging/handlers/app_engine.py | 2 - logging/nox.py | 2 +- logging/tests/unit/handlers/test__helpers.py | 57 +++++++++++++++++++ .../tests/unit/handlers/test_app_engine.py | 3 +- 5 files changed, 67 insertions(+), 9 deletions(-) create mode 100644 logging/tests/unit/handlers/test__helpers.py diff --git a/logging/google/cloud/logging/handlers/_helpers.py b/logging/google/cloud/logging/handlers/_helpers.py index ca94e17ebb92..17caf76e4aa9 100644 --- a/logging/google/cloud/logging/handlers/_helpers.py +++ b/logging/google/cloud/logging/handlers/_helpers.py @@ -27,6 +27,8 @@ _FLASK_TRACE_HEADER = 'X_CLOUD_TRACE_CONTEXT' _DJANGO_TRACE_HEADER = 'HTTP_X_CLOUD_TRACE_CONTEXT' +_EMPTY_TRACE_ID = 'None' + def format_stackdriver_json(record, message): """Helper to format a LogRecord in in Stackdriver fluentd format. @@ -56,12 +58,12 @@ def get_trace_id_from_flask(): :return: Trace_id in HTTP request headers. """ if not flask or not flask.request: - return None + return _EMPTY_TRACE_ID header = flask.request.headers.get(_FLASK_TRACE_HEADER) if not header: - return None + return _EMPTY_TRACE_ID trace_id = header.split('/')[0] @@ -78,12 +80,12 @@ def get_trace_id_from_django(): request = request_middleware.get_request() if not request: - return None + return _EMPTY_TRACE_ID try: header = request.META[_DJANGO_TRACE_HEADER] except KeyError: - return None + return _EMPTY_TRACE_ID trace_id = header.split('/')[0] @@ -100,7 +102,7 @@ def get_trace_id(): for checker in checkers: trace_id = checker() - if trace_id is not None: + if trace_id is not _EMPTY_TRACE_ID: return trace_id return trace_id diff --git a/logging/google/cloud/logging/handlers/app_engine.py b/logging/google/cloud/logging/handlers/app_engine.py index 7bf4790e0603..daa48132e246 100644 --- a/logging/google/cloud/logging/handlers/app_engine.py +++ b/logging/google/cloud/logging/handlers/app_engine.py @@ -77,8 +77,6 @@ def get_gae_labels(self): :returns: Labels for GAE app. """ trace_id = get_trace_id() - if trace_id is None: - trace_id = 'unknown' gae_labels = { 'appengine.googleapis.com/trace_id': trace_id, } diff --git a/logging/nox.py b/logging/nox.py index 5d4751a955a5..11d484b232e7 100644 --- a/logging/nox.py +++ b/logging/nox.py @@ -31,7 +31,7 @@ def unit_tests(session, python_version): session.interpreter = 'python{}'.format(python_version) # Install all test dependencies, then install this package in-place. - session.install('mock', 'pytest', 'pytest-cov', *LOCAL_DEPS) + session.install('mock', 'pytest', 'pytest-cov', 'flask', *LOCAL_DEPS) session.install('-e', '.') # Run py.test against the unit tests. diff --git a/logging/tests/unit/handlers/test__helpers.py b/logging/tests/unit/handlers/test__helpers.py new file mode 100644 index 000000000000..db9d51d27920 --- /dev/null +++ b/logging/tests/unit/handlers/test__helpers.py @@ -0,0 +1,57 @@ +# Copyright 2017 Google Inc. All Rights Reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +import unittest + + +class TestFlaskTrace(unittest.TestCase): + + def create_app(self): + import flask + + app = flask.Flask(__name__) + + @app.route('/') + def index(): + return 'test flask trace' + + return app + + def setUp(self): + self.app = self.create_app() + + def test_trace_id_no_context_header(self): + from google.cloud.logging.handlers._helpers import get_trace_id_from_flask + from google.cloud.logging.handlers._helpers import _EMPTY_TRACE_ID + + with self.app.test_request_context( + path='/', + headers={}): + trace_id = get_trace_id_from_flask() + + self.assertEqual(trace_id, _EMPTY_TRACE_ID) + + def test_trace_id_valid_context_header(self): + from google.cloud.logging.handlers._helpers import get_trace_id_from_flask + + FLASK_TRACE_HEADER = 'X_CLOUD_TRACE_CONTEXT' + FLASK_TRACE_ID = 'testtraceid/testspanid' + + with self.app.test_request_context( + path='/', + headers={FLASK_TRACE_HEADER:FLASK_TRACE_ID}): + trace_id = get_trace_id_from_flask() + + EXPECTED_TRACE_ID = 'testtraceid' + self.assertEqual(trace_id, EXPECTED_TRACE_ID) diff --git a/logging/tests/unit/handlers/test_app_engine.py b/logging/tests/unit/handlers/test_app_engine.py index 0f8ace3a95e7..886dbbbb5584 100644 --- a/logging/tests/unit/handlers/test_app_engine.py +++ b/logging/tests/unit/handlers/test_app_engine.py @@ -32,6 +32,7 @@ def test_constructor(self): from google.cloud.logging.handlers.app_engine import _GAE_PROJECT_ENV from google.cloud.logging.handlers.app_engine import _GAE_SERVICE_ENV from google.cloud.logging.handlers.app_engine import _GAE_VERSION_ENV + from google.cloud.logging.handlers._helpers import _EMPTY_TRACE_ID TRACE_ID_LABEL = 'appengine.googleapis.com/trace_id' @@ -46,7 +47,7 @@ def test_constructor(self): self.assertEqual(handler.resource.labels['project_id'], 'test_project') self.assertEqual(handler.resource.labels['module_id'], 'test_service') self.assertEqual(handler.resource.labels['version_id'], 'test_version') - self.assertEqual(handler.labels[TRACE_ID_LABEL], 'unknown') + self.assertEqual(handler.labels[TRACE_ID_LABEL], _EMPTY_TRACE_ID) def test_emit(self): import mock From 44800270d6dcf141d1c04da65bbba1ff3ddbc109 Mon Sep 17 00:00:00 2001 From: Angela Li Date: Fri, 26 May 2017 10:20:56 -0700 Subject: [PATCH 24/47] Add unit test for middleware --- logging/nox.py | 2 +- .../unit/handlers/middleware/test_request.py | 39 +++++++++++++++++++ 2 files changed, 40 insertions(+), 1 deletion(-) create mode 100644 logging/tests/unit/handlers/middleware/test_request.py diff --git a/logging/nox.py b/logging/nox.py index 11d484b232e7..de975600fd04 100644 --- a/logging/nox.py +++ b/logging/nox.py @@ -31,7 +31,7 @@ def unit_tests(session, python_version): session.interpreter = 'python{}'.format(python_version) # Install all test dependencies, then install this package in-place. - session.install('mock', 'pytest', 'pytest-cov', 'flask', *LOCAL_DEPS) + session.install('mock', 'pytest', 'pytest-cov', 'flask', 'django', *LOCAL_DEPS) session.install('-e', '.') # Run py.test against the unit tests. diff --git a/logging/tests/unit/handlers/middleware/test_request.py b/logging/tests/unit/handlers/middleware/test_request.py new file mode 100644 index 000000000000..ad784b45c48e --- /dev/null +++ b/logging/tests/unit/handlers/middleware/test_request.py @@ -0,0 +1,39 @@ +# Copyright 2017 Google Inc. All Rights Reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +import unittest + + +class TestRequestMiddleware(unittest.TestCase): + + def _get_target_class(self): + from google.cloud.logging.handlers.middleware.request import RequestMiddleware + + return RequestMiddleware + + def _make_one(self, *args, **kw): + return self._get_target_class()(*args, **kw) + + def test_get_request(self): + from django.conf import settings + from django.test.utils import setup_test_environment + from django.test import RequestFactory + + settings.configure() + setup_test_environment() + + self.middleware = self._make_one() + self.request = RequestFactory().get('/') + self.middleware.process_request(self.request) + self.assertEqual(self.middleware.get_request(), self.request) From ad00bdb98131f71396dd05c9b219211892efe481 Mon Sep 17 00:00:00 2001 From: Angela Li Date: Fri, 26 May 2017 11:25:41 -0700 Subject: [PATCH 25/47] Add unit test for django --- .../unit/handlers/middleware/test_request.py | 15 ++++- logging/tests/unit/handlers/test__helpers.py | 67 ++++++++++++++++++- 2 files changed, 77 insertions(+), 5 deletions(-) diff --git a/logging/tests/unit/handlers/middleware/test_request.py b/logging/tests/unit/handlers/middleware/test_request.py index ad784b45c48e..5b6dfca8dc67 100644 --- a/logging/tests/unit/handlers/middleware/test_request.py +++ b/logging/tests/unit/handlers/middleware/test_request.py @@ -25,15 +25,24 @@ def _get_target_class(self): def _make_one(self, *args, **kw): return self._get_target_class()(*args, **kw) - def test_get_request(self): + def setUp(self): from django.conf import settings from django.test.utils import setup_test_environment - from django.test import RequestFactory - settings.configure() + if not settings.configured: + settings.configure() setup_test_environment() + def test_get_request(self): + from django.test import RequestFactory + self.middleware = self._make_one() self.request = RequestFactory().get('/') self.middleware.process_request(self.request) self.assertEqual(self.middleware.get_request(), self.request) + + + def tearDown(self): + from django.test.utils import teardown_test_environment + + teardown_test_environment() diff --git a/logging/tests/unit/handlers/test__helpers.py b/logging/tests/unit/handlers/test__helpers.py index db9d51d27920..3d023dd8f9cf 100644 --- a/logging/tests/unit/handlers/test__helpers.py +++ b/logging/tests/unit/handlers/test__helpers.py @@ -33,25 +33,88 @@ def setUp(self): def test_trace_id_no_context_header(self): from google.cloud.logging.handlers._helpers import get_trace_id_from_flask + from google.cloud.logging.handlers._helpers import get_trace_id from google.cloud.logging.handlers._helpers import _EMPTY_TRACE_ID with self.app.test_request_context( path='/', headers={}): trace_id = get_trace_id_from_flask() + trace_id_returned = get_trace_id() self.assertEqual(trace_id, _EMPTY_TRACE_ID) + self.assertEqual(trace_id_returned, _EMPTY_TRACE_ID) def test_trace_id_valid_context_header(self): from google.cloud.logging.handlers._helpers import get_trace_id_from_flask + from google.cloud.logging.handlers._helpers import get_trace_id FLASK_TRACE_HEADER = 'X_CLOUD_TRACE_CONTEXT' - FLASK_TRACE_ID = 'testtraceid/testspanid' + FLASK_TRACE_ID = 'testtraceidflask/testspanid' with self.app.test_request_context( path='/', headers={FLASK_TRACE_HEADER:FLASK_TRACE_ID}): trace_id = get_trace_id_from_flask() + trace_id_returned = get_trace_id() - EXPECTED_TRACE_ID = 'testtraceid' + EXPECTED_TRACE_ID = 'testtraceidflask' self.assertEqual(trace_id, EXPECTED_TRACE_ID) + self.assertEqual(trace_id, trace_id_returned) + + +class TestDjangoTrace(unittest.TestCase): + + def setUp(self): + from django.conf import settings + from django.test.utils import setup_test_environment + + if not settings.configured: + settings.configure() + setup_test_environment() + + def test_trace_id_no_context_header(self): + import mock + from django.test import RequestFactory + from google.cloud.logging.handlers._helpers import get_trace_id_from_django + from google.cloud.logging.handlers._helpers import get_trace_id + from google.cloud.logging.handlers._helpers import _EMPTY_TRACE_ID + + request = RequestFactory().get('/') + + with mock.patch( + 'google.cloud.logging.handlers.middleware.RequestMiddleware.get_request', + return_value=request): + trace_id = get_trace_id_from_django() + trace_id_returned = get_trace_id() + + self.assertEqual(trace_id, _EMPTY_TRACE_ID) + self.assertEqual(trace_id_returned, _EMPTY_TRACE_ID) + + def test_trace_id_valid_context_header(self): + import mock + from django.test import RequestFactory + from google.cloud.logging.handlers._helpers import get_trace_id_from_django + from google.cloud.logging.handlers._helpers import get_trace_id + + DJANGO_TRACE_HEADER = 'HTTP_X_CLOUD_TRACE_CONTEXT' + DJANGO_TRACE_ID = 'testtraceiddjango/testspanid' + + request = RequestFactory().get( + '/', + **{DJANGO_TRACE_HEADER:DJANGO_TRACE_ID}) + + with mock.patch( + 'google.cloud.logging.handlers.middleware.RequestMiddleware.get_request', + return_value=request): + trace_id = get_trace_id_from_django() + trace_id_returned = get_trace_id() + + EXPECTED_TRACE_ID = 'testtraceiddjango' + self.assertEqual(trace_id, EXPECTED_TRACE_ID) + self.assertEqual(trace_id, trace_id_returned) + + def tearDown(self): + from django.test.utils import teardown_test_environment + + teardown_test_environment() From 8ed81ad0b6f4afcc0cb7b3d89cb26693fb137d4b Mon Sep 17 00:00:00 2001 From: Angela Li Date: Fri, 26 May 2017 11:29:54 -0700 Subject: [PATCH 26/47] Fix style --- logging/tests/unit/handlers/middleware/test_request.py | 1 - 1 file changed, 1 deletion(-) diff --git a/logging/tests/unit/handlers/middleware/test_request.py b/logging/tests/unit/handlers/middleware/test_request.py index 5b6dfca8dc67..0aa950263f5d 100644 --- a/logging/tests/unit/handlers/middleware/test_request.py +++ b/logging/tests/unit/handlers/middleware/test_request.py @@ -41,7 +41,6 @@ def test_get_request(self): self.middleware.process_request(self.request) self.assertEqual(self.middleware.get_request(), self.request) - def tearDown(self): from django.test.utils import teardown_test_environment From a6b25bbdaf5da742e90c27d8118a7df493685a11 Mon Sep 17 00:00:00 2001 From: Angela Li Date: Fri, 26 May 2017 14:38:45 -0700 Subject: [PATCH 27/47] Address jon's comments --- .../google/cloud/logging/handlers/_helpers.py | 17 ++--- .../cloud/logging/handlers/app_engine.py | 10 ++- .../logging/handlers/middleware/request.py | 17 ++--- .../unit/handlers/middleware/test_request.py | 13 ++-- logging/tests/unit/handlers/test__helpers.py | 66 ++++++++++--------- .../tests/unit/handlers/test_app_engine.py | 6 +- 6 files changed, 66 insertions(+), 63 deletions(-) diff --git a/logging/google/cloud/logging/handlers/_helpers.py b/logging/google/cloud/logging/handlers/_helpers.py index 17caf76e4aa9..2c88f427f00b 100644 --- a/logging/google/cloud/logging/handlers/_helpers.py +++ b/logging/google/cloud/logging/handlers/_helpers.py @@ -22,13 +22,11 @@ except ImportError: flask = None -from google.cloud.logging.handlers.middleware.request import RequestMiddleware +from google.cloud.logging.handlers.middleware.request import _get_request _FLASK_TRACE_HEADER = 'X_CLOUD_TRACE_CONTEXT' _DJANGO_TRACE_HEADER = 'HTTP_X_CLOUD_TRACE_CONTEXT' -_EMPTY_TRACE_ID = 'None' - def format_stackdriver_json(record, message): """Helper to format a LogRecord in in Stackdriver fluentd format. @@ -58,12 +56,12 @@ def get_trace_id_from_flask(): :return: Trace_id in HTTP request headers. """ if not flask or not flask.request: - return _EMPTY_TRACE_ID + return None header = flask.request.headers.get(_FLASK_TRACE_HEADER) if not header: - return _EMPTY_TRACE_ID + return None trace_id = header.split('/')[0] @@ -76,16 +74,15 @@ def get_trace_id_from_django(): :rtype: str :return: Trace_id in HTTP request headers. """ - request_middleware = RequestMiddleware() - request = request_middleware.get_request() + request = _get_request() if not request: - return _EMPTY_TRACE_ID + return None try: header = request.META[_DJANGO_TRACE_HEADER] except KeyError: - return _EMPTY_TRACE_ID + return None trace_id = header.split('/')[0] @@ -102,7 +99,7 @@ def get_trace_id(): for checker in checkers: trace_id = checker() - if trace_id is not _EMPTY_TRACE_ID: + if trace_id is not None: return trace_id return trace_id diff --git a/logging/google/cloud/logging/handlers/app_engine.py b/logging/google/cloud/logging/handlers/app_engine.py index daa48132e246..f8b12c44a42e 100644 --- a/logging/google/cloud/logging/handlers/app_engine.py +++ b/logging/google/cloud/logging/handlers/app_engine.py @@ -31,6 +31,8 @@ _GAE_SERVICE_ENV = 'GAE_SERVICE' _GAE_VERSION_ENV = 'GAE_VERSION' +_TRACE_ID_LABEL = 'appengine.googleapis.com/trace_id' + class AppEngineHandler(CloudLoggingHandler): """A logging handler that sends App Engine-formatted logs to Stackdriver. @@ -76,8 +78,10 @@ def get_gae_labels(self): :rtype: dict :returns: Labels for GAE app. """ + gae_labels = {} + trace_id = get_trace_id() - gae_labels = { - 'appengine.googleapis.com/trace_id': trace_id, - } + if trace_id is not None: + gae_labels[_TRACE_ID_LABEL] = trace_id + return gae_labels diff --git a/logging/google/cloud/logging/handlers/middleware/request.py b/logging/google/cloud/logging/handlers/middleware/request.py index 7e0e89f6aa39..9baf11f4dbbe 100644 --- a/logging/google/cloud/logging/handlers/middleware/request.py +++ b/logging/google/cloud/logging/handlers/middleware/request.py @@ -18,6 +18,15 @@ _thread_locals = local() +def _get_request(): + """Get Django request from thread local. + + :rtype: str + :returns: Django request. + """ + return getattr(_thread_locals, 'request', None) + + class RequestMiddleware(object): """Saves the request in thread local""" @@ -28,11 +37,3 @@ def process_request(self, request): :param request: Django http request. """ _thread_locals.request = request - - def get_request(self): - """Get Django request from thread local. - - :rtype: str - :returns: Django request. - """ - return getattr(_thread_locals, 'request', None) diff --git a/logging/tests/unit/handlers/middleware/test_request.py b/logging/tests/unit/handlers/middleware/test_request.py index 0aa950263f5d..349247805104 100644 --- a/logging/tests/unit/handlers/middleware/test_request.py +++ b/logging/tests/unit/handlers/middleware/test_request.py @@ -33,15 +33,16 @@ def setUp(self): settings.configure() setup_test_environment() + def tearDown(self): + from django.test.utils import teardown_test_environment + + teardown_test_environment() + def test_get_request(self): from django.test import RequestFactory + from google.cloud.logging.handlers.middleware.request import _get_request self.middleware = self._make_one() self.request = RequestFactory().get('/') self.middleware.process_request(self.request) - self.assertEqual(self.middleware.get_request(), self.request) - - def tearDown(self): - from django.test.utils import teardown_test_environment - - teardown_test_environment() + self.assertEqual(_get_request(), self.request) diff --git a/logging/tests/unit/handlers/test__helpers.py b/logging/tests/unit/handlers/test__helpers.py index 3d023dd8f9cf..634ceb26e8a6 100644 --- a/logging/tests/unit/handlers/test__helpers.py +++ b/logging/tests/unit/handlers/test__helpers.py @@ -34,7 +34,6 @@ def setUp(self): def test_trace_id_no_context_header(self): from google.cloud.logging.handlers._helpers import get_trace_id_from_flask from google.cloud.logging.handlers._helpers import get_trace_id - from google.cloud.logging.handlers._helpers import _EMPTY_TRACE_ID with self.app.test_request_context( path='/', @@ -42,24 +41,24 @@ def test_trace_id_no_context_header(self): trace_id = get_trace_id_from_flask() trace_id_returned = get_trace_id() - self.assertEqual(trace_id, _EMPTY_TRACE_ID) - self.assertEqual(trace_id_returned, _EMPTY_TRACE_ID) + self.assertEqual(trace_id, None) + self.assertEqual(trace_id_returned, None) def test_trace_id_valid_context_header(self): from google.cloud.logging.handlers._helpers import get_trace_id_from_flask from google.cloud.logging.handlers._helpers import get_trace_id - FLASK_TRACE_HEADER = 'X_CLOUD_TRACE_CONTEXT' - FLASK_TRACE_ID = 'testtraceidflask/testspanid' + flask_trace_header = 'X_CLOUD_TRACE_CONTEXT' + flask_trace_id = 'testtraceidflask/testspanid' with self.app.test_request_context( path='/', - headers={FLASK_TRACE_HEADER:FLASK_TRACE_ID}): + headers={flask_trace_header:flask_trace_id}): trace_id = get_trace_id_from_flask() trace_id_returned = get_trace_id() - EXPECTED_TRACE_ID = 'testtraceidflask' - self.assertEqual(trace_id, EXPECTED_TRACE_ID) + expected_trace_id = 'testtraceidflask' + self.assertEqual(trace_id, expected_trace_id) self.assertEqual(trace_id, trace_id_returned) @@ -73,48 +72,51 @@ def setUp(self): settings.configure() setup_test_environment() + def tearDown(self): + from django.test.utils import teardown_test_environment + + teardown_test_environment() + def test_trace_id_no_context_header(self): - import mock from django.test import RequestFactory from google.cloud.logging.handlers._helpers import get_trace_id_from_django from google.cloud.logging.handlers._helpers import get_trace_id - from google.cloud.logging.handlers._helpers import _EMPTY_TRACE_ID + from google.cloud.logging.handlers.middleware.request import RequestMiddleware + from google.cloud.logging.handlers.middleware.request import _thread_locals request = RequestFactory().get('/') - with mock.patch( - 'google.cloud.logging.handlers.middleware.RequestMiddleware.get_request', - return_value=request): - trace_id = get_trace_id_from_django() - trace_id_returned = get_trace_id() + middleware = RequestMiddleware() + middleware.process_request(request) + trace_id = get_trace_id_from_django() + trace_id_returned = get_trace_id() - self.assertEqual(trace_id, _EMPTY_TRACE_ID) - self.assertEqual(trace_id_returned, _EMPTY_TRACE_ID) + self.assertEqual(trace_id, None) + self.assertEqual(trace_id_returned, None) + + _thread_locals.__dict__.clear() def test_trace_id_valid_context_header(self): - import mock from django.test import RequestFactory from google.cloud.logging.handlers._helpers import get_trace_id_from_django from google.cloud.logging.handlers._helpers import get_trace_id + from google.cloud.logging.handlers.middleware.request import RequestMiddleware + from google.cloud.logging.handlers.middleware.request import _thread_locals - DJANGO_TRACE_HEADER = 'HTTP_X_CLOUD_TRACE_CONTEXT' - DJANGO_TRACE_ID = 'testtraceiddjango/testspanid' + django_trace_header = 'HTTP_X_CLOUD_TRACE_CONTEXT' + django_trace_id = 'testtraceiddjango/testspanid' request = RequestFactory().get( '/', - **{DJANGO_TRACE_HEADER:DJANGO_TRACE_ID}) + **{django_trace_header:django_trace_id}) - with mock.patch( - 'google.cloud.logging.handlers.middleware.RequestMiddleware.get_request', - return_value=request): - trace_id = get_trace_id_from_django() - trace_id_returned = get_trace_id() + middleware = RequestMiddleware() + middleware.process_request(request) + trace_id = get_trace_id_from_django() + trace_id_returned = get_trace_id() - EXPECTED_TRACE_ID = 'testtraceiddjango' - self.assertEqual(trace_id, EXPECTED_TRACE_ID) + expected_trace_id = 'testtraceiddjango' + self.assertEqual(trace_id, expected_trace_id) self.assertEqual(trace_id, trace_id_returned) - def tearDown(self): - from django.test.utils import teardown_test_environment - - teardown_test_environment() + _thread_locals.__dict__.clear() diff --git a/logging/tests/unit/handlers/test_app_engine.py b/logging/tests/unit/handlers/test_app_engine.py index 886dbbbb5584..ccdaf2992335 100644 --- a/logging/tests/unit/handlers/test_app_engine.py +++ b/logging/tests/unit/handlers/test_app_engine.py @@ -32,9 +32,7 @@ def test_constructor(self): from google.cloud.logging.handlers.app_engine import _GAE_PROJECT_ENV from google.cloud.logging.handlers.app_engine import _GAE_SERVICE_ENV from google.cloud.logging.handlers.app_engine import _GAE_VERSION_ENV - from google.cloud.logging.handlers._helpers import _EMPTY_TRACE_ID - - TRACE_ID_LABEL = 'appengine.googleapis.com/trace_id' + from google.cloud.logging.handlers.app_engine import _TRACE_ID_LABEL client = mock.Mock(project=self.PROJECT, spec=['project']) @@ -47,7 +45,7 @@ def test_constructor(self): self.assertEqual(handler.resource.labels['project_id'], 'test_project') self.assertEqual(handler.resource.labels['module_id'], 'test_service') self.assertEqual(handler.resource.labels['version_id'], 'test_version') - self.assertEqual(handler.labels[TRACE_ID_LABEL], _EMPTY_TRACE_ID) + self.assertEqual(handler.labels, {}) def test_emit(self): import mock From 61237646c11f8fe66b0f2024552ea32118ad66a1 Mon Sep 17 00:00:00 2001 From: Angela Li Date: Tue, 30 May 2017 12:28:30 -0700 Subject: [PATCH 28/47] Address jon's comments --- logging/google/cloud/logging/handlers/_helpers.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/logging/google/cloud/logging/handlers/_helpers.py b/logging/google/cloud/logging/handlers/_helpers.py index 2c88f427f00b..00135d0fe243 100644 --- a/logging/google/cloud/logging/handlers/_helpers.py +++ b/logging/google/cloud/logging/handlers/_helpers.py @@ -80,10 +80,12 @@ def get_trace_id_from_django(): return None try: - header = request.META[_DJANGO_TRACE_HEADER] + header = request.META.get(_DJANGO_TRACE_HEADER) except KeyError: return None + if not header: + return None trace_id = header.split('/')[0] return trace_id @@ -102,4 +104,4 @@ def get_trace_id(): if trace_id is not None: return trace_id - return trace_id + return None From 3caccc7cc0ae40b933d06ec354d796feee1af56f Mon Sep 17 00:00:00 2001 From: Angela Li Date: Fri, 2 Jun 2017 00:28:10 -0700 Subject: [PATCH 29/47] Address all comments --- .../google/cloud/logging/handlers/_helpers.py | 14 ++--- .../logging/handlers/middleware/request.py | 6 +- logging/nox.py | 4 +- .../unit/handlers/middleware/test_request.py | 18 +++--- logging/tests/unit/handlers/test__helpers.py | 56 +++++++++---------- logging/tests/unit/handlers/test_handlers.py | 4 +- .../transports/test_background_thread.py | 3 +- .../unit/handlers/transports/test_sync.py | 3 +- 8 files changed, 58 insertions(+), 50 deletions(-) diff --git a/logging/google/cloud/logging/handlers/_helpers.py b/logging/google/cloud/logging/handlers/_helpers.py index 00135d0fe243..e5bdabe2e388 100644 --- a/logging/google/cloud/logging/handlers/_helpers.py +++ b/logging/google/cloud/logging/handlers/_helpers.py @@ -22,7 +22,7 @@ except ImportError: flask = None -from google.cloud.logging.handlers.middleware.request import _get_request +from google.cloud.logging.handlers.middleware.request import _get_django_request _FLASK_TRACE_HEADER = 'X_CLOUD_TRACE_CONTEXT' _DJANGO_TRACE_HEADER = 'HTTP_X_CLOUD_TRACE_CONTEXT' @@ -55,12 +55,12 @@ def get_trace_id_from_flask(): :rtype: str :return: Trace_id in HTTP request headers. """ - if not flask or not flask.request: + if flask is None or not flask.request: return None header = flask.request.headers.get(_FLASK_TRACE_HEADER) - if not header: + if header is None: return None trace_id = header.split('/')[0] @@ -74,9 +74,9 @@ def get_trace_id_from_django(): :rtype: str :return: Trace_id in HTTP request headers. """ - request = _get_request() + request = _get_django_request() - if not request: + if request is None: return None try: @@ -84,7 +84,7 @@ def get_trace_id_from_django(): except KeyError: return None - if not header: + if header is None: return None trace_id = header.split('/')[0] @@ -97,7 +97,7 @@ def get_trace_id(): :rtype: str :returns: Trace_id in HTTP request headers. """ - checkers = [get_trace_id_from_django, get_trace_id_from_flask] + checkers = (get_trace_id_from_django, get_trace_id_from_flask) for checker in checkers: trace_id = checker() diff --git a/logging/google/cloud/logging/handlers/middleware/request.py b/logging/google/cloud/logging/handlers/middleware/request.py index 9baf11f4dbbe..1acf7ffd9961 100644 --- a/logging/google/cloud/logging/handlers/middleware/request.py +++ b/logging/google/cloud/logging/handlers/middleware/request.py @@ -12,13 +12,13 @@ # See the License for the specific language governing permissions and # limitations under the License. -from threading import local +import threading -_thread_locals = local() +_thread_locals = threading.local() -def _get_request(): +def _get_django_request(): """Get Django request from thread local. :rtype: str diff --git a/logging/nox.py b/logging/nox.py index de975600fd04..fbbbec1958c1 100644 --- a/logging/nox.py +++ b/logging/nox.py @@ -31,7 +31,9 @@ def unit_tests(session, python_version): session.interpreter = 'python{}'.format(python_version) # Install all test dependencies, then install this package in-place. - session.install('mock', 'pytest', 'pytest-cov', 'flask', 'django', *LOCAL_DEPS) + session.install( + 'mock', 'pytest', 'pytest-cov', + 'flask', 'django', *LOCAL_DEPS) session.install('-e', '.') # Run py.test against the unit tests. diff --git a/logging/tests/unit/handlers/middleware/test_request.py b/logging/tests/unit/handlers/middleware/test_request.py index 349247805104..ab8c18d1aa0e 100644 --- a/logging/tests/unit/handlers/middleware/test_request.py +++ b/logging/tests/unit/handlers/middleware/test_request.py @@ -25,7 +25,8 @@ def _get_target_class(self): def _make_one(self, *args, **kw): return self._get_target_class()(*args, **kw) - def setUp(self): + @classmethod + def setUpClass(cls): from django.conf import settings from django.test.utils import setup_test_environment @@ -33,16 +34,17 @@ def setUp(self): settings.configure() setup_test_environment() - def tearDown(self): + @classmethod + def tearDownClass(cls): from django.test.utils import teardown_test_environment teardown_test_environment() - def test_get_request(self): + def test_get_django_request(self): from django.test import RequestFactory - from google.cloud.logging.handlers.middleware.request import _get_request + from google.cloud.logging.handlers.middleware.request import _get_django_request - self.middleware = self._make_one() - self.request = RequestFactory().get('/') - self.middleware.process_request(self.request) - self.assertEqual(_get_request(), self.request) + middleware = self._make_one() + request = RequestFactory().get('/') + middleware.process_request(request) + self.assertEqual(_get_django_request(), request) diff --git a/logging/tests/unit/handlers/test__helpers.py b/logging/tests/unit/handlers/test__helpers.py index 634ceb26e8a6..59e56f34524b 100644 --- a/logging/tests/unit/handlers/test__helpers.py +++ b/logging/tests/unit/handlers/test__helpers.py @@ -15,9 +15,10 @@ import unittest -class TestFlaskTrace(unittest.TestCase): +class Test_get_trace_id_from_flask(unittest.TestCase): - def create_app(self): + @staticmethod + def create_app(): import flask app = flask.Flask(__name__) @@ -29,40 +30,41 @@ def index(): return app def setUp(self): - self.app = self.create_app() + self.app = Test_get_trace_id_from_flask.create_app() def test_trace_id_no_context_header(self): - from google.cloud.logging.handlers._helpers import get_trace_id_from_flask - from google.cloud.logging.handlers._helpers import get_trace_id + from google.cloud.logging.handlers import _helpers with self.app.test_request_context( path='/', headers={}): - trace_id = get_trace_id_from_flask() - trace_id_returned = get_trace_id() + trace_id = _helpers.get_trace_id_from_flask() + trace_id_returned = _helpers.get_trace_id() - self.assertEqual(trace_id, None) - self.assertEqual(trace_id_returned, None) + self.assertIsNone(trace_id) + self.assertIsNone(trace_id_returned) def test_trace_id_valid_context_header(self): from google.cloud.logging.handlers._helpers import get_trace_id_from_flask from google.cloud.logging.handlers._helpers import get_trace_id flask_trace_header = 'X_CLOUD_TRACE_CONTEXT' - flask_trace_id = 'testtraceidflask/testspanid' + expected_trace_id = 'testtraceidflask' + flask_trace_id = expected_trace_id + '/testspanid' - with self.app.test_request_context( - path='/', - headers={flask_trace_header:flask_trace_id}): + context = self.app.test_request_context( + path='/', + headers={flask_trace_header: flask_trace_id}) + + with context: trace_id = get_trace_id_from_flask() trace_id_returned = get_trace_id() - expected_trace_id = 'testtraceidflask' self.assertEqual(trace_id, expected_trace_id) self.assertEqual(trace_id, trace_id_returned) -class TestDjangoTrace(unittest.TestCase): +class Test_get_trace_id_from_django(unittest.TestCase): def setUp(self): from django.conf import settings @@ -79,22 +81,20 @@ def tearDown(self): def test_trace_id_no_context_header(self): from django.test import RequestFactory - from google.cloud.logging.handlers._helpers import get_trace_id_from_django - from google.cloud.logging.handlers._helpers import get_trace_id - from google.cloud.logging.handlers.middleware.request import RequestMiddleware - from google.cloud.logging.handlers.middleware.request import _thread_locals + from google.cloud.logging.handlers import _helpers + from google.cloud.logging.handlers.middleware import request - request = RequestFactory().get('/') + django_request = RequestFactory().get('/') - middleware = RequestMiddleware() - middleware.process_request(request) - trace_id = get_trace_id_from_django() - trace_id_returned = get_trace_id() + middleware = request.RequestMiddleware() + middleware.process_request(django_request) + trace_id = _helpers.get_trace_id_from_django() + trace_id_returned = _helpers.get_trace_id() - self.assertEqual(trace_id, None) - self.assertEqual(trace_id_returned, None) + self.assertIsNone(trace_id) + self.assertIsNone(trace_id_returned) - _thread_locals.__dict__.clear() + request._thread_locals.__dict__.clear() def test_trace_id_valid_context_header(self): from django.test import RequestFactory @@ -108,7 +108,7 @@ def test_trace_id_valid_context_header(self): request = RequestFactory().get( '/', - **{django_trace_header:django_trace_id}) + **{django_trace_header: django_trace_id}) middleware = RequestMiddleware() middleware.process_request(request) diff --git a/logging/tests/unit/handlers/test_handlers.py b/logging/tests/unit/handlers/test_handlers.py index 5459a0c10b96..96823b2e906d 100644 --- a/logging/tests/unit/handlers/test_handlers.py +++ b/logging/tests/unit/handlers/test_handlers.py @@ -45,7 +45,9 @@ def test_emit(self): None, None) handler.emit(record) - self.assertEqual(handler.transport.send_called_with, (record, message, _GLOBAL_RESOURCE, None)) + self.assertEqual( + handler.transport.send_called_with, + (record, message, _GLOBAL_RESOURCE, None)) class TestSetupLogging(unittest.TestCase): diff --git a/logging/tests/unit/handlers/transports/test_background_thread.py b/logging/tests/unit/handlers/transports/test_background_thread.py index b9449ce56685..f6671273b53d 100644 --- a/logging/tests/unit/handlers/transports/test_background_thread.py +++ b/logging/tests/unit/handlers/transports/test_background_thread.py @@ -63,7 +63,8 @@ def test_send(self): transport.send(record, message, _GLOBAL_RESOURCE, None) - transport.worker.enqueue.assert_called_once_with(record, message, _GLOBAL_RESOURCE, None) + transport.worker.enqueue.assert_called_once_with( + record, message, _GLOBAL_RESOURCE, None) def test_flush(self): client = _Client(self.PROJECT) diff --git a/logging/tests/unit/handlers/transports/test_sync.py b/logging/tests/unit/handlers/transports/test_sync.py index e18920ce2167..01c15240f3b7 100644 --- a/logging/tests/unit/handlers/transports/test_sync.py +++ b/logging/tests/unit/handlers/transports/test_sync.py @@ -63,7 +63,8 @@ class _Logger(object): def __init__(self, name): self.name = name - def log_struct(self, message, severity=None, resource=_GLOBAL_RESOURCE, labels=None): + def log_struct(self, message, severity=None, + resource=_GLOBAL_RESOURCE, labels=None): self.log_struct_called_with = (message, severity, resource, labels) From 4589a06e322402c18587304d41069769f5fbfbcb Mon Sep 17 00:00:00 2001 From: Angela Li Date: Fri, 2 Jun 2017 00:34:48 -0700 Subject: [PATCH 30/47] fix style --- logging/tests/unit/handlers/test__helpers.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/logging/tests/unit/handlers/test__helpers.py b/logging/tests/unit/handlers/test__helpers.py index 59e56f34524b..c50ce0044e85 100644 --- a/logging/tests/unit/handlers/test__helpers.py +++ b/logging/tests/unit/handlers/test__helpers.py @@ -104,7 +104,8 @@ def test_trace_id_valid_context_header(self): from google.cloud.logging.handlers.middleware.request import _thread_locals django_trace_header = 'HTTP_X_CLOUD_TRACE_CONTEXT' - django_trace_id = 'testtraceiddjango/testspanid' + expected_trace_id = 'testtraceiddjango' + django_trace_id = expected_trace_id + '/testspanid' request = RequestFactory().get( '/', @@ -115,7 +116,6 @@ def test_trace_id_valid_context_header(self): trace_id = get_trace_id_from_django() trace_id_returned = get_trace_id() - expected_trace_id = 'testtraceiddjango' self.assertEqual(trace_id, expected_trace_id) self.assertEqual(trace_id, trace_id_returned) From 2889cfc8fc23ce2bde735339d77887136836898b Mon Sep 17 00:00:00 2001 From: Danny Hermes Date: Fri, 2 Jun 2017 11:43:59 -0700 Subject: [PATCH 31/47] Tweaks for style / coverage / lint. --- .../google/cloud/logging/handlers/_helpers.py | 12 +- .../cloud/logging/handlers/app_engine.py | 5 +- .../logging/handlers/middleware/request.py | 6 + .../unit/handlers/middleware/test_request.py | 62 ++++++++-- logging/tests/unit/handlers/test__helpers.py | 115 +++++++++++++----- .../tests/unit/handlers/test_app_engine.py | 36 +++++- 6 files changed, 179 insertions(+), 57 deletions(-) diff --git a/logging/google/cloud/logging/handlers/_helpers.py b/logging/google/cloud/logging/handlers/_helpers.py index e5bdabe2e388..8eb2434a27a0 100644 --- a/logging/google/cloud/logging/handlers/_helpers.py +++ b/logging/google/cloud/logging/handlers/_helpers.py @@ -19,10 +19,11 @@ try: import flask -except ImportError: +except ImportError: # pragma: NO COVER flask = None -from google.cloud.logging.handlers.middleware.request import _get_django_request +from google.cloud.logging.handlers.middleware.request import ( + _get_django_request) _FLASK_TRACE_HEADER = 'X_CLOUD_TRACE_CONTEXT' _DJANGO_TRACE_HEADER = 'HTTP_X_CLOUD_TRACE_CONTEXT' @@ -79,13 +80,10 @@ def get_trace_id_from_django(): if request is None: return None - try: - header = request.META.get(_DJANGO_TRACE_HEADER) - except KeyError: - return None - + header = request.META.get(_DJANGO_TRACE_HEADER) if header is None: return None + trace_id = header.split('/')[0] return trace_id diff --git a/logging/google/cloud/logging/handlers/app_engine.py b/logging/google/cloud/logging/handlers/app_engine.py index f8b12c44a42e..f1998a9ee13e 100644 --- a/logging/google/cloud/logging/handlers/app_engine.py +++ b/logging/google/cloud/logging/handlers/app_engine.py @@ -73,7 +73,10 @@ def get_gae_resource(self): return gae_resource def get_gae_labels(self): - """Return the labels for GAE app which includes trace_id. + """Return the labels for GAE app. + + If the trace ID can be detected, it will be included as a label. + Currently, no other labels are included. :rtype: dict :returns: Labels for GAE app. diff --git a/logging/google/cloud/logging/handlers/middleware/request.py b/logging/google/cloud/logging/handlers/middleware/request.py index 1acf7ffd9961..4c0b22a8e96b 100644 --- a/logging/google/cloud/logging/handlers/middleware/request.py +++ b/logging/google/cloud/logging/handlers/middleware/request.py @@ -12,6 +12,12 @@ # See the License for the specific language governing permissions and # limitations under the License. +"""Django middleware helper to capture a request. + +The request is stored on a thread-local so that it can be +inspected by other helpers. +""" + import threading diff --git a/logging/tests/unit/handlers/middleware/test_request.py b/logging/tests/unit/handlers/middleware/test_request.py index ab8c18d1aa0e..983d67129647 100644 --- a/logging/tests/unit/handlers/middleware/test_request.py +++ b/logging/tests/unit/handlers/middleware/test_request.py @@ -14,16 +14,10 @@ import unittest +import mock -class TestRequestMiddleware(unittest.TestCase): - def _get_target_class(self): - from google.cloud.logging.handlers.middleware.request import RequestMiddleware - - return RequestMiddleware - - def _make_one(self, *args, **kw): - return self._get_target_class()(*args, **kw) +class DjangoBase(unittest.TestCase): @classmethod def setUpClass(cls): @@ -40,11 +34,53 @@ def tearDownClass(cls): teardown_test_environment() - def test_get_django_request(self): + +class TestRequestMiddleware(DjangoBase): + + def _get_target_class(self): + from google.cloud.logging.handlers.middleware import request + + return request.RequestMiddleware + + def _make_one(self, *args, **kw): + return self._get_target_class()(*args, **kw) + + def test_process_request(self): from django.test import RequestFactory - from google.cloud.logging.handlers.middleware.request import _get_django_request + from google.cloud.logging.handlers.middleware import request middleware = self._make_one() - request = RequestFactory().get('/') - middleware.process_request(request) - self.assertEqual(_get_django_request(), request) + mock_request = RequestFactory().get('/') + middleware.process_request(mock_request) + + django_request = request._get_django_request() + self.assertEqual(django_request, mock_request) + + +class Test__get_django_request(DjangoBase): + + @staticmethod + def _call_fut(): + from google.cloud.logging.handlers.middleware import request + + return request._get_django_request() + + @staticmethod + def _make_patch(new_locals): + return mock.patch( + 'google.cloud.logging.handlers.middleware.request._thread_locals', + new=new_locals) + + def test_with_request(self): + thread_locals = mock.Mock(spec=['request']) + with self._make_patch(thread_locals): + django_request = self._call_fut() + + self.assertIs(django_request, thread_locals.request) + + def test_without_request(self): + thread_locals = mock.Mock(spec=[]) + with self._make_patch(thread_locals): + django_request = self._call_fut() + + self.assertIsNone(django_request) diff --git a/logging/tests/unit/handlers/test__helpers.py b/logging/tests/unit/handlers/test__helpers.py index c50ce0044e85..a69e765afa98 100644 --- a/logging/tests/unit/handlers/test__helpers.py +++ b/logging/tests/unit/handlers/test__helpers.py @@ -14,9 +14,17 @@ import unittest +import mock + class Test_get_trace_id_from_flask(unittest.TestCase): + @staticmethod + def _call_fut(): + from google.cloud.logging.handlers import _helpers + + return _helpers.get_trace_id_from_flask() + @staticmethod def create_app(): import flask @@ -30,24 +38,19 @@ def index(): return app def setUp(self): - self.app = Test_get_trace_id_from_flask.create_app() + self.app = self.create_app() - def test_trace_id_no_context_header(self): + def test_no_context_header(self): from google.cloud.logging.handlers import _helpers with self.app.test_request_context( path='/', headers={}): - trace_id = _helpers.get_trace_id_from_flask() - trace_id_returned = _helpers.get_trace_id() + trace_id = self._call_fut() self.assertIsNone(trace_id) - self.assertIsNone(trace_id_returned) - - def test_trace_id_valid_context_header(self): - from google.cloud.logging.handlers._helpers import get_trace_id_from_flask - from google.cloud.logging.handlers._helpers import get_trace_id + def test_valid_context_header(self): flask_trace_header = 'X_CLOUD_TRACE_CONTEXT' expected_trace_id = 'testtraceidflask' flask_trace_id = expected_trace_id + '/testspanid' @@ -57,15 +60,19 @@ def test_trace_id_valid_context_header(self): headers={flask_trace_header: flask_trace_id}) with context: - trace_id = get_trace_id_from_flask() - trace_id_returned = get_trace_id() + trace_id = self._call_fut() self.assertEqual(trace_id, expected_trace_id) - self.assertEqual(trace_id, trace_id_returned) class Test_get_trace_id_from_django(unittest.TestCase): + @staticmethod + def _call_fut(): + from google.cloud.logging.handlers import _helpers + + return _helpers.get_trace_id_from_django() + def setUp(self): from django.conf import settings from django.test.utils import setup_test_environment @@ -76,47 +83,91 @@ def setUp(self): def tearDown(self): from django.test.utils import teardown_test_environment + from google.cloud.logging.handlers.middleware import request teardown_test_environment() + request._thread_locals.__dict__.clear() - def test_trace_id_no_context_header(self): + def test_no_context_header(self): from django.test import RequestFactory - from google.cloud.logging.handlers import _helpers from google.cloud.logging.handlers.middleware import request django_request = RequestFactory().get('/') middleware = request.RequestMiddleware() middleware.process_request(django_request) - trace_id = _helpers.get_trace_id_from_django() - trace_id_returned = _helpers.get_trace_id() - + trace_id = self._call_fut() self.assertIsNone(trace_id) - self.assertIsNone(trace_id_returned) - request._thread_locals.__dict__.clear() - - def test_trace_id_valid_context_header(self): + def test_valid_context_header(self): from django.test import RequestFactory - from google.cloud.logging.handlers._helpers import get_trace_id_from_django - from google.cloud.logging.handlers._helpers import get_trace_id - from google.cloud.logging.handlers.middleware.request import RequestMiddleware - from google.cloud.logging.handlers.middleware.request import _thread_locals + from google.cloud.logging.handlers.middleware import request django_trace_header = 'HTTP_X_CLOUD_TRACE_CONTEXT' expected_trace_id = 'testtraceiddjango' django_trace_id = expected_trace_id + '/testspanid' - request = RequestFactory().get( + django_request = RequestFactory().get( '/', **{django_trace_header: django_trace_id}) - middleware = RequestMiddleware() - middleware.process_request(request) - trace_id = get_trace_id_from_django() - trace_id_returned = get_trace_id() + middleware = request.RequestMiddleware() + middleware.process_request(django_request) + trace_id = self._call_fut() self.assertEqual(trace_id, expected_trace_id) - self.assertEqual(trace_id, trace_id_returned) - _thread_locals.__dict__.clear() + +class Test_get_trace_id(unittest.TestCase): + + @staticmethod + def _call_fut(): + from google.cloud.logging.handlers import _helpers + + return _helpers.get_trace_id() + + def _helper(self, django_return, flask_return): + django_patch = mock.patch( + 'google.cloud.logging.handlers._helpers.get_trace_id_from_django', + return_value=django_return) + flask_patch = mock.patch( + 'google.cloud.logging.handlers._helpers.get_trace_id_from_flask', + return_value=flask_return) + + with django_patch as django_mock: + with flask_patch as flask_mock: + trace_id = self._call_fut() + + return django_mock, flask_mock, trace_id + + def test_from_django(self): + django_mock, flask_mock, trace_id = self._helper( + 'test-django-trace-id', None) + self.assertEqual(trace_id, django_mock.return_value) + + django_mock.assert_called_once_with() + flask_mock.assert_not_called() + + def test_from_flask(self): + django_mock, flask_mock, trace_id = self._helper( + None, 'test-flask-trace-id') + self.assertEqual(trace_id, flask_mock.return_value) + + django_mock.assert_called_once_with() + flask_mock.assert_called_once_with() + + def test_from_django_and_flask(self): + django_mock, flask_mock, trace_id = self._helper( + 'test-django-trace-id', 'test-flask-trace-id') + # Django wins. + self.assertEqual(trace_id, django_mock.return_value) + + django_mock.assert_called_once_with() + flask_mock.assert_not_called() + + def test_missing(self): + django_mock, flask_mock, trace_id = self._helper(None, None) + self.assertIsNone(trace_id) + + django_mock.assert_called_once_with() + flask_mock.assert_called_once_with() diff --git a/logging/tests/unit/handlers/test_app_engine.py b/logging/tests/unit/handlers/test_app_engine.py index ccdaf2992335..6438c4abb8a0 100644 --- a/logging/tests/unit/handlers/test_app_engine.py +++ b/logging/tests/unit/handlers/test_app_engine.py @@ -15,8 +15,10 @@ import logging import unittest +import mock -class TestAppEngineHandlerHandler(unittest.TestCase): + +class TestAppEngineHandler(unittest.TestCase): PROJECT = 'PROJECT' def _get_target_class(self): @@ -28,7 +30,6 @@ def _make_one(self, *args, **kw): return self._get_target_class()(*args, **kw) def test_constructor(self): - import mock from google.cloud.logging.handlers.app_engine import _GAE_PROJECT_ENV from google.cloud.logging.handlers.app_engine import _GAE_SERVICE_ENV from google.cloud.logging.handlers.app_engine import _GAE_VERSION_ENV @@ -48,8 +49,6 @@ def test_constructor(self): self.assertEqual(handler.labels, {}) def test_emit(self): - import mock - client = mock.Mock(project=self.PROJECT, spec=['project']) handler = self._make_one(client, transport=_Transport) gae_resource = handler.get_gae_resource() @@ -66,6 +65,35 @@ def test_emit(self): handler.transport.send_called_with, (record, message, gae_resource, gae_labels)) + def _get_gae_labels_helper(self, trace_id): + get_trace_patch = mock.patch( + 'google.cloud.logging.handlers.app_engine.get_trace_id', + return_value=trace_id) + + client = mock.Mock(project=self.PROJECT, spec=['project']) + # The handler actually calls ``get_gae_labels()``. + with get_trace_patch as mock_get_trace: + handler = self._make_one(client, transport=_Transport) + mock_get_trace.assert_called_once_with() + + gae_labels = handler.get_gae_labels() + self.assertEqual(mock_get_trace.mock_calls, + [mock.call(), mock.call()]) + + return gae_labels + + def test_get_gae_labels_with_label(self): + from google.cloud.logging.handlers import app_engine + + trace_id = 'test-gae-trace-id' + gae_labels = self._get_gae_labels_helper(trace_id) + expected_labels = {app_engine._TRACE_ID_LABEL: trace_id} + self.assertEqual(gae_labels, expected_labels) + + def test_get_gae_labels_without_label(self): + gae_labels = self._get_gae_labels_helper(None) + self.assertEqual(gae_labels, {}) + class _Transport(object): From 195707739b43c11a259a8eb6f2aa32da3aba9857 Mon Sep 17 00:00:00 2001 From: Angela Li Date: Mon, 22 May 2017 16:44:45 -0700 Subject: [PATCH 32/47] Send trace context with logs from web applications --- .../google/cloud/logging/handlers/_helpers.py | 39 ++++++++++++++++++ .../cloud/logging/handlers/app_engine.py | 18 ++++++++- .../google/cloud/logging/handlers/handlers.py | 12 +++++- .../logging/handlers/middleware/__init__.py | 17 ++++++++ .../logging/handlers/middleware/request.py | 40 +++++++++++++++++++ .../handlers/transports/background_thread.py | 13 ++++-- .../cloud/logging/handlers/transports/base.py | 5 ++- .../cloud/logging/handlers/transports/sync.py | 11 ++++- 8 files changed, 146 insertions(+), 9 deletions(-) create mode 100644 logging/google/cloud/logging/handlers/middleware/__init__.py create mode 100644 logging/google/cloud/logging/handlers/middleware/request.py diff --git a/logging/google/cloud/logging/handlers/_helpers.py b/logging/google/cloud/logging/handlers/_helpers.py index 81adcf0eb545..97b44d53ec19 100644 --- a/logging/google/cloud/logging/handlers/_helpers.py +++ b/logging/google/cloud/logging/handlers/_helpers.py @@ -17,6 +17,23 @@ import math import json +try: + import django +except ImportError: + _USE_DJANGO = False +else: + _USE_DJANGO = True + +try: + import flask +except ImportError: + _USE_FLASK = False + flask = None +else: + _USE_FLASK = True + +from google.cloud.logging.handlers.middleware.request import get_request + def format_stackdriver_json(record, message): """Helper to format a LogRecord in in Stackdriver fluentd format. @@ -37,3 +54,25 @@ def format_stackdriver_json(record, message): } return json.dumps(payload) + + +def get_trace_id_from_request_header(): + """Helper to get trace_id from web application request header. + + :rtype: str + :returns: Trace_id in HTTP request headers. + """ + if _USE_FLASK: + try: + trace_id = flask.request.headers['X_CLOUD_TRACE_CONTEXT'].split('/')[0] + except Exception: + trace_id = None + elif _USE_DJANGO: + try: + request = get_request() + trace_id = request.META['HTTP_X_CLOUD_TRACE_CONTEXT'].split('/')[0] + except Exception: + trace_id = None + else: + trace_id = None + return trace_id diff --git a/logging/google/cloud/logging/handlers/app_engine.py b/logging/google/cloud/logging/handlers/app_engine.py index 7011819f8a2f..04e35d3c547e 100644 --- a/logging/google/cloud/logging/handlers/app_engine.py +++ b/logging/google/cloud/logging/handlers/app_engine.py @@ -20,6 +20,7 @@ import os +from google.cloud.logging.handlers._helpers import get_trace_id_from_request_header from google.cloud.logging.handlers.handlers import CloudLoggingHandler from google.cloud.logging.handlers.transports import BackgroundThreadTransport from google.cloud.logging.resource import Resource @@ -50,7 +51,8 @@ def __init__(self, client, client, name=_DEFAULT_GAE_LOGGER_NAME, transport=transport, - resource=self.get_gae_resource()) + resource=self.get_gae_resource(), + labels=self.get_gae_labels()) def get_gae_resource(self): """Return the GAE resource using the environment variables. @@ -67,3 +69,17 @@ def get_gae_resource(self): }, ) return gae_resource + + def get_gae_labels(self): + """Return the labels for GAE app which includes trace_id. + + :rtype: dict + :returns: Labels for GAE app. + """ + trace_id = get_trace_id_from_request_header() + if trace_id is None: + trace_id = 'unknown' + gae_labels = { + 'appengine.googleapis.com/trace_id': trace_id, + } + return gae_labels \ No newline at end of file diff --git a/logging/google/cloud/logging/handlers/handlers.py b/logging/google/cloud/logging/handlers/handlers.py index 97afde9f87fb..7f094bba19b1 100644 --- a/logging/google/cloud/logging/handlers/handlers.py +++ b/logging/google/cloud/logging/handlers/handlers.py @@ -57,6 +57,9 @@ class CloudLoggingHandler(logging.StreamHandler): :param resource: (Optional) Monitored resource of the entry, defaults to the global resource type. + :type labels: dict + :param labels: (Optional) Mapping of labels for the entry. + Example: .. code-block:: python @@ -79,12 +82,14 @@ class CloudLoggingHandler(logging.StreamHandler): def __init__(self, client, name=DEFAULT_LOGGER_NAME, transport=BackgroundThreadTransport, - resource=_GLOBAL_RESOURCE): + resource=_GLOBAL_RESOURCE, + labels=None): super(CloudLoggingHandler, self).__init__() self.name = name self.client = client self.transport = transport(client, name) self.resource = resource + self.labels = labels def emit(self, record): """Actually log the specified logging record. @@ -97,7 +102,10 @@ def emit(self, record): :param record: The record to be logged. """ message = super(CloudLoggingHandler, self).format(record) - self.transport.send(record, message, resource=self.resource) + self.transport.send(record, + message, + resource=self.resource, + labels=self.labels) def setup_logging(handler, excluded_loggers=EXCLUDED_LOGGER_DEFAULTS, diff --git a/logging/google/cloud/logging/handlers/middleware/__init__.py b/logging/google/cloud/logging/handlers/middleware/__init__.py new file mode 100644 index 000000000000..79c3abee7894 --- /dev/null +++ b/logging/google/cloud/logging/handlers/middleware/__init__.py @@ -0,0 +1,17 @@ +# Copyright 2016 Google Inc. All Rights Reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +from google.cloud.logging.handlers.middleware.request import RequestMiddleware + +__all__ = ['RequestMiddleware'] diff --git a/logging/google/cloud/logging/handlers/middleware/request.py b/logging/google/cloud/logging/handlers/middleware/request.py new file mode 100644 index 000000000000..0c6d1f46c5cd --- /dev/null +++ b/logging/google/cloud/logging/handlers/middleware/request.py @@ -0,0 +1,40 @@ +# Copyright 2016 Google Inc. All Rights Reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +try: + from threading import local +except ImportError: + from django.utils._threading_local import local + +_thread_locals = local() + + +def get_request(): + """Get Django request from thread local. + + :rtype: str + :returns: Django request. + """ + return getattr(_thread_locals, 'request', None) + +class RequestMiddleware(object): + """Saves the request in thread local""" + + def process_request(self, request): + """Called on each request, before Django decides which view to execute. + + :type request: :class:`~django.http.request.HttpRequest` + :param request: Django http request. + """ + _thread_locals.request = request diff --git a/logging/google/cloud/logging/handlers/transports/background_thread.py b/logging/google/cloud/logging/handlers/transports/background_thread.py index 010c06b36bc9..d889bed62626 100644 --- a/logging/google/cloud/logging/handlers/transports/background_thread.py +++ b/logging/google/cloud/logging/handlers/transports/background_thread.py @@ -203,7 +203,7 @@ def _main_thread_terminated(self): else: print('Failed to send %d pending logs.' % (self._queue.qsize(),)) - def enqueue(self, record, message, resource=None): + def enqueue(self, record, message, resource=None, labels=None): """Queues a log entry to be written by the background thread. :type record: :class:`logging.LogRecord` @@ -215,6 +215,9 @@ def enqueue(self, record, message, resource=None): :type resource: :class:`~google.cloud.logging.resource.Resource` :param resource: (Optional) Monitored resource of the entry + + :type labels: dict + :param labels: (Optional) Mapping of labels for the entry. """ self._queue.put_nowait({ 'info': { @@ -223,6 +226,7 @@ def enqueue(self, record, message, resource=None): }, 'severity': record.levelname, 'resource': resource, + 'labels': labels, }) def flush(self): @@ -257,7 +261,7 @@ def __init__(self, client, name, grace_period=_DEFAULT_GRACE_PERIOD, self.worker = _Worker(logger) self.worker.start() - def send(self, record, message, resource=None): + def send(self, record, message, resource=None, labels=None): """Overrides Transport.send(). :type record: :class:`logging.LogRecord` @@ -269,8 +273,11 @@ def send(self, record, message, resource=None): :type resource: :class:`~google.cloud.logging.resource.Resource` :param resource: (Optional) Monitored resource of the entry. + + :type labels: dict + :param labels: (Optional) Mapping of labels for the entry. """ - self.worker.enqueue(record, message, resource=resource) + self.worker.enqueue(record, message, resource=resource, labels=labels) def flush(self): """Submit any pending log records.""" diff --git a/logging/google/cloud/logging/handlers/transports/base.py b/logging/google/cloud/logging/handlers/transports/base.py index 21957021793f..7829201b1c98 100644 --- a/logging/google/cloud/logging/handlers/transports/base.py +++ b/logging/google/cloud/logging/handlers/transports/base.py @@ -22,7 +22,7 @@ class Transport(object): client and name object, and must override :meth:`send`. """ - def send(self, record, message, resource=None): + def send(self, record, message, resource=None, labels=None): """Transport send to be implemented by subclasses. :type record: :class:`logging.LogRecord` @@ -34,6 +34,9 @@ def send(self, record, message, resource=None): :type resource: :class:`~google.cloud.logging.resource.Resource` :param resource: (Optional) Monitored resource of the entry. + + :type labels: dict + :param labels: (Optional) Mapping of labels for the entry. """ raise NotImplementedError diff --git a/logging/google/cloud/logging/handlers/transports/sync.py b/logging/google/cloud/logging/handlers/transports/sync.py index 0dd6e0bd7e24..be70e60a14e1 100644 --- a/logging/google/cloud/logging/handlers/transports/sync.py +++ b/logging/google/cloud/logging/handlers/transports/sync.py @@ -29,7 +29,7 @@ class SyncTransport(Transport): def __init__(self, client, name): self.logger = client.logger(name) - def send(self, record, message, resource=None): + def send(self, record, message, resource=None, labels=None): """Overrides transport.send(). :type record: :class:`logging.LogRecord` @@ -38,8 +38,15 @@ def send(self, record, message, resource=None): :type message: str :param message: The message from the ``LogRecord`` after being formatted by the associated log formatters. + + :type resource: :class:`~google.cloud.logging.resource.Resource` + :param resource: (Optional) Monitored resource of the entry. + + :type labels: dict + :param labels: (Optional) Mapping of labels for the entry. """ info = {'message': message, 'python_logger': record.name} self.logger.log_struct(info, severity=record.levelname, - resource=resource) + resource=resource, + labels=labels) From bf683f7e14b9d25b48a727f416cd63669c12480f Mon Sep 17 00:00:00 2001 From: Angela Li Date: Mon, 22 May 2017 16:58:43 -0700 Subject: [PATCH 33/47] Fix style --- logging/google/cloud/logging/handlers/app_engine.py | 2 +- logging/google/cloud/logging/handlers/middleware/request.py | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/logging/google/cloud/logging/handlers/app_engine.py b/logging/google/cloud/logging/handlers/app_engine.py index 04e35d3c547e..4d5972edfa51 100644 --- a/logging/google/cloud/logging/handlers/app_engine.py +++ b/logging/google/cloud/logging/handlers/app_engine.py @@ -82,4 +82,4 @@ def get_gae_labels(self): gae_labels = { 'appengine.googleapis.com/trace_id': trace_id, } - return gae_labels \ No newline at end of file + return gae_labels diff --git a/logging/google/cloud/logging/handlers/middleware/request.py b/logging/google/cloud/logging/handlers/middleware/request.py index 0c6d1f46c5cd..a37a6af47035 100644 --- a/logging/google/cloud/logging/handlers/middleware/request.py +++ b/logging/google/cloud/logging/handlers/middleware/request.py @@ -28,6 +28,7 @@ def get_request(): """ return getattr(_thread_locals, 'request', None) + class RequestMiddleware(object): """Saves the request in thread local""" From 51cdb39597937c29c4de505b3b4904396d7d3ea5 Mon Sep 17 00:00:00 2001 From: Angela Li Date: Tue, 23 May 2017 14:23:32 -0700 Subject: [PATCH 34/47] Improved code for web framework detection --- .../google/cloud/logging/handlers/_helpers.py | 37 ++++++++++++++----- 1 file changed, 27 insertions(+), 10 deletions(-) diff --git a/logging/google/cloud/logging/handlers/_helpers.py b/logging/google/cloud/logging/handlers/_helpers.py index 97b44d53ec19..a1c697ba404c 100644 --- a/logging/google/cloud/logging/handlers/_helpers.py +++ b/logging/google/cloud/logging/handlers/_helpers.py @@ -16,24 +16,18 @@ import math import json - -try: - import django -except ImportError: - _USE_DJANGO = False -else: - _USE_DJANGO = True +import sys try: import flask except ImportError: - _USE_FLASK = False flask = None -else: - _USE_FLASK = True from google.cloud.logging.handlers.middleware.request import get_request +_USE_FLASK = False +_USE_DJANGO = False + def format_stackdriver_json(record, message): """Helper to format a LogRecord in in Stackdriver fluentd format. @@ -56,12 +50,35 @@ def format_stackdriver_json(record, message): return json.dumps(payload) +def detect_web_framework(): + """Detect web framework used in this environment. + + Detect which web framework is used by looking at the sys.modules. + If multiple or no supported web frameworks detected in the modules, then + print a warning message. + Set _USE_FLASK or _USE_DJANGO to True if one of them are detected. + """ + modules = sys.modules + global _USE_FLASK, _USE_DJANGO + + if 'flask' in modules and 'django' in modules: + print('Cannot determine, found multiple web frameworks.') + elif 'flask' in modules: + _USE_FLASK = True + elif 'django' in modules: + _USE_DJANGO = True + else: + print('No supported web framework found in the modules.') + + def get_trace_id_from_request_header(): """Helper to get trace_id from web application request header. :rtype: str :returns: Trace_id in HTTP request headers. """ + detect_web_framework() + if _USE_FLASK: try: trace_id = flask.request.headers['X_CLOUD_TRACE_CONTEXT'].split('/')[0] From 2479d916625eebd27dc4627ade02115fa6534723 Mon Sep 17 00:00:00 2001 From: Angela Li Date: Tue, 23 May 2017 14:28:01 -0700 Subject: [PATCH 35/47] Fix year --- logging/google/cloud/logging/handlers/middleware/__init__.py | 2 +- logging/google/cloud/logging/handlers/middleware/request.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/logging/google/cloud/logging/handlers/middleware/__init__.py b/logging/google/cloud/logging/handlers/middleware/__init__.py index 79c3abee7894..c340235b8bdd 100644 --- a/logging/google/cloud/logging/handlers/middleware/__init__.py +++ b/logging/google/cloud/logging/handlers/middleware/__init__.py @@ -1,4 +1,4 @@ -# Copyright 2016 Google Inc. All Rights Reserved. +# Copyright 2017 Google Inc. All Rights Reserved. # # Licensed under the Apache License, Version 2.0 (the "License"); # you may not use this file except in compliance with the License. diff --git a/logging/google/cloud/logging/handlers/middleware/request.py b/logging/google/cloud/logging/handlers/middleware/request.py index a37a6af47035..be5102487cc6 100644 --- a/logging/google/cloud/logging/handlers/middleware/request.py +++ b/logging/google/cloud/logging/handlers/middleware/request.py @@ -1,4 +1,4 @@ -# Copyright 2016 Google Inc. All Rights Reserved. +# Copyright 2017 Google Inc. All Rights Reserved. # # Licensed under the Apache License, Version 2.0 (the "License"); # you may not use this file except in compliance with the License. From 6c04196b1e738e3bbe399f3fd6558b6f552e3930 Mon Sep 17 00:00:00 2001 From: Angela Li Date: Tue, 23 May 2017 14:45:28 -0700 Subject: [PATCH 36/47] Drop module level variables --- .../google/cloud/logging/handlers/_helpers.py | 22 ++++++++++--------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/logging/google/cloud/logging/handlers/_helpers.py b/logging/google/cloud/logging/handlers/_helpers.py index a1c697ba404c..6bf0edf79d54 100644 --- a/logging/google/cloud/logging/handlers/_helpers.py +++ b/logging/google/cloud/logging/handlers/_helpers.py @@ -25,9 +25,6 @@ from google.cloud.logging.handlers.middleware.request import get_request -_USE_FLASK = False -_USE_DJANGO = False - def format_stackdriver_json(record, message): """Helper to format a LogRecord in in Stackdriver fluentd format. @@ -56,19 +53,24 @@ def detect_web_framework(): Detect which web framework is used by looking at the sys.modules. If multiple or no supported web frameworks detected in the modules, then print a warning message. - Set _USE_FLASK or _USE_DJANGO to True if one of them are detected. + Return the name of web framework detected if flask or django is found. + Return unknown if cannot determine. + + :rtype: str + :returns: Web framework detected in this environment. """ modules = sys.modules - global _USE_FLASK, _USE_DJANGO + web_framework = 'unknown' if 'flask' in modules and 'django' in modules: print('Cannot determine, found multiple web frameworks.') elif 'flask' in modules: - _USE_FLASK = True + web_framework = 'flask' elif 'django' in modules: - _USE_DJANGO = True + web_framework = 'django' else: print('No supported web framework found in the modules.') + return web_framework def get_trace_id_from_request_header(): @@ -77,14 +79,14 @@ def get_trace_id_from_request_header(): :rtype: str :returns: Trace_id in HTTP request headers. """ - detect_web_framework() + web_framework = detect_web_framework() - if _USE_FLASK: + if web_framework is 'flask': try: trace_id = flask.request.headers['X_CLOUD_TRACE_CONTEXT'].split('/')[0] except Exception: trace_id = None - elif _USE_DJANGO: + elif web_framework is 'django': try: request = get_request() trace_id = request.META['HTTP_X_CLOUD_TRACE_CONTEXT'].split('/')[0] From f70c84eba51d3a7b5d9bb407edd9b3b1017913aa Mon Sep 17 00:00:00 2001 From: Angela Li Date: Thu, 25 May 2017 10:07:25 -0700 Subject: [PATCH 37/47] Address Jon's comments and add some unit tests (not complete yet) --- .../google/cloud/logging/handlers/_helpers.py | 66 ++++++++----------- .../cloud/logging/handlers/app_engine.py | 4 +- .../google/cloud/logging/handlers/handlers.py | 9 +-- .../logging/handlers/middleware/request.py | 6 +- .../tests/unit/handlers/test_app_engine.py | 11 +++- logging/tests/unit/handlers/test_handlers.py | 6 +- .../transports/test_background_thread.py | 8 +-- .../unit/handlers/transports/test_sync.py | 6 +- 8 files changed, 56 insertions(+), 60 deletions(-) diff --git a/logging/google/cloud/logging/handlers/_helpers.py b/logging/google/cloud/logging/handlers/_helpers.py index 6bf0edf79d54..0af7a53de476 100644 --- a/logging/google/cloud/logging/handlers/_helpers.py +++ b/logging/google/cloud/logging/handlers/_helpers.py @@ -16,7 +16,6 @@ import math import json -import sys try: import flask @@ -47,51 +46,44 @@ def format_stackdriver_json(record, message): return json.dumps(payload) -def detect_web_framework(): - """Detect web framework used in this environment. - - Detect which web framework is used by looking at the sys.modules. - If multiple or no supported web frameworks detected in the modules, then - print a warning message. - Return the name of web framework detected if flask or django is found. - Return unknown if cannot determine. +def get_trace_id_from_flask(): + """Get trace_id from flask request headers. :rtype: str - :returns: Web framework detected in this environment. + :return: Trace_id in HTTP request headers. """ - modules = sys.modules - web_framework = 'unknown' + try: + trace_id = flask.request.headers['X_CLOUD_TRACE_CONTEXT'].split('/')[0] + except Exception: + trace_id = None + return trace_id - if 'flask' in modules and 'django' in modules: - print('Cannot determine, found multiple web frameworks.') - elif 'flask' in modules: - web_framework = 'flask' - elif 'django' in modules: - web_framework = 'django' - else: - print('No supported web framework found in the modules.') - return web_framework + +def get_trace_id_from_django(): + """Get trace_id from django request headers. + + :rtype: str + :return: Trace_id in HTTP request headers. + """ + try: + request = get_request() + trace_id = request.META['HTTP_X_CLOUD_TRACE_CONTEXT'].split('/')[0] + except Exception: + trace_id = None + return trace_id -def get_trace_id_from_request_header(): +def get_trace_id(): """Helper to get trace_id from web application request header. :rtype: str :returns: Trace_id in HTTP request headers. """ - web_framework = detect_web_framework() - - if web_framework is 'flask': - try: - trace_id = flask.request.headers['X_CLOUD_TRACE_CONTEXT'].split('/')[0] - except Exception: - trace_id = None - elif web_framework is 'django': - try: - request = get_request() - trace_id = request.META['HTTP_X_CLOUD_TRACE_CONTEXT'].split('/')[0] - except Exception: - trace_id = None - else: - trace_id = None + checkers = [get_trace_id_from_django, get_trace_id_from_flask] + + for checker in checkers: + trace_id = checker() + if trace_id is not None: + return trace_id + return trace_id diff --git a/logging/google/cloud/logging/handlers/app_engine.py b/logging/google/cloud/logging/handlers/app_engine.py index 4d5972edfa51..8ebb460f23c6 100644 --- a/logging/google/cloud/logging/handlers/app_engine.py +++ b/logging/google/cloud/logging/handlers/app_engine.py @@ -20,7 +20,7 @@ import os -from google.cloud.logging.handlers._helpers import get_trace_id_from_request_header +from google.cloud.logging.handlers._helpers import get_trace_id from google.cloud.logging.handlers.handlers import CloudLoggingHandler from google.cloud.logging.handlers.transports import BackgroundThreadTransport from google.cloud.logging.resource import Resource @@ -76,7 +76,7 @@ def get_gae_labels(self): :rtype: dict :returns: Labels for GAE app. """ - trace_id = get_trace_id_from_request_header() + trace_id = get_trace_id() if trace_id is None: trace_id = 'unknown' gae_labels = { diff --git a/logging/google/cloud/logging/handlers/handlers.py b/logging/google/cloud/logging/handlers/handlers.py index 7f094bba19b1..fe9848848d38 100644 --- a/logging/google/cloud/logging/handlers/handlers.py +++ b/logging/google/cloud/logging/handlers/handlers.py @@ -102,10 +102,11 @@ def emit(self, record): :param record: The record to be logged. """ message = super(CloudLoggingHandler, self).format(record) - self.transport.send(record, - message, - resource=self.resource, - labels=self.labels) + self.transport.send( + record, + message, + resource=self.resource, + labels=self.labels) def setup_logging(handler, excluded_loggers=EXCLUDED_LOGGER_DEFAULTS, diff --git a/logging/google/cloud/logging/handlers/middleware/request.py b/logging/google/cloud/logging/handlers/middleware/request.py index be5102487cc6..92bfd4369b00 100644 --- a/logging/google/cloud/logging/handlers/middleware/request.py +++ b/logging/google/cloud/logging/handlers/middleware/request.py @@ -12,10 +12,8 @@ # See the License for the specific language governing permissions and # limitations under the License. -try: - from threading import local -except ImportError: - from django.utils._threading_local import local +from threading import local + _thread_locals = local() diff --git a/logging/tests/unit/handlers/test_app_engine.py b/logging/tests/unit/handlers/test_app_engine.py index c39328593f7a..99f5ea93625a 100644 --- a/logging/tests/unit/handlers/test_app_engine.py +++ b/logging/tests/unit/handlers/test_app_engine.py @@ -34,6 +34,7 @@ def test_constructor(self): from google.cloud.logging.handlers.app_engine import _GAE_VERSION_ENV client = mock.Mock(project=self.PROJECT, spec=['project']) + with mock.patch('os.environ', new={_GAE_PROJECT_ENV: 'test_project', _GAE_SERVICE_ENV: 'test_service', _GAE_VERSION_ENV: 'test_version'}): @@ -43,6 +44,7 @@ def test_constructor(self): self.assertEqual(handler.resource.labels['project_id'], 'test_project') self.assertEqual(handler.resource.labels['module_id'], 'test_service') self.assertEqual(handler.resource.labels['version_id'], 'test_version') + self.assertEqual(handler.labels['appengine.googleapis.com/trace_id'], 'unknown') def test_emit(self): import mock @@ -50,6 +52,7 @@ def test_emit(self): client = mock.Mock(project=self.PROJECT, spec=['project']) handler = self._make_one(client, transport=_Transport) gae_resource = handler.get_gae_resource() + gae_labels = handler.get_gae_labels() logname = 'app' message = 'hello world' record = logging.LogRecord(logname, logging, None, None, message, @@ -58,7 +61,9 @@ def test_emit(self): self.assertIs(handler.transport.client, client) self.assertEqual(handler.transport.name, logname) - self.assertEqual(handler.transport.send_called_with, (record, message, gae_resource)) + self.assertEqual( + handler.transport.send_called_with, + (record, message, gae_resource, gae_labels)) class _Transport(object): @@ -67,5 +72,5 @@ def __init__(self, client, name): self.client = client self.name = name - def send(self, record, message, resource): - self.send_called_with = (record, message, resource) + def send(self, record, message, resource, labels): + self.send_called_with = (record, message, resource, labels) diff --git a/logging/tests/unit/handlers/test_handlers.py b/logging/tests/unit/handlers/test_handlers.py index 05dc87631478..5459a0c10b96 100644 --- a/logging/tests/unit/handlers/test_handlers.py +++ b/logging/tests/unit/handlers/test_handlers.py @@ -45,7 +45,7 @@ def test_emit(self): None, None) handler.emit(record) - self.assertEqual(handler.transport.send_called_with, (record, message, _GLOBAL_RESOURCE)) + self.assertEqual(handler.transport.send_called_with, (record, message, _GLOBAL_RESOURCE, None)) class TestSetupLogging(unittest.TestCase): @@ -110,5 +110,5 @@ class _Transport(object): def __init__(self, client, name): pass - def send(self, record, message, resource): - self.send_called_with = (record, message, resource) + def send(self, record, message, resource, labels=None): + self.send_called_with = (record, message, resource, labels) diff --git a/logging/tests/unit/handlers/transports/test_background_thread.py b/logging/tests/unit/handlers/transports/test_background_thread.py index 3e3378dcd361..b9449ce56685 100644 --- a/logging/tests/unit/handlers/transports/test_background_thread.py +++ b/logging/tests/unit/handlers/transports/test_background_thread.py @@ -61,9 +61,9 @@ def test_send(self): python_logger_name, logging.INFO, None, None, message, None, None) - transport.send(record, message, _GLOBAL_RESOURCE) + transport.send(record, message, _GLOBAL_RESOURCE, None) - transport.worker.enqueue.assert_called_once_with(record, message, _GLOBAL_RESOURCE) + transport.worker.enqueue.assert_called_once_with(record, message, _GLOBAL_RESOURCE, None) def test_flush(self): client = _Client(self.PROJECT) @@ -287,13 +287,13 @@ def __init__(self): self.commit_called = False self.commit_count = None - def log_struct(self, info, severity=logging.INFO, resource=None): + def log_struct(self, info, severity=logging.INFO, resource=None, labels=None): from google.cloud.logging.logger import _GLOBAL_RESOURCE assert resource is None resource = _GLOBAL_RESOURCE - self.log_struct_called_with = (info, severity, resource) + self.log_struct_called_with = (info, severity, resource, labels) self.entries.append(info) def commit(self): diff --git a/logging/tests/unit/handlers/transports/test_sync.py b/logging/tests/unit/handlers/transports/test_sync.py index 475ecc9c6a71..e18920ce2167 100644 --- a/logging/tests/unit/handlers/transports/test_sync.py +++ b/logging/tests/unit/handlers/transports/test_sync.py @@ -52,7 +52,7 @@ def test_send(self): 'message': message, 'python_logger': python_logger_name, } - EXPECTED_SENT = (EXPECTED_STRUCT, 'INFO', _GLOBAL_RESOURCE) + EXPECTED_SENT = (EXPECTED_STRUCT, 'INFO', _GLOBAL_RESOURCE, None) self.assertEqual( transport.logger.log_struct_called_with, EXPECTED_SENT) @@ -63,8 +63,8 @@ class _Logger(object): def __init__(self, name): self.name = name - def log_struct(self, message, severity=None, resource=_GLOBAL_RESOURCE): - self.log_struct_called_with = (message, severity, resource) + def log_struct(self, message, severity=None, resource=_GLOBAL_RESOURCE, labels=None): + self.log_struct_called_with = (message, severity, resource, labels) class _Client(object): From 6fc4b21faff5ee1a231904e82304d250344c71dd Mon Sep 17 00:00:00 2001 From: Angela Li Date: Thu, 25 May 2017 10:45:36 -0700 Subject: [PATCH 38/47] Fix stuff --- .../google/cloud/logging/handlers/_helpers.py | 35 ++++++++++++++----- .../logging/handlers/middleware/request.py | 17 +++++---- .../tests/unit/handlers/test_app_engine.py | 4 ++- 3 files changed, 37 insertions(+), 19 deletions(-) diff --git a/logging/google/cloud/logging/handlers/_helpers.py b/logging/google/cloud/logging/handlers/_helpers.py index 0af7a53de476..ca94e17ebb92 100644 --- a/logging/google/cloud/logging/handlers/_helpers.py +++ b/logging/google/cloud/logging/handlers/_helpers.py @@ -22,7 +22,10 @@ except ImportError: flask = None -from google.cloud.logging.handlers.middleware.request import get_request +from google.cloud.logging.handlers.middleware.request import RequestMiddleware + +_FLASK_TRACE_HEADER = 'X_CLOUD_TRACE_CONTEXT' +_DJANGO_TRACE_HEADER = 'HTTP_X_CLOUD_TRACE_CONTEXT' def format_stackdriver_json(record, message): @@ -52,10 +55,16 @@ def get_trace_id_from_flask(): :rtype: str :return: Trace_id in HTTP request headers. """ - try: - trace_id = flask.request.headers['X_CLOUD_TRACE_CONTEXT'].split('/')[0] - except Exception: - trace_id = None + if not flask or not flask.request: + return None + + header = flask.request.headers.get(_FLASK_TRACE_HEADER) + + if not header: + return None + + trace_id = header.split('/')[0] + return trace_id @@ -65,11 +74,19 @@ def get_trace_id_from_django(): :rtype: str :return: Trace_id in HTTP request headers. """ + request_middleware = RequestMiddleware() + request = request_middleware.get_request() + + if not request: + return None + try: - request = get_request() - trace_id = request.META['HTTP_X_CLOUD_TRACE_CONTEXT'].split('/')[0] - except Exception: - trace_id = None + header = request.META[_DJANGO_TRACE_HEADER] + except KeyError: + return None + + trace_id = header.split('/')[0] + return trace_id diff --git a/logging/google/cloud/logging/handlers/middleware/request.py b/logging/google/cloud/logging/handlers/middleware/request.py index 92bfd4369b00..7e0e89f6aa39 100644 --- a/logging/google/cloud/logging/handlers/middleware/request.py +++ b/logging/google/cloud/logging/handlers/middleware/request.py @@ -18,15 +18,6 @@ _thread_locals = local() -def get_request(): - """Get Django request from thread local. - - :rtype: str - :returns: Django request. - """ - return getattr(_thread_locals, 'request', None) - - class RequestMiddleware(object): """Saves the request in thread local""" @@ -37,3 +28,11 @@ def process_request(self, request): :param request: Django http request. """ _thread_locals.request = request + + def get_request(self): + """Get Django request from thread local. + + :rtype: str + :returns: Django request. + """ + return getattr(_thread_locals, 'request', None) diff --git a/logging/tests/unit/handlers/test_app_engine.py b/logging/tests/unit/handlers/test_app_engine.py index 99f5ea93625a..0f8ace3a95e7 100644 --- a/logging/tests/unit/handlers/test_app_engine.py +++ b/logging/tests/unit/handlers/test_app_engine.py @@ -33,6 +33,8 @@ def test_constructor(self): from google.cloud.logging.handlers.app_engine import _GAE_SERVICE_ENV from google.cloud.logging.handlers.app_engine import _GAE_VERSION_ENV + TRACE_ID_LABEL = 'appengine.googleapis.com/trace_id' + client = mock.Mock(project=self.PROJECT, spec=['project']) with mock.patch('os.environ', new={_GAE_PROJECT_ENV: 'test_project', @@ -44,7 +46,7 @@ def test_constructor(self): self.assertEqual(handler.resource.labels['project_id'], 'test_project') self.assertEqual(handler.resource.labels['module_id'], 'test_service') self.assertEqual(handler.resource.labels['version_id'], 'test_version') - self.assertEqual(handler.labels['appengine.googleapis.com/trace_id'], 'unknown') + self.assertEqual(handler.labels[TRACE_ID_LABEL], 'unknown') def test_emit(self): import mock From 2106bfc8bf6cdc68515f4954bbf057bc3edcfa1a Mon Sep 17 00:00:00 2001 From: Angela Li Date: Thu, 25 May 2017 14:54:23 -0700 Subject: [PATCH 39/47] Add unit test for flask and some refactor --- .../google/cloud/logging/handlers/_helpers.py | 12 ++-- .../cloud/logging/handlers/app_engine.py | 2 - logging/nox.py | 2 +- logging/tests/unit/handlers/test__helpers.py | 57 +++++++++++++++++++ .../tests/unit/handlers/test_app_engine.py | 3 +- 5 files changed, 67 insertions(+), 9 deletions(-) create mode 100644 logging/tests/unit/handlers/test__helpers.py diff --git a/logging/google/cloud/logging/handlers/_helpers.py b/logging/google/cloud/logging/handlers/_helpers.py index ca94e17ebb92..17caf76e4aa9 100644 --- a/logging/google/cloud/logging/handlers/_helpers.py +++ b/logging/google/cloud/logging/handlers/_helpers.py @@ -27,6 +27,8 @@ _FLASK_TRACE_HEADER = 'X_CLOUD_TRACE_CONTEXT' _DJANGO_TRACE_HEADER = 'HTTP_X_CLOUD_TRACE_CONTEXT' +_EMPTY_TRACE_ID = 'None' + def format_stackdriver_json(record, message): """Helper to format a LogRecord in in Stackdriver fluentd format. @@ -56,12 +58,12 @@ def get_trace_id_from_flask(): :return: Trace_id in HTTP request headers. """ if not flask or not flask.request: - return None + return _EMPTY_TRACE_ID header = flask.request.headers.get(_FLASK_TRACE_HEADER) if not header: - return None + return _EMPTY_TRACE_ID trace_id = header.split('/')[0] @@ -78,12 +80,12 @@ def get_trace_id_from_django(): request = request_middleware.get_request() if not request: - return None + return _EMPTY_TRACE_ID try: header = request.META[_DJANGO_TRACE_HEADER] except KeyError: - return None + return _EMPTY_TRACE_ID trace_id = header.split('/')[0] @@ -100,7 +102,7 @@ def get_trace_id(): for checker in checkers: trace_id = checker() - if trace_id is not None: + if trace_id is not _EMPTY_TRACE_ID: return trace_id return trace_id diff --git a/logging/google/cloud/logging/handlers/app_engine.py b/logging/google/cloud/logging/handlers/app_engine.py index 8ebb460f23c6..ccab4101848b 100644 --- a/logging/google/cloud/logging/handlers/app_engine.py +++ b/logging/google/cloud/logging/handlers/app_engine.py @@ -77,8 +77,6 @@ def get_gae_labels(self): :returns: Labels for GAE app. """ trace_id = get_trace_id() - if trace_id is None: - trace_id = 'unknown' gae_labels = { 'appengine.googleapis.com/trace_id': trace_id, } diff --git a/logging/nox.py b/logging/nox.py index 5d4751a955a5..11d484b232e7 100644 --- a/logging/nox.py +++ b/logging/nox.py @@ -31,7 +31,7 @@ def unit_tests(session, python_version): session.interpreter = 'python{}'.format(python_version) # Install all test dependencies, then install this package in-place. - session.install('mock', 'pytest', 'pytest-cov', *LOCAL_DEPS) + session.install('mock', 'pytest', 'pytest-cov', 'flask', *LOCAL_DEPS) session.install('-e', '.') # Run py.test against the unit tests. diff --git a/logging/tests/unit/handlers/test__helpers.py b/logging/tests/unit/handlers/test__helpers.py new file mode 100644 index 000000000000..db9d51d27920 --- /dev/null +++ b/logging/tests/unit/handlers/test__helpers.py @@ -0,0 +1,57 @@ +# Copyright 2017 Google Inc. All Rights Reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +import unittest + + +class TestFlaskTrace(unittest.TestCase): + + def create_app(self): + import flask + + app = flask.Flask(__name__) + + @app.route('/') + def index(): + return 'test flask trace' + + return app + + def setUp(self): + self.app = self.create_app() + + def test_trace_id_no_context_header(self): + from google.cloud.logging.handlers._helpers import get_trace_id_from_flask + from google.cloud.logging.handlers._helpers import _EMPTY_TRACE_ID + + with self.app.test_request_context( + path='/', + headers={}): + trace_id = get_trace_id_from_flask() + + self.assertEqual(trace_id, _EMPTY_TRACE_ID) + + def test_trace_id_valid_context_header(self): + from google.cloud.logging.handlers._helpers import get_trace_id_from_flask + + FLASK_TRACE_HEADER = 'X_CLOUD_TRACE_CONTEXT' + FLASK_TRACE_ID = 'testtraceid/testspanid' + + with self.app.test_request_context( + path='/', + headers={FLASK_TRACE_HEADER:FLASK_TRACE_ID}): + trace_id = get_trace_id_from_flask() + + EXPECTED_TRACE_ID = 'testtraceid' + self.assertEqual(trace_id, EXPECTED_TRACE_ID) diff --git a/logging/tests/unit/handlers/test_app_engine.py b/logging/tests/unit/handlers/test_app_engine.py index 0f8ace3a95e7..886dbbbb5584 100644 --- a/logging/tests/unit/handlers/test_app_engine.py +++ b/logging/tests/unit/handlers/test_app_engine.py @@ -32,6 +32,7 @@ def test_constructor(self): from google.cloud.logging.handlers.app_engine import _GAE_PROJECT_ENV from google.cloud.logging.handlers.app_engine import _GAE_SERVICE_ENV from google.cloud.logging.handlers.app_engine import _GAE_VERSION_ENV + from google.cloud.logging.handlers._helpers import _EMPTY_TRACE_ID TRACE_ID_LABEL = 'appengine.googleapis.com/trace_id' @@ -46,7 +47,7 @@ def test_constructor(self): self.assertEqual(handler.resource.labels['project_id'], 'test_project') self.assertEqual(handler.resource.labels['module_id'], 'test_service') self.assertEqual(handler.resource.labels['version_id'], 'test_version') - self.assertEqual(handler.labels[TRACE_ID_LABEL], 'unknown') + self.assertEqual(handler.labels[TRACE_ID_LABEL], _EMPTY_TRACE_ID) def test_emit(self): import mock From c66fc317de4595b8ed361bca17496f4d5e3fceef Mon Sep 17 00:00:00 2001 From: Angela Li Date: Fri, 26 May 2017 10:20:56 -0700 Subject: [PATCH 40/47] Add unit test for middleware --- logging/nox.py | 2 +- .../unit/handlers/middleware/test_request.py | 39 +++++++++++++++++++ 2 files changed, 40 insertions(+), 1 deletion(-) create mode 100644 logging/tests/unit/handlers/middleware/test_request.py diff --git a/logging/nox.py b/logging/nox.py index 11d484b232e7..de975600fd04 100644 --- a/logging/nox.py +++ b/logging/nox.py @@ -31,7 +31,7 @@ def unit_tests(session, python_version): session.interpreter = 'python{}'.format(python_version) # Install all test dependencies, then install this package in-place. - session.install('mock', 'pytest', 'pytest-cov', 'flask', *LOCAL_DEPS) + session.install('mock', 'pytest', 'pytest-cov', 'flask', 'django', *LOCAL_DEPS) session.install('-e', '.') # Run py.test against the unit tests. diff --git a/logging/tests/unit/handlers/middleware/test_request.py b/logging/tests/unit/handlers/middleware/test_request.py new file mode 100644 index 000000000000..ad784b45c48e --- /dev/null +++ b/logging/tests/unit/handlers/middleware/test_request.py @@ -0,0 +1,39 @@ +# Copyright 2017 Google Inc. All Rights Reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +import unittest + + +class TestRequestMiddleware(unittest.TestCase): + + def _get_target_class(self): + from google.cloud.logging.handlers.middleware.request import RequestMiddleware + + return RequestMiddleware + + def _make_one(self, *args, **kw): + return self._get_target_class()(*args, **kw) + + def test_get_request(self): + from django.conf import settings + from django.test.utils import setup_test_environment + from django.test import RequestFactory + + settings.configure() + setup_test_environment() + + self.middleware = self._make_one() + self.request = RequestFactory().get('/') + self.middleware.process_request(self.request) + self.assertEqual(self.middleware.get_request(), self.request) From 035c4a7be20b1705836b31b946e82c15be04487e Mon Sep 17 00:00:00 2001 From: Angela Li Date: Fri, 26 May 2017 11:25:41 -0700 Subject: [PATCH 41/47] Add unit test for django --- .../unit/handlers/middleware/test_request.py | 15 ++++- logging/tests/unit/handlers/test__helpers.py | 67 ++++++++++++++++++- 2 files changed, 77 insertions(+), 5 deletions(-) diff --git a/logging/tests/unit/handlers/middleware/test_request.py b/logging/tests/unit/handlers/middleware/test_request.py index ad784b45c48e..5b6dfca8dc67 100644 --- a/logging/tests/unit/handlers/middleware/test_request.py +++ b/logging/tests/unit/handlers/middleware/test_request.py @@ -25,15 +25,24 @@ def _get_target_class(self): def _make_one(self, *args, **kw): return self._get_target_class()(*args, **kw) - def test_get_request(self): + def setUp(self): from django.conf import settings from django.test.utils import setup_test_environment - from django.test import RequestFactory - settings.configure() + if not settings.configured: + settings.configure() setup_test_environment() + def test_get_request(self): + from django.test import RequestFactory + self.middleware = self._make_one() self.request = RequestFactory().get('/') self.middleware.process_request(self.request) self.assertEqual(self.middleware.get_request(), self.request) + + + def tearDown(self): + from django.test.utils import teardown_test_environment + + teardown_test_environment() diff --git a/logging/tests/unit/handlers/test__helpers.py b/logging/tests/unit/handlers/test__helpers.py index db9d51d27920..3d023dd8f9cf 100644 --- a/logging/tests/unit/handlers/test__helpers.py +++ b/logging/tests/unit/handlers/test__helpers.py @@ -33,25 +33,88 @@ def setUp(self): def test_trace_id_no_context_header(self): from google.cloud.logging.handlers._helpers import get_trace_id_from_flask + from google.cloud.logging.handlers._helpers import get_trace_id from google.cloud.logging.handlers._helpers import _EMPTY_TRACE_ID with self.app.test_request_context( path='/', headers={}): trace_id = get_trace_id_from_flask() + trace_id_returned = get_trace_id() self.assertEqual(trace_id, _EMPTY_TRACE_ID) + self.assertEqual(trace_id_returned, _EMPTY_TRACE_ID) def test_trace_id_valid_context_header(self): from google.cloud.logging.handlers._helpers import get_trace_id_from_flask + from google.cloud.logging.handlers._helpers import get_trace_id FLASK_TRACE_HEADER = 'X_CLOUD_TRACE_CONTEXT' - FLASK_TRACE_ID = 'testtraceid/testspanid' + FLASK_TRACE_ID = 'testtraceidflask/testspanid' with self.app.test_request_context( path='/', headers={FLASK_TRACE_HEADER:FLASK_TRACE_ID}): trace_id = get_trace_id_from_flask() + trace_id_returned = get_trace_id() - EXPECTED_TRACE_ID = 'testtraceid' + EXPECTED_TRACE_ID = 'testtraceidflask' self.assertEqual(trace_id, EXPECTED_TRACE_ID) + self.assertEqual(trace_id, trace_id_returned) + + +class TestDjangoTrace(unittest.TestCase): + + def setUp(self): + from django.conf import settings + from django.test.utils import setup_test_environment + + if not settings.configured: + settings.configure() + setup_test_environment() + + def test_trace_id_no_context_header(self): + import mock + from django.test import RequestFactory + from google.cloud.logging.handlers._helpers import get_trace_id_from_django + from google.cloud.logging.handlers._helpers import get_trace_id + from google.cloud.logging.handlers._helpers import _EMPTY_TRACE_ID + + request = RequestFactory().get('/') + + with mock.patch( + 'google.cloud.logging.handlers.middleware.RequestMiddleware.get_request', + return_value=request): + trace_id = get_trace_id_from_django() + trace_id_returned = get_trace_id() + + self.assertEqual(trace_id, _EMPTY_TRACE_ID) + self.assertEqual(trace_id_returned, _EMPTY_TRACE_ID) + + def test_trace_id_valid_context_header(self): + import mock + from django.test import RequestFactory + from google.cloud.logging.handlers._helpers import get_trace_id_from_django + from google.cloud.logging.handlers._helpers import get_trace_id + + DJANGO_TRACE_HEADER = 'HTTP_X_CLOUD_TRACE_CONTEXT' + DJANGO_TRACE_ID = 'testtraceiddjango/testspanid' + + request = RequestFactory().get( + '/', + **{DJANGO_TRACE_HEADER:DJANGO_TRACE_ID}) + + with mock.patch( + 'google.cloud.logging.handlers.middleware.RequestMiddleware.get_request', + return_value=request): + trace_id = get_trace_id_from_django() + trace_id_returned = get_trace_id() + + EXPECTED_TRACE_ID = 'testtraceiddjango' + self.assertEqual(trace_id, EXPECTED_TRACE_ID) + self.assertEqual(trace_id, trace_id_returned) + + def tearDown(self): + from django.test.utils import teardown_test_environment + + teardown_test_environment() From bd8c1b4e50a616ba5296a6f980e202cbbec38a68 Mon Sep 17 00:00:00 2001 From: Angela Li Date: Fri, 26 May 2017 11:29:54 -0700 Subject: [PATCH 42/47] Fix style --- logging/tests/unit/handlers/middleware/test_request.py | 1 - 1 file changed, 1 deletion(-) diff --git a/logging/tests/unit/handlers/middleware/test_request.py b/logging/tests/unit/handlers/middleware/test_request.py index 5b6dfca8dc67..0aa950263f5d 100644 --- a/logging/tests/unit/handlers/middleware/test_request.py +++ b/logging/tests/unit/handlers/middleware/test_request.py @@ -41,7 +41,6 @@ def test_get_request(self): self.middleware.process_request(self.request) self.assertEqual(self.middleware.get_request(), self.request) - def tearDown(self): from django.test.utils import teardown_test_environment From 51bf1931e5656265d3e9ebc36ce102e9c63dad24 Mon Sep 17 00:00:00 2001 From: Angela Li Date: Fri, 26 May 2017 14:38:45 -0700 Subject: [PATCH 43/47] Address jon's comments --- .../google/cloud/logging/handlers/_helpers.py | 17 ++--- .../cloud/logging/handlers/app_engine.py | 10 ++- .../logging/handlers/middleware/request.py | 17 ++--- .../unit/handlers/middleware/test_request.py | 13 ++-- logging/tests/unit/handlers/test__helpers.py | 66 ++++++++++--------- .../tests/unit/handlers/test_app_engine.py | 6 +- 6 files changed, 66 insertions(+), 63 deletions(-) diff --git a/logging/google/cloud/logging/handlers/_helpers.py b/logging/google/cloud/logging/handlers/_helpers.py index 17caf76e4aa9..2c88f427f00b 100644 --- a/logging/google/cloud/logging/handlers/_helpers.py +++ b/logging/google/cloud/logging/handlers/_helpers.py @@ -22,13 +22,11 @@ except ImportError: flask = None -from google.cloud.logging.handlers.middleware.request import RequestMiddleware +from google.cloud.logging.handlers.middleware.request import _get_request _FLASK_TRACE_HEADER = 'X_CLOUD_TRACE_CONTEXT' _DJANGO_TRACE_HEADER = 'HTTP_X_CLOUD_TRACE_CONTEXT' -_EMPTY_TRACE_ID = 'None' - def format_stackdriver_json(record, message): """Helper to format a LogRecord in in Stackdriver fluentd format. @@ -58,12 +56,12 @@ def get_trace_id_from_flask(): :return: Trace_id in HTTP request headers. """ if not flask or not flask.request: - return _EMPTY_TRACE_ID + return None header = flask.request.headers.get(_FLASK_TRACE_HEADER) if not header: - return _EMPTY_TRACE_ID + return None trace_id = header.split('/')[0] @@ -76,16 +74,15 @@ def get_trace_id_from_django(): :rtype: str :return: Trace_id in HTTP request headers. """ - request_middleware = RequestMiddleware() - request = request_middleware.get_request() + request = _get_request() if not request: - return _EMPTY_TRACE_ID + return None try: header = request.META[_DJANGO_TRACE_HEADER] except KeyError: - return _EMPTY_TRACE_ID + return None trace_id = header.split('/')[0] @@ -102,7 +99,7 @@ def get_trace_id(): for checker in checkers: trace_id = checker() - if trace_id is not _EMPTY_TRACE_ID: + if trace_id is not None: return trace_id return trace_id diff --git a/logging/google/cloud/logging/handlers/app_engine.py b/logging/google/cloud/logging/handlers/app_engine.py index ccab4101848b..4561b83555c4 100644 --- a/logging/google/cloud/logging/handlers/app_engine.py +++ b/logging/google/cloud/logging/handlers/app_engine.py @@ -31,6 +31,8 @@ _GAE_SERVICE_ENV = 'GAE_SERVICE' _GAE_VERSION_ENV = 'GAE_VERSION' +_TRACE_ID_LABEL = 'appengine.googleapis.com/trace_id' + class AppEngineHandler(CloudLoggingHandler): """A logging handler that sends App Engine-formatted logs to Stackdriver. @@ -76,8 +78,10 @@ def get_gae_labels(self): :rtype: dict :returns: Labels for GAE app. """ + gae_labels = {} + trace_id = get_trace_id() - gae_labels = { - 'appengine.googleapis.com/trace_id': trace_id, - } + if trace_id is not None: + gae_labels[_TRACE_ID_LABEL] = trace_id + return gae_labels diff --git a/logging/google/cloud/logging/handlers/middleware/request.py b/logging/google/cloud/logging/handlers/middleware/request.py index 7e0e89f6aa39..9baf11f4dbbe 100644 --- a/logging/google/cloud/logging/handlers/middleware/request.py +++ b/logging/google/cloud/logging/handlers/middleware/request.py @@ -18,6 +18,15 @@ _thread_locals = local() +def _get_request(): + """Get Django request from thread local. + + :rtype: str + :returns: Django request. + """ + return getattr(_thread_locals, 'request', None) + + class RequestMiddleware(object): """Saves the request in thread local""" @@ -28,11 +37,3 @@ def process_request(self, request): :param request: Django http request. """ _thread_locals.request = request - - def get_request(self): - """Get Django request from thread local. - - :rtype: str - :returns: Django request. - """ - return getattr(_thread_locals, 'request', None) diff --git a/logging/tests/unit/handlers/middleware/test_request.py b/logging/tests/unit/handlers/middleware/test_request.py index 0aa950263f5d..349247805104 100644 --- a/logging/tests/unit/handlers/middleware/test_request.py +++ b/logging/tests/unit/handlers/middleware/test_request.py @@ -33,15 +33,16 @@ def setUp(self): settings.configure() setup_test_environment() + def tearDown(self): + from django.test.utils import teardown_test_environment + + teardown_test_environment() + def test_get_request(self): from django.test import RequestFactory + from google.cloud.logging.handlers.middleware.request import _get_request self.middleware = self._make_one() self.request = RequestFactory().get('/') self.middleware.process_request(self.request) - self.assertEqual(self.middleware.get_request(), self.request) - - def tearDown(self): - from django.test.utils import teardown_test_environment - - teardown_test_environment() + self.assertEqual(_get_request(), self.request) diff --git a/logging/tests/unit/handlers/test__helpers.py b/logging/tests/unit/handlers/test__helpers.py index 3d023dd8f9cf..634ceb26e8a6 100644 --- a/logging/tests/unit/handlers/test__helpers.py +++ b/logging/tests/unit/handlers/test__helpers.py @@ -34,7 +34,6 @@ def setUp(self): def test_trace_id_no_context_header(self): from google.cloud.logging.handlers._helpers import get_trace_id_from_flask from google.cloud.logging.handlers._helpers import get_trace_id - from google.cloud.logging.handlers._helpers import _EMPTY_TRACE_ID with self.app.test_request_context( path='/', @@ -42,24 +41,24 @@ def test_trace_id_no_context_header(self): trace_id = get_trace_id_from_flask() trace_id_returned = get_trace_id() - self.assertEqual(trace_id, _EMPTY_TRACE_ID) - self.assertEqual(trace_id_returned, _EMPTY_TRACE_ID) + self.assertEqual(trace_id, None) + self.assertEqual(trace_id_returned, None) def test_trace_id_valid_context_header(self): from google.cloud.logging.handlers._helpers import get_trace_id_from_flask from google.cloud.logging.handlers._helpers import get_trace_id - FLASK_TRACE_HEADER = 'X_CLOUD_TRACE_CONTEXT' - FLASK_TRACE_ID = 'testtraceidflask/testspanid' + flask_trace_header = 'X_CLOUD_TRACE_CONTEXT' + flask_trace_id = 'testtraceidflask/testspanid' with self.app.test_request_context( path='/', - headers={FLASK_TRACE_HEADER:FLASK_TRACE_ID}): + headers={flask_trace_header:flask_trace_id}): trace_id = get_trace_id_from_flask() trace_id_returned = get_trace_id() - EXPECTED_TRACE_ID = 'testtraceidflask' - self.assertEqual(trace_id, EXPECTED_TRACE_ID) + expected_trace_id = 'testtraceidflask' + self.assertEqual(trace_id, expected_trace_id) self.assertEqual(trace_id, trace_id_returned) @@ -73,48 +72,51 @@ def setUp(self): settings.configure() setup_test_environment() + def tearDown(self): + from django.test.utils import teardown_test_environment + + teardown_test_environment() + def test_trace_id_no_context_header(self): - import mock from django.test import RequestFactory from google.cloud.logging.handlers._helpers import get_trace_id_from_django from google.cloud.logging.handlers._helpers import get_trace_id - from google.cloud.logging.handlers._helpers import _EMPTY_TRACE_ID + from google.cloud.logging.handlers.middleware.request import RequestMiddleware + from google.cloud.logging.handlers.middleware.request import _thread_locals request = RequestFactory().get('/') - with mock.patch( - 'google.cloud.logging.handlers.middleware.RequestMiddleware.get_request', - return_value=request): - trace_id = get_trace_id_from_django() - trace_id_returned = get_trace_id() + middleware = RequestMiddleware() + middleware.process_request(request) + trace_id = get_trace_id_from_django() + trace_id_returned = get_trace_id() - self.assertEqual(trace_id, _EMPTY_TRACE_ID) - self.assertEqual(trace_id_returned, _EMPTY_TRACE_ID) + self.assertEqual(trace_id, None) + self.assertEqual(trace_id_returned, None) + + _thread_locals.__dict__.clear() def test_trace_id_valid_context_header(self): - import mock from django.test import RequestFactory from google.cloud.logging.handlers._helpers import get_trace_id_from_django from google.cloud.logging.handlers._helpers import get_trace_id + from google.cloud.logging.handlers.middleware.request import RequestMiddleware + from google.cloud.logging.handlers.middleware.request import _thread_locals - DJANGO_TRACE_HEADER = 'HTTP_X_CLOUD_TRACE_CONTEXT' - DJANGO_TRACE_ID = 'testtraceiddjango/testspanid' + django_trace_header = 'HTTP_X_CLOUD_TRACE_CONTEXT' + django_trace_id = 'testtraceiddjango/testspanid' request = RequestFactory().get( '/', - **{DJANGO_TRACE_HEADER:DJANGO_TRACE_ID}) + **{django_trace_header:django_trace_id}) - with mock.patch( - 'google.cloud.logging.handlers.middleware.RequestMiddleware.get_request', - return_value=request): - trace_id = get_trace_id_from_django() - trace_id_returned = get_trace_id() + middleware = RequestMiddleware() + middleware.process_request(request) + trace_id = get_trace_id_from_django() + trace_id_returned = get_trace_id() - EXPECTED_TRACE_ID = 'testtraceiddjango' - self.assertEqual(trace_id, EXPECTED_TRACE_ID) + expected_trace_id = 'testtraceiddjango' + self.assertEqual(trace_id, expected_trace_id) self.assertEqual(trace_id, trace_id_returned) - def tearDown(self): - from django.test.utils import teardown_test_environment - - teardown_test_environment() + _thread_locals.__dict__.clear() diff --git a/logging/tests/unit/handlers/test_app_engine.py b/logging/tests/unit/handlers/test_app_engine.py index 886dbbbb5584..ccdaf2992335 100644 --- a/logging/tests/unit/handlers/test_app_engine.py +++ b/logging/tests/unit/handlers/test_app_engine.py @@ -32,9 +32,7 @@ def test_constructor(self): from google.cloud.logging.handlers.app_engine import _GAE_PROJECT_ENV from google.cloud.logging.handlers.app_engine import _GAE_SERVICE_ENV from google.cloud.logging.handlers.app_engine import _GAE_VERSION_ENV - from google.cloud.logging.handlers._helpers import _EMPTY_TRACE_ID - - TRACE_ID_LABEL = 'appengine.googleapis.com/trace_id' + from google.cloud.logging.handlers.app_engine import _TRACE_ID_LABEL client = mock.Mock(project=self.PROJECT, spec=['project']) @@ -47,7 +45,7 @@ def test_constructor(self): self.assertEqual(handler.resource.labels['project_id'], 'test_project') self.assertEqual(handler.resource.labels['module_id'], 'test_service') self.assertEqual(handler.resource.labels['version_id'], 'test_version') - self.assertEqual(handler.labels[TRACE_ID_LABEL], _EMPTY_TRACE_ID) + self.assertEqual(handler.labels, {}) def test_emit(self): import mock From 6b90d1e3527aef66b94527ca1b285e9dd9e8d16e Mon Sep 17 00:00:00 2001 From: Angela Li Date: Tue, 30 May 2017 12:28:30 -0700 Subject: [PATCH 44/47] Address jon's comments --- logging/google/cloud/logging/handlers/_helpers.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/logging/google/cloud/logging/handlers/_helpers.py b/logging/google/cloud/logging/handlers/_helpers.py index 2c88f427f00b..00135d0fe243 100644 --- a/logging/google/cloud/logging/handlers/_helpers.py +++ b/logging/google/cloud/logging/handlers/_helpers.py @@ -80,10 +80,12 @@ def get_trace_id_from_django(): return None try: - header = request.META[_DJANGO_TRACE_HEADER] + header = request.META.get(_DJANGO_TRACE_HEADER) except KeyError: return None + if not header: + return None trace_id = header.split('/')[0] return trace_id @@ -102,4 +104,4 @@ def get_trace_id(): if trace_id is not None: return trace_id - return trace_id + return None From 0c6c4f6b0dc410259d0f465b3c614d08412d78f1 Mon Sep 17 00:00:00 2001 From: Angela Li Date: Fri, 2 Jun 2017 00:28:10 -0700 Subject: [PATCH 45/47] Address all comments --- .../google/cloud/logging/handlers/_helpers.py | 14 ++--- .../logging/handlers/middleware/request.py | 6 +- logging/nox.py | 4 +- .../unit/handlers/middleware/test_request.py | 18 +++--- logging/tests/unit/handlers/test__helpers.py | 56 +++++++++---------- logging/tests/unit/handlers/test_handlers.py | 4 +- .../transports/test_background_thread.py | 3 +- .../unit/handlers/transports/test_sync.py | 3 +- 8 files changed, 58 insertions(+), 50 deletions(-) diff --git a/logging/google/cloud/logging/handlers/_helpers.py b/logging/google/cloud/logging/handlers/_helpers.py index 00135d0fe243..e5bdabe2e388 100644 --- a/logging/google/cloud/logging/handlers/_helpers.py +++ b/logging/google/cloud/logging/handlers/_helpers.py @@ -22,7 +22,7 @@ except ImportError: flask = None -from google.cloud.logging.handlers.middleware.request import _get_request +from google.cloud.logging.handlers.middleware.request import _get_django_request _FLASK_TRACE_HEADER = 'X_CLOUD_TRACE_CONTEXT' _DJANGO_TRACE_HEADER = 'HTTP_X_CLOUD_TRACE_CONTEXT' @@ -55,12 +55,12 @@ def get_trace_id_from_flask(): :rtype: str :return: Trace_id in HTTP request headers. """ - if not flask or not flask.request: + if flask is None or not flask.request: return None header = flask.request.headers.get(_FLASK_TRACE_HEADER) - if not header: + if header is None: return None trace_id = header.split('/')[0] @@ -74,9 +74,9 @@ def get_trace_id_from_django(): :rtype: str :return: Trace_id in HTTP request headers. """ - request = _get_request() + request = _get_django_request() - if not request: + if request is None: return None try: @@ -84,7 +84,7 @@ def get_trace_id_from_django(): except KeyError: return None - if not header: + if header is None: return None trace_id = header.split('/')[0] @@ -97,7 +97,7 @@ def get_trace_id(): :rtype: str :returns: Trace_id in HTTP request headers. """ - checkers = [get_trace_id_from_django, get_trace_id_from_flask] + checkers = (get_trace_id_from_django, get_trace_id_from_flask) for checker in checkers: trace_id = checker() diff --git a/logging/google/cloud/logging/handlers/middleware/request.py b/logging/google/cloud/logging/handlers/middleware/request.py index 9baf11f4dbbe..1acf7ffd9961 100644 --- a/logging/google/cloud/logging/handlers/middleware/request.py +++ b/logging/google/cloud/logging/handlers/middleware/request.py @@ -12,13 +12,13 @@ # See the License for the specific language governing permissions and # limitations under the License. -from threading import local +import threading -_thread_locals = local() +_thread_locals = threading.local() -def _get_request(): +def _get_django_request(): """Get Django request from thread local. :rtype: str diff --git a/logging/nox.py b/logging/nox.py index de975600fd04..fbbbec1958c1 100644 --- a/logging/nox.py +++ b/logging/nox.py @@ -31,7 +31,9 @@ def unit_tests(session, python_version): session.interpreter = 'python{}'.format(python_version) # Install all test dependencies, then install this package in-place. - session.install('mock', 'pytest', 'pytest-cov', 'flask', 'django', *LOCAL_DEPS) + session.install( + 'mock', 'pytest', 'pytest-cov', + 'flask', 'django', *LOCAL_DEPS) session.install('-e', '.') # Run py.test against the unit tests. diff --git a/logging/tests/unit/handlers/middleware/test_request.py b/logging/tests/unit/handlers/middleware/test_request.py index 349247805104..ab8c18d1aa0e 100644 --- a/logging/tests/unit/handlers/middleware/test_request.py +++ b/logging/tests/unit/handlers/middleware/test_request.py @@ -25,7 +25,8 @@ def _get_target_class(self): def _make_one(self, *args, **kw): return self._get_target_class()(*args, **kw) - def setUp(self): + @classmethod + def setUpClass(cls): from django.conf import settings from django.test.utils import setup_test_environment @@ -33,16 +34,17 @@ def setUp(self): settings.configure() setup_test_environment() - def tearDown(self): + @classmethod + def tearDownClass(cls): from django.test.utils import teardown_test_environment teardown_test_environment() - def test_get_request(self): + def test_get_django_request(self): from django.test import RequestFactory - from google.cloud.logging.handlers.middleware.request import _get_request + from google.cloud.logging.handlers.middleware.request import _get_django_request - self.middleware = self._make_one() - self.request = RequestFactory().get('/') - self.middleware.process_request(self.request) - self.assertEqual(_get_request(), self.request) + middleware = self._make_one() + request = RequestFactory().get('/') + middleware.process_request(request) + self.assertEqual(_get_django_request(), request) diff --git a/logging/tests/unit/handlers/test__helpers.py b/logging/tests/unit/handlers/test__helpers.py index 634ceb26e8a6..59e56f34524b 100644 --- a/logging/tests/unit/handlers/test__helpers.py +++ b/logging/tests/unit/handlers/test__helpers.py @@ -15,9 +15,10 @@ import unittest -class TestFlaskTrace(unittest.TestCase): +class Test_get_trace_id_from_flask(unittest.TestCase): - def create_app(self): + @staticmethod + def create_app(): import flask app = flask.Flask(__name__) @@ -29,40 +30,41 @@ def index(): return app def setUp(self): - self.app = self.create_app() + self.app = Test_get_trace_id_from_flask.create_app() def test_trace_id_no_context_header(self): - from google.cloud.logging.handlers._helpers import get_trace_id_from_flask - from google.cloud.logging.handlers._helpers import get_trace_id + from google.cloud.logging.handlers import _helpers with self.app.test_request_context( path='/', headers={}): - trace_id = get_trace_id_from_flask() - trace_id_returned = get_trace_id() + trace_id = _helpers.get_trace_id_from_flask() + trace_id_returned = _helpers.get_trace_id() - self.assertEqual(trace_id, None) - self.assertEqual(trace_id_returned, None) + self.assertIsNone(trace_id) + self.assertIsNone(trace_id_returned) def test_trace_id_valid_context_header(self): from google.cloud.logging.handlers._helpers import get_trace_id_from_flask from google.cloud.logging.handlers._helpers import get_trace_id flask_trace_header = 'X_CLOUD_TRACE_CONTEXT' - flask_trace_id = 'testtraceidflask/testspanid' + expected_trace_id = 'testtraceidflask' + flask_trace_id = expected_trace_id + '/testspanid' - with self.app.test_request_context( - path='/', - headers={flask_trace_header:flask_trace_id}): + context = self.app.test_request_context( + path='/', + headers={flask_trace_header: flask_trace_id}) + + with context: trace_id = get_trace_id_from_flask() trace_id_returned = get_trace_id() - expected_trace_id = 'testtraceidflask' self.assertEqual(trace_id, expected_trace_id) self.assertEqual(trace_id, trace_id_returned) -class TestDjangoTrace(unittest.TestCase): +class Test_get_trace_id_from_django(unittest.TestCase): def setUp(self): from django.conf import settings @@ -79,22 +81,20 @@ def tearDown(self): def test_trace_id_no_context_header(self): from django.test import RequestFactory - from google.cloud.logging.handlers._helpers import get_trace_id_from_django - from google.cloud.logging.handlers._helpers import get_trace_id - from google.cloud.logging.handlers.middleware.request import RequestMiddleware - from google.cloud.logging.handlers.middleware.request import _thread_locals + from google.cloud.logging.handlers import _helpers + from google.cloud.logging.handlers.middleware import request - request = RequestFactory().get('/') + django_request = RequestFactory().get('/') - middleware = RequestMiddleware() - middleware.process_request(request) - trace_id = get_trace_id_from_django() - trace_id_returned = get_trace_id() + middleware = request.RequestMiddleware() + middleware.process_request(django_request) + trace_id = _helpers.get_trace_id_from_django() + trace_id_returned = _helpers.get_trace_id() - self.assertEqual(trace_id, None) - self.assertEqual(trace_id_returned, None) + self.assertIsNone(trace_id) + self.assertIsNone(trace_id_returned) - _thread_locals.__dict__.clear() + request._thread_locals.__dict__.clear() def test_trace_id_valid_context_header(self): from django.test import RequestFactory @@ -108,7 +108,7 @@ def test_trace_id_valid_context_header(self): request = RequestFactory().get( '/', - **{django_trace_header:django_trace_id}) + **{django_trace_header: django_trace_id}) middleware = RequestMiddleware() middleware.process_request(request) diff --git a/logging/tests/unit/handlers/test_handlers.py b/logging/tests/unit/handlers/test_handlers.py index 5459a0c10b96..96823b2e906d 100644 --- a/logging/tests/unit/handlers/test_handlers.py +++ b/logging/tests/unit/handlers/test_handlers.py @@ -45,7 +45,9 @@ def test_emit(self): None, None) handler.emit(record) - self.assertEqual(handler.transport.send_called_with, (record, message, _GLOBAL_RESOURCE, None)) + self.assertEqual( + handler.transport.send_called_with, + (record, message, _GLOBAL_RESOURCE, None)) class TestSetupLogging(unittest.TestCase): diff --git a/logging/tests/unit/handlers/transports/test_background_thread.py b/logging/tests/unit/handlers/transports/test_background_thread.py index b9449ce56685..f6671273b53d 100644 --- a/logging/tests/unit/handlers/transports/test_background_thread.py +++ b/logging/tests/unit/handlers/transports/test_background_thread.py @@ -63,7 +63,8 @@ def test_send(self): transport.send(record, message, _GLOBAL_RESOURCE, None) - transport.worker.enqueue.assert_called_once_with(record, message, _GLOBAL_RESOURCE, None) + transport.worker.enqueue.assert_called_once_with( + record, message, _GLOBAL_RESOURCE, None) def test_flush(self): client = _Client(self.PROJECT) diff --git a/logging/tests/unit/handlers/transports/test_sync.py b/logging/tests/unit/handlers/transports/test_sync.py index e18920ce2167..01c15240f3b7 100644 --- a/logging/tests/unit/handlers/transports/test_sync.py +++ b/logging/tests/unit/handlers/transports/test_sync.py @@ -63,7 +63,8 @@ class _Logger(object): def __init__(self, name): self.name = name - def log_struct(self, message, severity=None, resource=_GLOBAL_RESOURCE, labels=None): + def log_struct(self, message, severity=None, + resource=_GLOBAL_RESOURCE, labels=None): self.log_struct_called_with = (message, severity, resource, labels) From fe8a3d539fa927d62abf3b2c6b461112df1a2d14 Mon Sep 17 00:00:00 2001 From: Angela Li Date: Fri, 2 Jun 2017 00:34:48 -0700 Subject: [PATCH 46/47] fix style --- logging/tests/unit/handlers/test__helpers.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/logging/tests/unit/handlers/test__helpers.py b/logging/tests/unit/handlers/test__helpers.py index 59e56f34524b..c50ce0044e85 100644 --- a/logging/tests/unit/handlers/test__helpers.py +++ b/logging/tests/unit/handlers/test__helpers.py @@ -104,7 +104,8 @@ def test_trace_id_valid_context_header(self): from google.cloud.logging.handlers.middleware.request import _thread_locals django_trace_header = 'HTTP_X_CLOUD_TRACE_CONTEXT' - django_trace_id = 'testtraceiddjango/testspanid' + expected_trace_id = 'testtraceiddjango' + django_trace_id = expected_trace_id + '/testspanid' request = RequestFactory().get( '/', @@ -115,7 +116,6 @@ def test_trace_id_valid_context_header(self): trace_id = get_trace_id_from_django() trace_id_returned = get_trace_id() - expected_trace_id = 'testtraceiddjango' self.assertEqual(trace_id, expected_trace_id) self.assertEqual(trace_id, trace_id_returned) From 4dff9043012bf7f5af0071df6ff4ee5facdacb84 Mon Sep 17 00:00:00 2001 From: Danny Hermes Date: Fri, 2 Jun 2017 11:43:59 -0700 Subject: [PATCH 47/47] Tweaks for style / coverage / lint. --- .../google/cloud/logging/handlers/_helpers.py | 12 +- .../cloud/logging/handlers/app_engine.py | 5 +- .../logging/handlers/middleware/request.py | 6 + .../unit/handlers/middleware/test_request.py | 62 ++++++++-- logging/tests/unit/handlers/test__helpers.py | 115 +++++++++++++----- .../tests/unit/handlers/test_app_engine.py | 36 +++++- 6 files changed, 179 insertions(+), 57 deletions(-) diff --git a/logging/google/cloud/logging/handlers/_helpers.py b/logging/google/cloud/logging/handlers/_helpers.py index e5bdabe2e388..8eb2434a27a0 100644 --- a/logging/google/cloud/logging/handlers/_helpers.py +++ b/logging/google/cloud/logging/handlers/_helpers.py @@ -19,10 +19,11 @@ try: import flask -except ImportError: +except ImportError: # pragma: NO COVER flask = None -from google.cloud.logging.handlers.middleware.request import _get_django_request +from google.cloud.logging.handlers.middleware.request import ( + _get_django_request) _FLASK_TRACE_HEADER = 'X_CLOUD_TRACE_CONTEXT' _DJANGO_TRACE_HEADER = 'HTTP_X_CLOUD_TRACE_CONTEXT' @@ -79,13 +80,10 @@ def get_trace_id_from_django(): if request is None: return None - try: - header = request.META.get(_DJANGO_TRACE_HEADER) - except KeyError: - return None - + header = request.META.get(_DJANGO_TRACE_HEADER) if header is None: return None + trace_id = header.split('/')[0] return trace_id diff --git a/logging/google/cloud/logging/handlers/app_engine.py b/logging/google/cloud/logging/handlers/app_engine.py index 4561b83555c4..509bf8002fb1 100644 --- a/logging/google/cloud/logging/handlers/app_engine.py +++ b/logging/google/cloud/logging/handlers/app_engine.py @@ -73,7 +73,10 @@ def get_gae_resource(self): return gae_resource def get_gae_labels(self): - """Return the labels for GAE app which includes trace_id. + """Return the labels for GAE app. + + If the trace ID can be detected, it will be included as a label. + Currently, no other labels are included. :rtype: dict :returns: Labels for GAE app. diff --git a/logging/google/cloud/logging/handlers/middleware/request.py b/logging/google/cloud/logging/handlers/middleware/request.py index 1acf7ffd9961..4c0b22a8e96b 100644 --- a/logging/google/cloud/logging/handlers/middleware/request.py +++ b/logging/google/cloud/logging/handlers/middleware/request.py @@ -12,6 +12,12 @@ # See the License for the specific language governing permissions and # limitations under the License. +"""Django middleware helper to capture a request. + +The request is stored on a thread-local so that it can be +inspected by other helpers. +""" + import threading diff --git a/logging/tests/unit/handlers/middleware/test_request.py b/logging/tests/unit/handlers/middleware/test_request.py index ab8c18d1aa0e..983d67129647 100644 --- a/logging/tests/unit/handlers/middleware/test_request.py +++ b/logging/tests/unit/handlers/middleware/test_request.py @@ -14,16 +14,10 @@ import unittest +import mock -class TestRequestMiddleware(unittest.TestCase): - def _get_target_class(self): - from google.cloud.logging.handlers.middleware.request import RequestMiddleware - - return RequestMiddleware - - def _make_one(self, *args, **kw): - return self._get_target_class()(*args, **kw) +class DjangoBase(unittest.TestCase): @classmethod def setUpClass(cls): @@ -40,11 +34,53 @@ def tearDownClass(cls): teardown_test_environment() - def test_get_django_request(self): + +class TestRequestMiddleware(DjangoBase): + + def _get_target_class(self): + from google.cloud.logging.handlers.middleware import request + + return request.RequestMiddleware + + def _make_one(self, *args, **kw): + return self._get_target_class()(*args, **kw) + + def test_process_request(self): from django.test import RequestFactory - from google.cloud.logging.handlers.middleware.request import _get_django_request + from google.cloud.logging.handlers.middleware import request middleware = self._make_one() - request = RequestFactory().get('/') - middleware.process_request(request) - self.assertEqual(_get_django_request(), request) + mock_request = RequestFactory().get('/') + middleware.process_request(mock_request) + + django_request = request._get_django_request() + self.assertEqual(django_request, mock_request) + + +class Test__get_django_request(DjangoBase): + + @staticmethod + def _call_fut(): + from google.cloud.logging.handlers.middleware import request + + return request._get_django_request() + + @staticmethod + def _make_patch(new_locals): + return mock.patch( + 'google.cloud.logging.handlers.middleware.request._thread_locals', + new=new_locals) + + def test_with_request(self): + thread_locals = mock.Mock(spec=['request']) + with self._make_patch(thread_locals): + django_request = self._call_fut() + + self.assertIs(django_request, thread_locals.request) + + def test_without_request(self): + thread_locals = mock.Mock(spec=[]) + with self._make_patch(thread_locals): + django_request = self._call_fut() + + self.assertIsNone(django_request) diff --git a/logging/tests/unit/handlers/test__helpers.py b/logging/tests/unit/handlers/test__helpers.py index c50ce0044e85..a69e765afa98 100644 --- a/logging/tests/unit/handlers/test__helpers.py +++ b/logging/tests/unit/handlers/test__helpers.py @@ -14,9 +14,17 @@ import unittest +import mock + class Test_get_trace_id_from_flask(unittest.TestCase): + @staticmethod + def _call_fut(): + from google.cloud.logging.handlers import _helpers + + return _helpers.get_trace_id_from_flask() + @staticmethod def create_app(): import flask @@ -30,24 +38,19 @@ def index(): return app def setUp(self): - self.app = Test_get_trace_id_from_flask.create_app() + self.app = self.create_app() - def test_trace_id_no_context_header(self): + def test_no_context_header(self): from google.cloud.logging.handlers import _helpers with self.app.test_request_context( path='/', headers={}): - trace_id = _helpers.get_trace_id_from_flask() - trace_id_returned = _helpers.get_trace_id() + trace_id = self._call_fut() self.assertIsNone(trace_id) - self.assertIsNone(trace_id_returned) - - def test_trace_id_valid_context_header(self): - from google.cloud.logging.handlers._helpers import get_trace_id_from_flask - from google.cloud.logging.handlers._helpers import get_trace_id + def test_valid_context_header(self): flask_trace_header = 'X_CLOUD_TRACE_CONTEXT' expected_trace_id = 'testtraceidflask' flask_trace_id = expected_trace_id + '/testspanid' @@ -57,15 +60,19 @@ def test_trace_id_valid_context_header(self): headers={flask_trace_header: flask_trace_id}) with context: - trace_id = get_trace_id_from_flask() - trace_id_returned = get_trace_id() + trace_id = self._call_fut() self.assertEqual(trace_id, expected_trace_id) - self.assertEqual(trace_id, trace_id_returned) class Test_get_trace_id_from_django(unittest.TestCase): + @staticmethod + def _call_fut(): + from google.cloud.logging.handlers import _helpers + + return _helpers.get_trace_id_from_django() + def setUp(self): from django.conf import settings from django.test.utils import setup_test_environment @@ -76,47 +83,91 @@ def setUp(self): def tearDown(self): from django.test.utils import teardown_test_environment + from google.cloud.logging.handlers.middleware import request teardown_test_environment() + request._thread_locals.__dict__.clear() - def test_trace_id_no_context_header(self): + def test_no_context_header(self): from django.test import RequestFactory - from google.cloud.logging.handlers import _helpers from google.cloud.logging.handlers.middleware import request django_request = RequestFactory().get('/') middleware = request.RequestMiddleware() middleware.process_request(django_request) - trace_id = _helpers.get_trace_id_from_django() - trace_id_returned = _helpers.get_trace_id() - + trace_id = self._call_fut() self.assertIsNone(trace_id) - self.assertIsNone(trace_id_returned) - request._thread_locals.__dict__.clear() - - def test_trace_id_valid_context_header(self): + def test_valid_context_header(self): from django.test import RequestFactory - from google.cloud.logging.handlers._helpers import get_trace_id_from_django - from google.cloud.logging.handlers._helpers import get_trace_id - from google.cloud.logging.handlers.middleware.request import RequestMiddleware - from google.cloud.logging.handlers.middleware.request import _thread_locals + from google.cloud.logging.handlers.middleware import request django_trace_header = 'HTTP_X_CLOUD_TRACE_CONTEXT' expected_trace_id = 'testtraceiddjango' django_trace_id = expected_trace_id + '/testspanid' - request = RequestFactory().get( + django_request = RequestFactory().get( '/', **{django_trace_header: django_trace_id}) - middleware = RequestMiddleware() - middleware.process_request(request) - trace_id = get_trace_id_from_django() - trace_id_returned = get_trace_id() + middleware = request.RequestMiddleware() + middleware.process_request(django_request) + trace_id = self._call_fut() self.assertEqual(trace_id, expected_trace_id) - self.assertEqual(trace_id, trace_id_returned) - _thread_locals.__dict__.clear() + +class Test_get_trace_id(unittest.TestCase): + + @staticmethod + def _call_fut(): + from google.cloud.logging.handlers import _helpers + + return _helpers.get_trace_id() + + def _helper(self, django_return, flask_return): + django_patch = mock.patch( + 'google.cloud.logging.handlers._helpers.get_trace_id_from_django', + return_value=django_return) + flask_patch = mock.patch( + 'google.cloud.logging.handlers._helpers.get_trace_id_from_flask', + return_value=flask_return) + + with django_patch as django_mock: + with flask_patch as flask_mock: + trace_id = self._call_fut() + + return django_mock, flask_mock, trace_id + + def test_from_django(self): + django_mock, flask_mock, trace_id = self._helper( + 'test-django-trace-id', None) + self.assertEqual(trace_id, django_mock.return_value) + + django_mock.assert_called_once_with() + flask_mock.assert_not_called() + + def test_from_flask(self): + django_mock, flask_mock, trace_id = self._helper( + None, 'test-flask-trace-id') + self.assertEqual(trace_id, flask_mock.return_value) + + django_mock.assert_called_once_with() + flask_mock.assert_called_once_with() + + def test_from_django_and_flask(self): + django_mock, flask_mock, trace_id = self._helper( + 'test-django-trace-id', 'test-flask-trace-id') + # Django wins. + self.assertEqual(trace_id, django_mock.return_value) + + django_mock.assert_called_once_with() + flask_mock.assert_not_called() + + def test_missing(self): + django_mock, flask_mock, trace_id = self._helper(None, None) + self.assertIsNone(trace_id) + + django_mock.assert_called_once_with() + flask_mock.assert_called_once_with() diff --git a/logging/tests/unit/handlers/test_app_engine.py b/logging/tests/unit/handlers/test_app_engine.py index ccdaf2992335..6438c4abb8a0 100644 --- a/logging/tests/unit/handlers/test_app_engine.py +++ b/logging/tests/unit/handlers/test_app_engine.py @@ -15,8 +15,10 @@ import logging import unittest +import mock -class TestAppEngineHandlerHandler(unittest.TestCase): + +class TestAppEngineHandler(unittest.TestCase): PROJECT = 'PROJECT' def _get_target_class(self): @@ -28,7 +30,6 @@ def _make_one(self, *args, **kw): return self._get_target_class()(*args, **kw) def test_constructor(self): - import mock from google.cloud.logging.handlers.app_engine import _GAE_PROJECT_ENV from google.cloud.logging.handlers.app_engine import _GAE_SERVICE_ENV from google.cloud.logging.handlers.app_engine import _GAE_VERSION_ENV @@ -48,8 +49,6 @@ def test_constructor(self): self.assertEqual(handler.labels, {}) def test_emit(self): - import mock - client = mock.Mock(project=self.PROJECT, spec=['project']) handler = self._make_one(client, transport=_Transport) gae_resource = handler.get_gae_resource() @@ -66,6 +65,35 @@ def test_emit(self): handler.transport.send_called_with, (record, message, gae_resource, gae_labels)) + def _get_gae_labels_helper(self, trace_id): + get_trace_patch = mock.patch( + 'google.cloud.logging.handlers.app_engine.get_trace_id', + return_value=trace_id) + + client = mock.Mock(project=self.PROJECT, spec=['project']) + # The handler actually calls ``get_gae_labels()``. + with get_trace_patch as mock_get_trace: + handler = self._make_one(client, transport=_Transport) + mock_get_trace.assert_called_once_with() + + gae_labels = handler.get_gae_labels() + self.assertEqual(mock_get_trace.mock_calls, + [mock.call(), mock.call()]) + + return gae_labels + + def test_get_gae_labels_with_label(self): + from google.cloud.logging.handlers import app_engine + + trace_id = 'test-gae-trace-id' + gae_labels = self._get_gae_labels_helper(trace_id) + expected_labels = {app_engine._TRACE_ID_LABEL: trace_id} + self.assertEqual(gae_labels, expected_labels) + + def test_get_gae_labels_without_label(self): + gae_labels = self._get_gae_labels_helper(None) + self.assertEqual(gae_labels, {}) + class _Transport(object):