Add support for logging the trace-id in webapp2 apps. by csilvers · Pull Request #3593 · googleapis/google-cloud-python

csilvers · 2017-07-07T00:26:06Z

Code was added in #3448 that set the trace-id when used in the
flask or django web frameworks.

This extends that to also support webapp2.

webapp2 is not cutting edge technology anymore -- it doesn't even work
on python3 -- but a lot of legacy GAE classic apps use webapp2, so I
think it makes sense to support it.

I tested this by running nox in the logging/ directory, which as far
as I can tell is current state of the art for running tests.

This extends that to also support webapp2. webapp2 is not cutting edge technology anymore -- it doesn't even work on python3 -- but a lot of legacy GAE classic apps use webapp2, so I think it makes sense to support it. I tested this by running `nox` in the logging/ directory, which as far as I can tell is current state of the art for running tests.

googlebot · 2017-07-07T00:26:09Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

csilvers · 2017-07-07T00:30:18Z

Our company (Khan Academy) has signed the CLA. I've been added to the group of authorized contributors. Maybe I need to be added under my github.com email and not my company email. I've done that and will try again.

csilvers · 2017-07-07T00:34:03Z

(I made a new comment but it doesn't look like the clabot has run again. I'm not sure what else I need to do.)

duggelz · 2017-07-07T00:53:37Z

@lukesneeringer How do we poke the clabot? (Craig is/was Google employee #1 so I'll vouch for him if need be :)

duggelz · 2017-07-07T01:21:39Z

The clabot doesn't think that Khan Academy has signed the corporate CLA (unless it has some wierd legal name that doesn't include the words Khan or Academy), nor does it think that csilvers (GitHub username) or csilvers@khanacademy.org has signed the individual CLA.

The docs says that corporate CLAs might take a few days to process, was it signed recently?

csilvers · 2017-07-07T01:40:52Z

Ah, that sounds like it must be it. I just signed the corporate CLA earlier today. I'm happy to wait (though ideally it wouldn't need to block review!)

csilvers · 2017-07-07T01:45:06Z

btw, hi doug!

duggelz · 2017-07-07T01:47:42Z

Hi Craig! :)

Sign in to view

+ # If you try to import webapp2 under python3, you'll get a syntax
+ # error (since it hasn't been ported yet). We just pretend it
+ # doesn't exist. This is unlikely to hit in real life but does
+ # in the tests.


liyanhui1228

Thanks for adding the support for webapp2. Did you test it by actually running a webapp2 application on GAE? If the log aggregation works then LGTM with the minor comments.

Sign in to view

 _get_django_request)

 _FLASK_TRACE_HEADER = 'X_CLOUD_TRACE_CONTEXT'
+_WEBAPP2_TRACE_HEADER = 'x-cloud-trace-context'


Sign in to view

+ """Get trace_id from webapp2 request headers.
+
+ :rtype: str
+ :return: Trace_id in HTTP request headers.


dhermes

Thanks for taking the time to send a PR in the style of our repo!

It looks pretty good, I mostly just have "annoying maintainer nits".

Might be good if @duggelz also took a pass?

ASIDE: It's pretty cool getting a PR from @csilvers! Cheers!

Sign in to view

+ try:
+ # get_request() succeeds if we're in the middle of a webapp2
+ # request, or raises an assertion error otherwise:
+ # "Request global variable is not set".


Sign in to view

+ """Get trace_id from webapp2 request headers.
+
+ :rtype: str
+ :return: Trace_id in HTTP request headers.


Sign in to view

 """
- checkers = (get_trace_id_from_django, get_trace_id_from_flask)
+ checkers = (get_trace_id_from_django, get_trace_id_from_flask,
+ get_trace_id_from_webapp2)


Sign in to view

+ if header is None:
+ return None
+
+ trace_id = header.split('/', 1)[0]


Sign in to view

 session.install(
 'mock', 'pytest', 'pytest-cov',
- 'flask', 'django', *LOCAL_DEPS)
+ 'flask', 'webapp2', 'webob', 'django', *LOCAL_DEPS)


Sign in to view

+ return app
+
+ def setUp(self):
+ self.app = self.create_app()


Sign in to view

+ def create_app():
+ import webapp2
+
+ class GetTraceId(webapp2.RequestHandler):


Sign in to view

+ from google.cloud.logging.handlers import _helpers
+
+ trace_id = _helpers.get_trace_id_from_webapp2()
+ self.response.content_type = "text/plain"


Sign in to view

+ response = req.get_response(self.app)
+ trace_id = response.body
+
+ self.assertEquals("None", trace_id)


Sign in to view

 # See the License for the specific language governing permissions and
 # limitations under the License.

+import sys


csilvers

`Thanks everyone for the super-quick feedback! I don't think I'll be able to get to the fixes tonight (though we'll see), but will do them as soon as I can.

Also, @liyanhui1228, to answer your original question: I haven't tried this in real life, just via the tests. Honestly, I'm not even sure how to test it in real life (we're just starting to play around with this library but know we'll need webapp2 support for our apps). I'm glad to try it though if I can figure out how!

Sign in to view

 _get_django_request)

 _FLASK_TRACE_HEADER = 'X_CLOUD_TRACE_CONTEXT'
+_WEBAPP2_TRACE_HEADER = 'x-cloud-trace-context'


Sign in to view

+ """Get trace_id from webapp2 request headers.
+
+ :rtype: str
+ :return: Trace_id in HTTP request headers.


Sign in to view

+ try:
+ # get_request() succeeds if we're in the middle of a webapp2
+ # request, or raises an assertion error otherwise:
+ # "Request global variable is not set".


Sign in to view

+ if header is None:
+ return None
+
+ trace_id = header.split('/', 1)[0]


Sign in to view

 """
- checkers = (get_trace_id_from_django, get_trace_id_from_flask)
+ checkers = (get_trace_id_from_django, get_trace_id_from_flask,
+ get_trace_id_from_webapp2)


Sign in to view

+ class Test_get_trace_id_from_webapp2(unittest.TestCase):
+
+ @staticmethod
+ def create_app():


Sign in to view

+ def create_app():
+ import webapp2
+
+ class GetTraceId(webapp2.RequestHandler):


Sign in to view

+ from google.cloud.logging.handlers import _helpers
+
+ trace_id = _helpers.get_trace_id_from_webapp2()
+ self.response.content_type = "text/plain"


Sign in to view

+ return app
+
+ def setUp(self):
+ self.app = self.create_app()


Sign in to view

+ response = req.get_response(self.app)
+ trace_id = response.body
+
+ self.assertEquals("None", trace_id)


liyanhui1228 · 2017-07-07T04:43:50Z

@csilvers Yeah there is some inconsistent in the repo for return and returns, I confirmed with Jon that we should use returns. Sorry for the confusion.

Also I have just tested your changes in real life and it works well! I know it will take some time to figure out how to test it in real life because I did the same thing when I worked on this feature. Thank you again for sending this PR :)

csilvers · 2017-07-07T20:44:45Z

Thanks @liyanhui1228 for doing the real-life testing! And also for your code: I got compliments on my coding style and that's entirely because I cribbed from you. :-)

Mostly style and documentation changes, and a few code-reorganization changes; nothing contentful.

csilvers · 2017-07-07T21:46:55Z

My latest commit addresses all the issues but these, which I'm leaving as-is unless people feel strongly:

I didn't use a helper function for extracting a trace-id from the trace-id/span-id pair.
I kept the code that installs webapp2 even for python3 tests (which ignore it).

liyanhui1228 · 2017-07-07T22:05:44Z

This LGTM, but wait for @dhermes for merging. Thanks!

lukesneeringer · 2017-07-10T16:12:16Z

Good to merge. Ignore the CLA bot.

duggelz · 2017-07-10T18:12:08Z

CLA bot now lists Khan Academy as a signer.

googlebot added the cla: no This human has *not* signed the Contributor License Agreement. label Jul 7, 2017

duggelz requested review from dhermes and liyanhui1228 July 7, 2017 00:53

dhermes reviewed Jul 7, 2017
View reviewed changes

liyanhui1228 suggested changes Jul 7, 2017
View reviewed changes

dhermes reviewed Jul 7, 2017
View reviewed changes

csilvers commented Jul 7, 2017
View reviewed changes

Updates based on reviewer feedback.
98401f2
Mostly style and documentation changes, and a few code-reorganization changes; nothing contentful.

liyanhui1228 approved these changes Jul 7, 2017
View reviewed changes

lukesneeringer merged commit 358a448 into googleapis:masterJul 10, 2017

tseaver pushed a commit that referenced this pull request Jul 10, 2017
Add support for logging the trace-id in webapp2 apps. (#3593)
76ae0fc

dhermes mentioned this pull request Aug 4, 2017
Updating all affected packages after google-cloud-core update. #3730
Merged

landrito pushed a commit to landrito/google-cloud-python that referenced this pull request Aug 21, 2017
Add support for logging the trace-id in webapp2 apps. (googleapis#3593)
766d9e9

landrito pushed a commit to landrito/google-cloud-python that referenced this pull request Aug 22, 2017
Add support for logging the trace-id in webapp2 apps. (googleapis#3593)
ebe3bd3

landrito pushed a commit to landrito/google-cloud-python that referenced this pull request Aug 22, 2017
Add support for logging the trace-id in webapp2 apps. (googleapis#3593)
53d0dfd

parthea pushed a commit that referenced this pull request Nov 24, 2025
Add support for logging the trace-id in webapp2 apps. (#3593)
e24ae67

Uh oh!

Conversation

csilvers commented Jul 7, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

googlebot commented Jul 7, 2017

Uh oh!

csilvers commented Jul 7, 2017

Uh oh!

csilvers commented Jul 7, 2017

Uh oh!

duggelz commented Jul 7, 2017

Uh oh!

duggelz commented Jul 7, 2017

Uh oh!

csilvers commented Jul 7, 2017

Uh oh!

csilvers commented Jul 7, 2017

Uh oh!

duggelz commented Jul 7, 2017

Uh oh!

This comment was marked as spam.

Uh oh!

liyanhui1228 left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

dhermes left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

csilvers left a comment

Choose a reason for hiding this comment

Uh oh!

csilvers commented Jul 7, 2017•
edited
Loading