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 --log-http option #326

Merged
merged 10 commits into from Aug 6, 2019
Merged

Add --log-http option #326

merged 10 commits into from Aug 6, 2019

Conversation

sixolet
Copy link
Contributor

@sixolet sixolet commented Aug 1, 2019

It prints all the HTTP requests and responses to stderr.

Quick and dirty, I just found it useful when debugging what requests kn was sending, maybe other people will too.

Prints the http requests before they get their tokens added, so it's not that much of a security risk.

@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Aug 1, 2019
@knative-prow-robot knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 1, 2019
@knative-prow-robot knative-prow-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Aug 1, 2019
@knative-prow-robot knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 1, 2019
@cppforlife
Copy link

@sixolet is that similar to how kubectl implements -v=8 option?

@sixolet
Copy link
Contributor Author

sixolet commented Aug 1, 2019

Looks like yes-ish, but they use the github.com/golang/glog module to read the global log level in the factory for deciding whether to generate the verbose transport or not, and can generate the transport at creation time, which didn't make its way into client-go. However, reading more code made me realize a slightly better way to do it that nixes the downcast, I think.

@sixolet
Copy link
Contributor Author

sixolet commented Aug 2, 2019 via email

@sixolet
Copy link
Contributor Author

sixolet commented Aug 2, 2019

... actually never mind, I was looking at the response. I'll fix.

Copy link
Contributor

@rhuss rhuss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot, I think this is a very useful feature.

My only concern is about the UX integration via --log-http. We have already PRs open which introduce --verbose and/or --debug. We should consolidate on that to offer a minimal UI surface for managing log and debug output (especially as it's a global option)

My suggestion would be that we could settle on a convention like:

  • Adding --debug for very low level information. Only useful for developer of kn itself
  • Adding a --verbose option for increasing the detail level of the information presented. Examples are
    • kn service describe --verbose to add more details to a service (like the image digests, or all env vars/annotations/labels)
    • kn plugin list --verbose to also print the plugin directories where plugins are looked up
    • kn service create --debug to print also the HTTP headers

In order to tune debug and verbose output, I'd suggest to introduce categories:

  • --debug --> all debug info
  • --debug=all --> all debug info
  • --debug=http --> only http related debugging
  • --verbose --> all verbose output
  • --verbose=config --> config related information, too
  • --verbose=config,details --> config information and details (made up here to show how to combine categories)

This suggestion would introduce exactly two option, --debug and --verbose (with the possible values shown in the help page). The alternative would be to have an extra option for every category like in this PR, but this would blow up the help pages and UX surface considerably over time.

Happy to use --log instead of --verbose it's not so much about naming but about being concise.

pkg/kn/commands/types_test.go Outdated Show resolved Hide resolved
pkg/util/logging_http_transport.go Outdated Show resolved Hide resolved
pkg/util/logging_http_transport.go Outdated Show resolved Hide resolved
pkg/util/logging_http_transport.go Show resolved Hide resolved
@knative-metrics-robot
Copy link

The following is the coverage report on pkg/.
Say /test pull-knative-client-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/kn/commands/types.go 53.6% 60.0% 6.4
pkg/kn/core/root.go 55.2% 55.7% 0.5
pkg/util/logging_http_transport.go Do not exist 92.6%

@sixolet
Copy link
Contributor Author

sixolet commented Aug 6, 2019

Let's merge this into --verbose as part of or shortly after that change?

@rhuss
Copy link
Contributor

rhuss commented Aug 6, 2019

Let's merge this into --verbose as part of or shortly after that change?

Let's do it after this PR is merged. I opened #333 for this.

/approve
/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 6, 2019
@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rhuss, sixolet

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@knative-prow-robot knative-prow-robot merged commit d30beba into knative:master Aug 6, 2019
@rhuss rhuss mentioned this pull request Aug 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cla: yes Indicates the PR's author has signed the CLA. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants