Add trace_id to log payload to enable log aggregation in GAE Flex #3355
Conversation
googlebot
added
the
cla: yes
There was a problem hiding this comment.
@liyanhui1228 Thanks for sending this patch but I'm not sure it's something we can accept / a feature we want to support.
@waprin WDYT?
PS I started a code review, but no need to address my review comments until we decide if this should exist / where this feature should live.
| else: | ||
| trace_id = None | ||
|
|
||
| return trace_id |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
| flask = None | ||
|
|
||
| # Extract the trace_id | ||
| if flask: |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
| try: | ||
| import flask | ||
| except ImportError: | ||
| flask = None |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
| """Parse flask request header to get trace_id. | ||
|
|
||
| :rtype: str | ||
| :return: trace_id get from flask header |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
| @@ -36,4 +38,44 @@ def format_stackdriver_json(record, message): | |||
| 'severity': record.levelname, | |||
| } | |||
|
|
|||
| # Make a best effort to add the trace id. | |||
| # First try to extract the trace_id from HTTP header, only support flask framework for now. | |||
| trace_id = parse_flask_request_header() | |||
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
|
This whole PR should get closed, we are just supposed to use the generic log handler according to @JustinBeckwith in #2997 |
|
Maybe we can still get the trace ID in there but it shouldn't be via the stackdriver helper that currently exists. |
|
/cc @duggelz for context |
|
@waprin Can we make a decision quickly and either move forward or close this thing? (i.e. just don't want it to come off our plates and sit open in limbo) |
|
I think we should close and @liyanhui1228 and @duggelz should have a quick chat about best way to do this. |
|
OK, closing now. We can always re-open. |
|
This has nothing to do with #2997 |
|
It has everything to do with it because the helper method it's modifying is used to format the messages for /var/log/appengine. |
|
Okay - deep breath - we failed to properly support a new contributor here. We need to do better. It's clear that while we want this feature, we're not ready to implement it. So, let's create a bug/feature request for this feature and mark all blocking bugs. If the owners of those bugs can't resolve them in a timely manner, let's see what we can do to enable @liyanhui1228 or @duggelz to address them so that this feature can be implemented. @dhermes or @lukesneeringer, can you own this? If not, please let me know and I will do this personally. |
|
I am happy to own it if needed ("buck stops here" kind of thing), but I am not sure I am the right person. I have read over this ticket and I do not intuitively grok the issue in question. I think the issue is that if there is an HTTP header that we can easily get at, we want to send it to Stackdriver in some way? Best case for me is if @jonparrott owns this, but I am willing to if needed. :-) |
|
@lukesneeringer I'm on it. |
Extract trace_id from HTTP request header and add it to application log. The pantheon UI will show the app logs grouped by trace_id and show them aggregated in one request.
Below is the screenshot of the aggregated log:
