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 `-ltrace` and requests debug logging. #6070

Merged
merged 7 commits into from Jul 7, 2018

Conversation

Projects
None yet
5 participants
@kwlzn
Copy link
Member

kwlzn commented Jul 5, 2018

this PR adds a new -ltrace log level and enables requests HTTP session logging if enabled.

[omerta pants2 (kwlzn/requests_logging)]$ rm -rf ~/.cache/pants/bin/cloc; ./pants -ltrace cloc src/python/pants/scm 2>&1 |grep -A14 ^"send: "
send: 'GET /bin/cloc/1.66/cloc HTTP/1.1\r\nHost: binaries.pantsbuild.org\r\nConnection: keep-alive\r\nAccept-Encoding: gzip, deflate\r\nAccept: */*\r\nUser-Agent: python-requests/2.18.4\r\n\r\n'
reply: 'HTTP/1.1 200 OK\r\n'
header: Date: Fri, 06 Jul 2018 19:23:23 GMT
header: Content-Type: binary/octet-stream
header: Content-Length: 425558
header: Connection: keep-alive
...
header: Last-Modified: Tue, 12 Jun 2018 01:11:59 GMT
header: ETag: "201e32c3aff9b485a5baa81155349029"
header: Accept-Ranges: bytes
header: Expect-CT: max-age=604800, report-uri="https://report-uri.cloudflare.com/cdn-cgi/beacon/expect-ct"
header: Server: cloudflare
...

@kwlzn kwlzn requested review from stuhood , jsirois and alanbato Jul 6, 2018

@kwlzn kwlzn force-pushed the twitter:kwlzn/requests_logging branch from 9283e44 to 705bce4 Jul 6, 2018

@kwlzn kwlzn changed the title Add log verbosity levels and requests debug logging. Add extended log verbosity levels and requests debug logging. Jul 6, 2018

@kwlzn kwlzn force-pushed the twitter:kwlzn/requests_logging branch from 705bce4 to e636e14 Jul 6, 2018

@alanbato
Copy link
Member

alanbato left a comment

This looks good! It'll make debugging things a lot easier :)
Just left a couple of nitpick comments, as I'm not aware of the formatting changes policy of this project.

GCCCCompiler, GCCCppCompiler,
Linker, LLVMCCompiler, LLVMCppCompiler,
Platform)
GCCCCompiler, GCCCppCompiler, Linker,

This comment has been minimized.

@alanbato

alanbato Jul 6, 2018

Member

nit: Is it okay to submit formatting changes along with features in a single PR?

This comment has been minimized.

@kwlzn

kwlzn Jul 6, 2018

Member

yeah, this is just a side effect of running ./build-support/bin/isort.sh -f against a dirty master.

@@ -9,6 +9,7 @@
import os
from functools import reduce


This comment has been minimized.

@alanbato

alanbato Jul 6, 2018

Member

nit: see previous nit

@@ -128,6 +128,9 @@ def register_bootstrap_options(cls, register):
logging.addLevelName(logging.WARNING, 'WARN')
register('-l', '--level', choices=['debug', 'info', 'warn'], default='info', recursive=True,

This comment has been minimized.

@stuhood

stuhood Jul 6, 2018

Member

Having both logging levels and a verbosity flag seems redundant. How would you feel about adding another level of logging (trace or something is conventional) that enables the logging you wanted here?

This comment has been minimized.

@baroquebobcat

baroquebobcat Jul 6, 2018

Contributor

I would prefer having more logging levels too.

This comment has been minimized.

@kwlzn

kwlzn Jul 6, 2018

Member

sounds reasonable to me, tho it will require some logging customization and won't inherently map to pex' existing use of more granular verbose levels - but neither are probably a big deal.

This comment has been minimized.

@kwlzn

kwlzn Jul 6, 2018

Member

k, should be good for another round of review.

@baroquebobcat
Copy link
Contributor

baroquebobcat left a comment

I think that having a flag to setup verbose requests logging is a great idea.

I'm not sure if it makes sense to have a separate mechanism from the existing --log/-ldebug et al path.

@@ -4,6 +4,7 @@
python_library(
dependencies=[
':plugins',
'3rdparty/python:six',

This comment has been minimized.

@baroquebobcat

baroquebobcat Jul 6, 2018

Contributor

nit: sort

This comment has been minimized.

@kwlzn

kwlzn Jul 6, 2018

Member

fixed

@@ -128,6 +128,9 @@ def register_bootstrap_options(cls, register):
logging.addLevelName(logging.WARNING, 'WARN')
register('-l', '--level', choices=['debug', 'info', 'warn'], default='info', recursive=True,
help='Set the logging level.')
# TODO: make `-v -v -v` -> options.verbosity == 3.
register('-v', '--verbosity', type=int, default=0, advanced=True,
help='Set the verbosity level for extended debug logging.')

This comment has been minimized.

@baroquebobcat

baroquebobcat Jul 6, 2018

Contributor

This might have interesting interactions with the existing -v, -V, --version handling from the arg splitter. My guess is that -v will print the version and -vv+ will change the verbosity due to how it detects the version flag.

_HELP_VERSION_ARGS = ('-v', '-V', '--version')

@@ -128,6 +128,9 @@ def register_bootstrap_options(cls, register):
logging.addLevelName(logging.WARNING, 'WARN')
register('-l', '--level', choices=['debug', 'info', 'warn'], default='info', recursive=True,

This comment has been minimized.

@baroquebobcat

baroquebobcat Jul 6, 2018

Contributor

I would prefer having more logging levels too.

@kwlzn kwlzn changed the title Add extended log verbosity levels and requests debug logging. Add `-ltrace` and requests debug logging. Jul 6, 2018

@jsirois

jsirois approved these changes Jul 6, 2018

kwlzn added some commits Jul 6, 2018

@stuhood

stuhood approved these changes Jul 6, 2018

Copy link
Member

stuhood left a comment

Thanks! Looks great.

@kwlzn kwlzn merged commit 3dc6cce into pantsbuild:master Jul 7, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment