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

Merging logging-stdlib-handler-feature branch back into master #2151

Merged
merged 2 commits into from
Aug 22, 2016

Conversation

dhermes
Copy link
Contributor

@dhermes dhermes commented Aug 19, 2016

Fixes #2102

Will likely fail the CLA check (since it contains someone else's commits) but we can just merge with a failed check.

/cc @waprin

@dhermes dhermes added hygiene api: logging Issues related to the Cloud Logging API. labels Aug 19, 2016
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Aug 19, 2016
@dhermes
Copy link
Contributor Author

dhermes commented Aug 19, 2016

@waprin There are two commits with subject "Add Logging Handler", and this one seems to not make sense:
9af9800

Can you vet?

@waprin
Copy link
Contributor

waprin commented Aug 19, 2016

Doesn't make sense but let me quickly review

@waprin
Copy link
Contributor

waprin commented Aug 19, 2016

Yeah bad commit that should be nuked

@dhermes
Copy link
Contributor Author

dhermes commented Aug 19, 2016

@waprin Where was it from? Stray commit from bad rebase? Also why did CLA bot not yell at me?

@dhermes
Copy link
Contributor Author

dhermes commented Aug 19, 2016

When you say nuke do mean folded into the original or just deleted?

@waprin
Copy link
Contributor

waprin commented Aug 19, 2016

deleted

@dhermes dhermes force-pushed the fix-2102 branch 2 times, most recently from 54f3b94 to 1916cca Compare August 20, 2016 05:10
@dhermes
Copy link
Contributor Author

dhermes commented Aug 20, 2016

@waprin I deleted and in the process decided to fold in the rebase changes needed to avoid the weird extra commit.

I made some other changes, but want them to be clear so it doesn't seem to nanny-ish (it is nanny-ish, but I think the changes are good?): https://gist.github.com/dhermes/02bf516901721cb3d2a3c48cd93958b0

As you can see the changes are the following:

  • Removing trailing empty lines in files
  • Changing double spaces for single spaces
  • Rewording / rewriting some docstrings (mostly to keep the first line below 80 chars, but also just adding more attention to detail a la Add Factory Methods for Metric/Resource/Timeseries #2071)
  • For "ugly indent" reasons constructing some dictionaries outside of function calls.
  • raised NotImplementedError instead of NotImplementedError() (does the same)

We can address outside this PR but I'm also pretty uncomfortable with

  • a module named gcloud.logging.handlers.handlers
  • the ad-hoc copy of a Client in BackgroundThreadTransport.__init__

@waprin
Copy link
Contributor

waprin commented Aug 22, 2016

@dhermes changes look good.

a module named gcloud.logging.handlers.handlers

Why do you consider this a problem? I'm fine to change it, we could roll that code into __init__ or maybe gcloud.logging.handler.core? gcloud.logging.handlers.logging but that might also be confusing.

the ad-hoc copy of a Client in BackgroundThreadTransport.__init__

Just not sure what to do with this, if you want I can help with googleapis/oauth2client#549 if that will expedite anything.

Bill Prin added 2 commits August 22, 2016 11:47
Refactors handlers into separate package

Adds background threaded transport

Adds fix to Batch commit to properly set log
name
@dhermes dhermes merged commit 0bf3b68 into googleapis:master Aug 22, 2016
@dhermes dhermes deleted the fix-2102 branch August 22, 2016 20:10
@dhermes
Copy link
Contributor Author

dhermes commented Aug 22, 2016

@waprin It's the repeated name, just feels wrong / like the base-package isn't named correctly.

As for the copy, why is it there?

@waprin
Copy link
Contributor

waprin commented Aug 22, 2016

The repeated name, I agree just struggling to think of better name. The whole thing is "add Python logging handler functionality", handler.py contains the actual logging handler class, the rest is support like transports etc.

The copy is there because since it uses the client on a background thread to write the logs, there's no guarantee the main thread isn't using the client at the same time. There's no way to lock the client and the system tests were occasionally failing due to this, copying seems the most sane approach.

@waprin
Copy link
Contributor

waprin commented Aug 22, 2016

gcloud.logging.handlers.logging is another option since its for Python logging but still repeats a name. logging_handler is a bit verbose but might be best way to disambiguate.

@dhermes dhermes mentioned this pull request Sep 19, 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants