Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Send trace context with logs from web applications #3448

Merged
merged 49 commits into from
Jun 2, 2017
Merged

Conversation

liyanhui1228
Copy link
Contributor

For #3359. This feature add the trace_id to GAE app logs and enables the logs correlation based on the trace_id.

For flask the trace_id is extracted from request headers, for django I used a middleware to store the request in thread local, and then get the trace_id from it. The user will need to add the middleware to django settings before deploy the app. Will add the unit test later.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label May 22, 2017
@dhermes
Copy link
Contributor

dhermes commented May 23, 2017

@liyanhui1228 Do you plan on updating system tests in this PR? If "no", a branch from your own fork is preferred over a branch in the main repository. No need to change it on this one (it isn't really a big deal) but something to think of for future PRs.

flask = None
else:
_USE_FLASK = True

This comment was marked as spam.

This comment was marked as spam.

@@ -0,0 +1,17 @@
# Copyright 2016 Google Inc. All Rights Reserved.

This comment was marked as spam.

This comment was marked as spam.

@liyanhui1228
Copy link
Contributor Author

@dhermes Yeah system test seems necessary for this PR, will consider add it. And thanks for the tip, next time I'll use my own fork if not updating system tests.



def detect_web_framework():
"""Detect web framework used in this environment.

This comment was marked as spam.

This comment was marked as spam.

import flask
except ImportError:
flask = None

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@waprin
Copy link
Contributor

waprin commented May 24, 2017

In Django you need users to specify the middleware in settings.py. So I think you either need a hack to try to inject it at runtime, or docs on how Django users are expected to install it.

Also Django changes reasonably quickly, so you're going to want to decide which versions you support and decide what to do for versions you don't support (probably fall back to unknown). Check out Django release roadmap. 1.11 is the current default pip package and the LTS version so that's the obvious one, 1.9, 1.10, and 2.0 are currently supported so also nice to have.

Overall direction LGTM, will be OOO later in week until after the holiday so don't block on me to merge.

"""
trace_id = get_trace_id_from_request_header()
if trace_id is None:
trace_id = 'unknown'

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@@ -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,

This comment was marked as spam.

This comment was marked as spam.

# limitations under the License.

try:
from threading import local

This comment was marked as spam.

This comment was marked as spam.

try:
from threading import local
except ImportError:
from django.utils._threading_local import local

This comment was marked as spam.

This comment was marked as spam.

@@ -37,3 +45,53 @@ def format_stackdriver_json(record, message):
}

return json.dumps(payload)


def detect_web_framework():

This comment was marked as spam.

This comment was marked as spam.

@liyanhui1228
Copy link
Contributor Author

@waprin Thanks for your comments! I plan to update the docs to let the user add the middleware to django settings, because I found that it is risky to modify the settings at runtime.

"""
try:
trace_id = flask.request.headers['X_CLOUD_TRACE_CONTEXT'].split('/')[0]
except Exception:

This comment was marked as spam.

This comment was marked as spam.

:return: Trace_id in HTTP request headers.
"""
try:
trace_id = flask.request.headers['X_CLOUD_TRACE_CONTEXT'].split('/')[0]

This comment was marked as spam.

This comment was marked as spam.

_thread_locals = local()


def get_request():

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@@ -43,13 +44,15 @@ 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')

This comment was marked as spam.

This comment was marked as spam.

_FLASK_TRACE_HEADER = 'X_CLOUD_TRACE_CONTEXT'
_DJANGO_TRACE_HEADER = 'HTTP_X_CLOUD_TRACE_CONTEXT'

_EMPTY_TRACE_ID = 'None'

This comment was marked as spam.

This comment was marked as spam.

:returns: Labels for GAE app.
"""
trace_id = get_trace_id()
gae_labels = {

This comment was marked as spam.

This comment was marked as spam.

"""
trace_id = get_trace_id()
gae_labels = {
'appengine.googleapis.com/trace_id': trace_id,

This comment was marked as spam.

This comment was marked as spam.

"""
_thread_locals.request = request

def get_request(self):

This comment was marked as spam.

This comment was marked as spam.

self.middleware.process_request(self.request)
self.assertEqual(self.middleware.get_request(), self.request)

def tearDown(self):

This comment was marked as spam.

This comment was marked as spam.

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'

This comment was marked as spam.

This comment was marked as spam.

@liyanhui1228
Copy link
Contributor Author

@dhermes Great! Thanks for fixing it.

@theacodes theacodes merged commit af1fd7e into master Jun 2, 2017
@theacodes theacodes deleted the yanhuil/trace_id branch June 2, 2017 22:24
@theacodes
Copy link
Contributor

@liyanhui1228 Thank you! :)

j97q5u-sqa

landrito pushed a commit to landrito/google-cloud-python that referenced this pull request Aug 21, 2017
landrito pushed a commit to landrito/google-cloud-python that referenced this pull request Aug 22, 2017
landrito pushed a commit to landrito/google-cloud-python that referenced this pull request Aug 22, 2017
@tcroiset
Copy link

tcroiset commented Feb 9, 2018

Hi @liyanhui1228
Can't find doc for that.
Do you have any example of appropriate django settings?

For now, we configure Django with this handler

handlers = {
    'app_engine': {
        'level': 'DEBUG',
        'formatter': 'verbose',
        'class': 'google.cloud.logging.handlers.app_engine.AppEngineHandler',
        'filters': ['request'],
        'client': logging.Client()
    }
}

Is it the suggested way?

@liyanhui1228
Copy link
Contributor Author

@tcroiset Adding 'google.cloud.logging.handlers.middleware.request.RequestMiddleware', to the MIDDLEWARE_CLASSES section in your settings.py will enable the logging correlation feature for django application.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: no This human has *not* signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants