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

Add Logging Handler #1881

Closed
wants to merge 1 commit into from
Closed

Add Logging Handler #1881

wants to merge 1 commit into from

Conversation

waprin
Copy link
Contributor

@waprin waprin commented Jun 21, 2016

First stab at addressing #1497.

Starting this with a baby step while we flesh out what we want. So it doesn't worry about fluentd integration, doesn't worry about async transports, doesn't worry about error reporting integration. Just a minimal handler that connects Python logging to Cloud Logging.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jun 21, 2016
@waprin waprin force-pushed the master branch 5 times, most recently from de6d47e to 0bbf805 Compare June 21, 2016 01:45
@daspecster daspecster added the api: logging Issues related to the Cloud Logging API. label Jun 21, 2016

Python Logging Integration:

It's possible to tie the Python `logging` module directly into Google Cloud Logging. To use it, create a :class:`CloudLoggingAPIHandler <gcloud.logging.CloudLoggingAPIHandler` instance from your Logging client.

This comment was marked as spam.

This comment was marked as spam.

@tseaver tseaver added the type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. label Jun 22, 2016
@@ -0,0 +1,7 @@
Logger
======

This comment was marked as spam.

This comment was marked as spam.

@theacodes
Copy link
Contributor

One concern I thought of is thread-safety. Logging is ostensibly thread-safe, but I know for sure our client is not.

"""

def __init__(self, client):
logging.StreamHandler.__init__(self)

This comment was marked as spam.

@waprin
Copy link
Contributor Author

waprin commented Jun 22, 2016

For thread safety, I think it's fine because the Python handler base class acquires/releases locks around the emit call.

@tseaver do you think this needs a system test added?

@tseaver
Copy link
Contributor

tseaver commented Jun 22, 2016

@waprin a system test would be a good idea. Figuring how how to tear down the handler, as plugged into the stdlib logging module, would be the only hard part, I think.

@waprin
Copy link
Contributor Author

waprin commented Jun 22, 2016

Yeah I think that's unfortunately not quite right on the unit tests either. I try to undo it but I see log statements in the tox output I usually don't see. I'll look into fixing it and adding a system test.

@waprin waprin force-pushed the master branch 4 times, most recently from ebba62b to 578cd38 Compare June 24, 2016 22:32
@waprin
Copy link
Contributor Author

waprin commented Jun 24, 2016

@tseaver Ok I added system tests and I think logging handlers cleanup is now right in both unit and system tests.

I also added a note to CONTRIBUTING that you need to change service account to Owner for logging. This is required to edit Sinks, not thrilled about it but I'm told it works as intended.

The system tests for logging are all waiting for 2 seconds, they were usually failing for me so I bumped it to 5. Unfortunately logging API is especially eventually-consistent so any of these system tests are always going to be at least a little flaky.

Given there's a bunch more I want to add to improve this handler, as well as more work in this library for Error Reporting, do you think we can start merging all this work into some sort of development branch? I wrote up an internal doc on the game plan, I can share it with you if you like (tseaver at company?). I think the work I have in mind belongs in this repo, but we can discuss, maybe not or maybe you want it as optional dependency or organized into contrib packages.

/cc @filipjs

/cc @steren (no work on error reporting yet, if you'd prefer I can only cc you on PRs relevant to that)

@tseaver
Copy link
Contributor

tseaver commented Jun 27, 2016

@waprin I just set up a logging-stdlib-handler feature branch for this work. Can you open a new PR against that branch?

This was referenced Jun 27, 2016
@tseaver
Copy link
Contributor

tseaver commented Jun 28, 2016

Superseded by #1920.

@tseaver tseaver closed this Jun 28, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: logging Issues related to the Cloud Logging API. cla: yes This human has signed the Contributor License Agreement. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants