Add flag to include file's dir in logs #74

vincepri · 2019-07-02T18:59:36Z

Signed-off-by: Vince Prignano vincepri@vmware.com

What this PR does / why we need it:
This PR adds a new flag to klog called add_dir_header. This flag, when enabled, includes the file's directory in the header.

This flag is meant to fix cases (like in Kubebuilder) where the generated files are called the same across different controllers/folders. This would reduce the need of renaming files, e.g. machine/controller.go -> machine/machine_controller.go.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

Release note:

dims · 2019-07-02T21:39:19Z

/assign @DirectXMan12 @liggitt

liggitt · 2019-07-02T21:45:59Z

klog.go

-		if slash >= 0 {
-			file = file[slash+1:]
+		dir := path.Base(path.Dir(file))
+		file = path.Base(file)


path.Base(path.Dir(...)) is significantly more expensive than the current code. until we have performance tests in place that tell us whether we are regressing (as several recent functional additions did), I don't think we should merge this

That's fine. If we can find it useful we can keep it until we have a better solution in place.

liggitt · 2019-07-02T21:47:52Z

klog.go

+		dir := path.Base(path.Dir(file))
+		file = path.Base(file)
+		if l.addDirHeader {
+			file = path.Join(dir, file)


alternatively, use a second strings.LastIndex call to back up a second segment if the addDirHeader option was true, and skip calling path.Base/path.Dir entirely

@liggitt what happens on windows? :)

what happens today? / is hard-coded

also, this is using path.Base(path.Dir(...)), not filepath...

ah good point.

Using a second LastIndex also sounds good. For windows compatibility question, we should probably open another issue

dims · 2019-07-08T21:24:10Z

/uncc

liggitt · 2019-07-09T15:28:42Z

can you add a benchmark that verifies this path does not regress in allocations or CPU when the add dir header option is false?

vincepri · 2019-07-09T15:56:54Z

BenchmarkHeader `branch=master`

$ go test -benchmem -run=^$ k8s.io/klog -bench "^(BenchmarkHeader)$" -v -count=10
goos: darwin
goarch: amd64
pkg: k8s.io/klog
BenchmarkHeader-32    	 2000000	       804 ns/op	     184 B/op	       2 allocs/op
BenchmarkHeader-32    	 2000000	       802 ns/op	     184 B/op	       2 allocs/op
BenchmarkHeader-32    	 2000000	       801 ns/op	     184 B/op	       2 allocs/op
BenchmarkHeader-32    	 2000000	       802 ns/op	     184 B/op	       2 allocs/op
BenchmarkHeader-32    	 2000000	       803 ns/op	     184 B/op	       2 allocs/op
BenchmarkHeader-32    	 2000000	       803 ns/op	     184 B/op	       2 allocs/op
BenchmarkHeader-32    	 2000000	       808 ns/op	     184 B/op	       2 allocs/op
BenchmarkHeader-32    	 2000000	       815 ns/op	     184 B/op	       2 allocs/op
BenchmarkHeader-32    	 2000000	       808 ns/op	     184 B/op	       2 allocs/op
BenchmarkHeader-32    	 2000000	       810 ns/op	     184 B/op	       2 allocs/op
PASS
ok  	k8s.io/klog	24.282s

BenchmarkHeader `branch=directory-header`

$ go test -benchmem -run=^$ k8s.io/klog -bench "^(BenchmarkHeader)$" -v -count=10
goos: darwin
goarch: amd64
pkg: k8s.io/klog
BenchmarkHeader-32    	 2000000	       808 ns/op	     184 B/op	       2 allocs/op
BenchmarkHeader-32    	 2000000	       807 ns/op	     184 B/op	       2 allocs/op
BenchmarkHeader-32    	 2000000	       807 ns/op	     184 B/op	       2 allocs/op
BenchmarkHeader-32    	 2000000	       810 ns/op	     184 B/op	       2 allocs/op
BenchmarkHeader-32    	 2000000	       807 ns/op	     184 B/op	       2 allocs/op
BenchmarkHeader-32    	 2000000	       807 ns/op	     184 B/op	       2 allocs/op
BenchmarkHeader-32    	 2000000	       809 ns/op	     184 B/op	       2 allocs/op
BenchmarkHeader-32    	 2000000	       805 ns/op	     184 B/op	       2 allocs/op
BenchmarkHeader-32    	 2000000	       806 ns/op	     184 B/op	       2 allocs/op
BenchmarkHeader-32    	 2000000	       807 ns/op	     184 B/op	       2 allocs/op
PASS
ok  	k8s.io/klog	24.345s

BenchmarkHeaderWithDir `branch=directory-header`

$ go test -benchmem -run=^$ k8s.io/klog -bench "^(BenchmarkHeaderWithDir)$" -v -count=10
goos: darwin
goarch: amd64
pkg: k8s.io/klog
BenchmarkHeaderWithDir-32    	 2000000	       815 ns/op	     184 B/op	       2 allocs/op
BenchmarkHeaderWithDir-32    	 2000000	       814 ns/op	     184 B/op	       2 allocs/op
BenchmarkHeaderWithDir-32    	 2000000	       812 ns/op	     184 B/op	       2 allocs/op
BenchmarkHeaderWithDir-32    	 2000000	       813 ns/op	     184 B/op	       2 allocs/op
BenchmarkHeaderWithDir-32    	 2000000	       815 ns/op	     184 B/op	       2 allocs/op
BenchmarkHeaderWithDir-32    	 2000000	       814 ns/op	     184 B/op	       2 allocs/op
BenchmarkHeaderWithDir-32    	 2000000	       815 ns/op	     184 B/op	       2 allocs/op
BenchmarkHeaderWithDir-32    	 2000000	       814 ns/op	     184 B/op	       2 allocs/op
BenchmarkHeaderWithDir-32    	 2000000	       814 ns/op	     184 B/op	       2 allocs/op
BenchmarkHeaderWithDir-32    	 2000000	       813 ns/op	     184 B/op	       2 allocs/op
PASS
ok  	k8s.io/klog	24.556s

Signed-off-by: Vince Prignano <vincepri@vmware.com>

liggitt · 2019-07-09T16:33:21Z

thanks
/lgtm
/approve

dims · 2019-07-09T16:53:05Z

@vincepri do you mind doc-ing the go test -benchmem magic. pretty please? :)

