Add Async Background Thread transport #1942

waprin · 2016-06-29T21:52:59Z

Next step: asynchronous transport using a background thread.

Some notes:

This includes a bugfix where Batch entries were writing logNames incorrectly
Background thread just writes to an entry and keeps re-using it. Seemed ok?
I didn't add a base Transport class yet, though possibly I should? With just a send message that raises NotImpementedException
Background thread will pretty much write an entry as soon as it shows up, although it's possible for a few entries to be written before it's scheduled. I want to run some performance tests on all these handlers and at that point I will start seeing if it would make a difference to wait for more entries to be written before committing them
Docs are getting kind of messy, feel like I might need more nesting. If other people agree I can try to spend more time on it
We should be using Stackdriver Logging everywhere, not CloudLogging. Will fix that all in one go in future PR.

Next handler up will be fluentd.

waprin · 2016-06-29T21:54:15Z

/cc @sharbison3 @filipjs @jonparrott

waprin · 2016-06-29T21:55:40Z

Actually I think I should add the Base transport class, will add that quickly first...

waprin · 2016-06-30T21:02:09Z

Added base Transport class, Travis passes, review/merge at your convenience.

docs/logging-usage.rst

+ entries on a background :class:`python.threading.Thread`.
+
+ 1. :class:`gcloud.logging.handlers.SyncTransport` this handler does a direct API call on each
+ logging statement to writ the entry.


googlebot · 2016-07-06T22:16:53Z

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

waprin · 2016-07-07T00:23:25Z

@tseaver was waiting for review/merge, I just rebased it onto origin/master but now it thinks all those commits are mine. Not sure how to fix?

tseaver · 2016-07-08T18:05:13Z

@waprin is 9e30584 the only "real" commit for this PR? I think you should've rebased it against the logging-stdlib-handler-feature branch, rather than master.

googlebot · 2016-07-08T18:50:48Z

CLAs look good, thanks!

waprin · 2016-07-08T20:50:07Z

@tseaver ok i rebased it, i was trying to merge upstream but I think I did that wrong and I'll just hold off on it for now.

Travis is now failing the lint but looks like origin/master is too so I'm guessing it's not related to this PR.

daspecster · 2016-07-14T14:50:31Z

@waprin, we've been battling with pylint and a few things. I think if you rebase again, it will fix this lint error in travis.

googlebot · 2016-07-14T23:50:43Z

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

daspecster · 2016-07-15T18:42:35Z

@waprin Yay!

waprin · 2016-07-15T18:47:33Z

@daspecster yes thanks for help. System tests are failing so will fix that then ping for review

gcloud/logging/handlers/transports/background_thread.py

+                             http)
+        logger = self.client.logger(name)
+        self.worker = _Worker(logger)
+


waprin · 2016-07-15T22:10:54Z

@daspecster ready for another review at your convenience, just mentioned yo in another comment about the need for the class to copy the client.

gcloud/logging/handlers/transports/background_thread.py

+            while not self.stopping:
+                if len(self.batch.entries) == 0:
+                    # branch coverage of this code extremely flaky
+                    self._entries_condition.wait()  # pragma: NO COVER


waprin · 2016-07-25T23:11:30Z

So where are we leaving off with this?

daspecster · 2016-07-25T23:13:03Z

@tseaver @dhermes WDYT?

dhermes · 2016-07-26T16:50:57Z

I'm too out of the loop to comment

waprin · 2016-07-27T22:51:12Z

@tseaver ping

waprin · 2016-07-28T15:39:57Z

@tseaver @dhermes @daspecster Been almost a month, would like this to get merged and the feature branch to get merged into master, what's going on?

jgeewax · 2016-07-28T15:41:11Z

@daspecster : Can you take a look?

daspecster · 2016-07-28T15:44:17Z

@jgeewax sure, I have already a little bit and @waprin addressed my questions. So, LGTM at the moment!

daspecster · 2016-07-28T15:46:15Z

I'll merge if we're good to go?

Refactors handlers into separate package Adds background threaded transport Adds fix to Batch commit to properly set log name

waprin · 2016-07-28T17:23:35Z

Had to kill shebang (since realized it's a bad thing to have in default template), but after Travis passes we can merge.

This is still just into a feature branch so we can still have Tres take a final pass before merge into master.

waprin · 2016-07-28T18:05:25Z

Traceback (most recent call last):
File "/home/travis/build/GoogleCloudPlatform/gcloud-python/.tox/py26/lib/python2.6/site-packages/nose/loader.py", line 418, in loadTestsFromName
addr.filename, addr.module)
File "/home/travis/build/GoogleCloudPlatform/gcloud-python/.tox/py26/lib/python2.6/site-packages/nose/importer.py", line 47, in importFromPath
return self.importFromDir(dir_path, fqname)
File "/home/travis/build/GoogleCloudPlatform/gcloud-python/.tox/py26/lib/python2.6/site-packages/nose/importer.py", line 94, in importFromDir
mod = load_module(part_fqname, fh, filename, desc)
File "/home/travis/build/GoogleCloudPlatform/gcloud-python/gcloud/logging/init.py", line 17, in
from gcloud.logging.client import Client
File "/home/travis/build/GoogleCloudPlatform/gcloud-python/gcloud/logging/client.py", line 42, in
from gcloud.logging.entries import ProtobufEntry
File "/home/travis/build/GoogleCloudPlatform/gcloud-python/gcloud/logging/entries.py", line 20, in
from google.protobuf.json_format import Parse
File "/home/travis/build/GoogleCloudPlatform/gcloud-python/.tox/py26/lib/python2.6/site-packages/google/protobuf/json_format.py", line 48, in
from ordereddict import OrderedDict #PY26
ImportError: No module named ordereddict

😢

tseaver · 2016-07-28T18:07:28Z

Ugh. That dependency is restored on master. Let me look at fixing the target branch.

tseaver · 2016-07-28T19:20:23Z

I merged master to the logging-stdlib-handler-feature branch, and restarted Travis. Fingers crossed.

tseaver · 2016-07-28T19:29:19Z

... and the bulid is green. Whew!

docs/logging-transports-base.rst

@@ -0,0 +1,7 @@
+Python Logging Handler Sync Transport
+======================================


waprin · 2016-07-28T20:17:07Z

Thanks, will fix those issues in final commit before I aim for a master merge (fluentd handler).

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jun 29, 2016

waprin force-pushed the master branch 4 times, most recently from d0e8da2 to f5b9da5 Compare June 30, 2016 20:40

theacodes reviewed Jun 30, 2016
View reviewed changes

docs/logging-usage.rst

entries on a background :class:`python.threading.Thread`.

1. :class:`gcloud.logging.handlers.SyncTransport` this handler does a direct API call on each

logging statement to writ the entry.

This comment was marked as spam.

Sign in to view

waprin force-pushed the master branch 2 times, most recently from d6b9658 to 9e30584 Compare July 6, 2016 22:15

googlebot added cla: no This human has *not* signed the Contributor License Agreement. and removed cla: yes This human has signed the Contributor License Agreement. labels Jul 6, 2016

waprin force-pushed the master branch from 9e30584 to 7c27e32 Compare July 8, 2016 18:50

googlebot added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels Jul 8, 2016

waprin force-pushed the master branch 2 times, most recently from a6bc526 to 61c88a6 Compare July 8, 2016 19:58

googlebot added cla: no This human has *not* signed the Contributor License Agreement. and removed cla: yes This human has signed the Contributor License Agreement. labels Jul 14, 2016

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jul 15, 2016

waprin force-pushed the master branch 3 times, most recently from 0576f1d to a62781c Compare July 15, 2016 21:46

waprin reviewed Jul 15, 2016
View reviewed changes

waprin mentioned this pull request Jul 22, 2016

Add Error Reporting Client #1946

Merged

daspecster reviewed Jul 22, 2016
View reviewed changes

Add Async Background Thread transport

4456c70

Refactors handlers into separate package Adds background threaded transport Adds fix to Batch commit to properly set log name

waprin force-pushed the master branch from a62781c to 4456c70 Compare July 28, 2016 17:15

tseaver reviewed Jul 28, 2016
View reviewed changes

docs/logging-transports-base.rst

@@ -0,0 +1,7 @@

Python Logging Handler Sync Transport

======================================

This comment was marked as spam.

Sign in to view

tseaver merged commit 930342a into googleapis:logging-stdlib-handler-feature Jul 28, 2016

waprin mentioned this pull request Aug 5, 2016

Asynchronous code in this library #2059

Closed

		@@ -0,0 +1,7 @@
		Python Logging Handler Sync Transport
		======================================

Add Async Background Thread transport #1942

Add Async Background Thread transport #1942

Conversation

waprin commented Jun 29, 2016

waprin commented Jun 29, 2016 • edited Loading

waprin commented Jun 29, 2016

waprin commented Jun 30, 2016

This comment was marked as spam.

googlebot commented Jul 6, 2016

waprin commented Jul 7, 2016

tseaver commented Jul 8, 2016

googlebot commented Jul 8, 2016

waprin commented Jul 8, 2016

daspecster commented Jul 14, 2016 • edited Loading

googlebot commented Jul 14, 2016

daspecster commented Jul 15, 2016

waprin commented Jul 15, 2016

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.

This comment was marked as spam.

waprin commented Jul 15, 2016

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 commented Jul 25, 2016

daspecster commented Jul 25, 2016

dhermes commented Jul 26, 2016

waprin commented Jul 27, 2016

waprin commented Jul 28, 2016

jgeewax commented Jul 28, 2016

daspecster commented Jul 28, 2016

daspecster commented Jul 28, 2016

waprin commented Jul 28, 2016

waprin commented Jul 28, 2016

tseaver commented Jul 28, 2016

tseaver commented Jul 28, 2016

tseaver commented Jul 28, 2016

This comment was marked as spam.

waprin commented Jul 28, 2016

waprin commented Jun 29, 2016 •

edited

Loading

daspecster commented Jul 14, 2016 •

edited

Loading