dims · 2019-07-09T16:53:16Z

/approve

k8s-ci-robot · 2019-07-09T16:53:25Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dims, liggitt, vincepri

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:

~~OWNERS~~ [dims]

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

jhahn21 · 2019-10-14T21:13:46Z

klog.go

@@ -432,6 +433,7 @@ func InitFlags(flagset *flag.FlagSet) {
 	flagset.BoolVar(&logging.toStderr, "logtostderr", logging.toStderr, "log to standard error instead of files")
 	flagset.BoolVar(&logging.alsoToStderr, "alsologtostderr", logging.alsoToStderr, "log to standard error as well as files")
 	flagset.Var(&logging.verbosity, "v", "number for the log level verbosity")
+	flagset.BoolVar(&logging.skipHeaders, "add_dir_header", logging.addDirHeader, "If true, adds the file directory to the header")


Shouldn't this be &logging.addDirHeader?

yes, it should. cc @dims

@jhahn21 Could you please file a PR to fix it. good catch. thanks!

@dims Sure, I will.

@dims Here is the PR: #101

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 2, 2019

k8s-ci-robot requested review from jayunit100 and pohly July 2, 2019 18:59

k8s-ci-robot assigned DirectXMan12 and liggitt Jul 2, 2019

liggitt reviewed Jul 2, 2019

View reviewed changes

vincepri force-pushed the directory-header branch from 05e67df to d1d548b Compare July 5, 2019 18:01

dims mentioned this pull request Jul 8, 2019

/uncc does not seem to work kubernetes/test-infra#13359

Closed

Add flag to include file's dir in logs

b33ae69

Signed-off-by: Vince Prignano <vincepri@vmware.com>

vincepri force-pushed the directory-header branch from d1d548b to b33ae69 Compare July 9, 2019 15:57

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 9, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 9, 2019

k8s-ci-robot merged commit aeddeec into kubernetes:master Jul 9, 2019

jhahn21 reviewed Oct 14, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add flag to include file's dir in logs #74

Add flag to include file's dir in logs #74

vincepri commented Jul 2, 2019

dims commented Jul 2, 2019

liggitt Jul 2, 2019

vincepri Jul 2, 2019

liggitt Jul 2, 2019

dims Jul 2, 2019

liggitt Jul 2, 2019

liggitt Jul 2, 2019

dims Jul 2, 2019

vincepri Jul 2, 2019

dims commented Jul 8, 2019

liggitt commented Jul 9, 2019

vincepri commented Jul 9, 2019

liggitt commented Jul 9, 2019

dims commented Jul 9, 2019

dims commented Jul 9, 2019

k8s-ci-robot commented Jul 9, 2019

jhahn21 Oct 14, 2019

liggitt Oct 14, 2019

dims Oct 14, 2019

jhahn21 Oct 15, 2019

jhahn21 Oct 15, 2019

Add flag to include file's dir in logs #74

Add flag to include file's dir in logs #74

Conversation

vincepri commented Jul 2, 2019

dims commented Jul 2, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dims commented Jul 8, 2019

liggitt commented Jul 9, 2019

vincepri commented Jul 9, 2019

BenchmarkHeader branch=master

BenchmarkHeader branch=directory-header

BenchmarkHeaderWithDir branch=directory-header

liggitt commented Jul 9, 2019

dims commented Jul 9, 2019

dims commented Jul 9, 2019

k8s-ci-robot commented Jul 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenchmarkHeader `branch=master`

BenchmarkHeader `branch=directory-header`

BenchmarkHeaderWithDir `branch=directory-header`