Better trimming of the source file path #17
Conversation
Only make the source file path relative to the current working directly *if* the source file is a child path of the current working directory. Typically, the source file path will only be a child of the CWD when the code is being run from its own project folder. But if the executable moved somewhere else first, or packaged in a container, or compiled with the -trimpath option, then this relative path logic doesn't work. If the file path isn't related to the CWT, then trim to just one path element above the file name, which is typically the go package name, e.g. "io/reader.go" Address phsym#16
|
Hi ! Thank you for this improvement. |
|
After thinking about it, I feel this can be quite inconvenient, and it may drop the module name as many go modules could be using slog together. Just an idea, but maybe there's something to do with |
|
Certainly another option would be to show the full path if that full path is not a child of the current working directory. In my experimentation, I found that the file path could either be:
If you just print out the full path though, these values get really long and end up taking over a big chunk of the log line. For me, I preferred the look of the truncated line. I'm playing now with truncating the source and the other headers ever further to guarantee (optionally) a fixed size header block, so the actual message always starts on the same column. For the source field, I'm thinking of truncating by dropping characters out of the middle of the path components, sort of like how fish shell shortens path names in the prompt. So like: might shorten to: So like, drop the module host, vowels, repeated characters, the .go extension...still playing with it to see how it looks. |
| if cwd != "" && strings.HasPrefix(path, cwd) { | ||
| if ff, err := filepath.Rel(cwd, path); err == nil { | ||
| return ff | ||
| } | ||
| } |
There was a problem hiding this comment.
does the prefix logic match on Windows ?
Could you add a test about this ?
Thanks
There was a problem hiding this comment.
Let's continue here
Oh, also, I don't have a windows machine to generate sample values for a test. Do you have any samples?
I don't either. When I'm unsure I tend to use GitHub CI and pop a windows image
| func trimmedPath(path string, cwd string) string { | ||
| // if the file path appears to be under the current | ||
| // working directory, then we're probably running | ||
| // in a dev environment, and we can show the | ||
| // path of the source file relative to the | ||
| // working directory | ||
| if cwd != "" && strings.HasPrefix(path, cwd) { | ||
| if ff, err := filepath.Rel(cwd, path); err == nil { | ||
| return ff | ||
| } | ||
| } | ||
|
|
||
| // Otherwise, show the filename and one | ||
| // path above it, which is typically going to | ||
| // be the package name | ||
| // Note that the go compiler always uses forward | ||
| // slashes, even if the compiler was run on Windows. | ||
| // | ||
| // See https://github.com/golang/go/issues/3335 | ||
| // and https://github.com/golang/go/issues/18151 | ||
|
|
||
| // This is equivalent to filepath.Base(path) | ||
| idx := strings.LastIndexByte(path, '/') | ||
| if idx == -1 { | ||
| return path | ||
| } | ||
|
|
||
| // And this walks back one more separater, which is | ||
| // equivalent to filepath.Join(filepath.Base(filepath.Dir(path)), filepath.Base(path)) | ||
| idx = strings.LastIndexByte(path[:idx], '/') | ||
| if idx == -1 { | ||
| return path | ||
| } | ||
|
|
||
| return path[idx+1:] | ||
| } |
There was a problem hiding this comment.
Please cover this with tests, especially on a Window env
I'm stil unsure why you are not using filepath.Base(path), because on Windows it would expect to split by \ but
| func trimmedPath(path string, cwd string) string { | ||
| // if the file path appears to be under the current | ||
| // working directory, then we're probably running | ||
| // in a dev environment, and we can show the | ||
| // path of the source file relative to the | ||
| // working directory | ||
| if cwd != "" && strings.HasPrefix(path, cwd) { | ||
| if ff, err := filepath.Rel(cwd, path); err == nil { | ||
| return ff | ||
| } | ||
| } | ||
|
|
||
| // Otherwise, show the filename and one | ||
| // path above it, which is typically going to | ||
| // be the package name | ||
| // Note that the go compiler always uses forward | ||
| // slashes, even if the compiler was run on Windows. | ||
| // | ||
| // See https://github.com/golang/go/issues/3335 | ||
| // and https://github.com/golang/go/issues/18151 | ||
|
|
||
| // This is equivalent to filepath.Base(path) | ||
| idx := strings.LastIndexByte(path, '/') | ||
| if idx == -1 { | ||
| return path | ||
| } | ||
|
|
||
| // And this walks back one more separater, which is | ||
| // equivalent to filepath.Join(filepath.Base(filepath.Dir(path)), filepath.Base(path)) | ||
| idx = strings.LastIndexByte(path[:idx], '/') | ||
| if idx == -1 { | ||
| return path | ||
| } | ||
|
|
||
| return path[idx+1:] | ||
| } |
There was a problem hiding this comment.
I assume you are doing such complicated things because of what you mention here
// Otherwise, show the filename and one
// path above it, which is typically going to
// be the package name
// Note that the go compiler always uses forward
// slashes, even if the compiler was run on Windows.
//
// See https://github.com/golang/go/issues/3335
// and https://github.com/golang/go/issues/18151
You will see in my other comments, I'm a bit worried.
But what about the following naive approach ?
| func trimmedPath(path string, cwd string) string { | |
| // if the file path appears to be under the current | |
| // working directory, then we're probably running | |
| // in a dev environment, and we can show the | |
| // path of the source file relative to the | |
| // working directory | |
| if cwd != "" && strings.HasPrefix(path, cwd) { | |
| if ff, err := filepath.Rel(cwd, path); err == nil { | |
| return ff | |
| } | |
| } | |
| // Otherwise, show the filename and one | |
| // path above it, which is typically going to | |
| // be the package name | |
| // Note that the go compiler always uses forward | |
| // slashes, even if the compiler was run on Windows. | |
| // | |
| // See https://github.com/golang/go/issues/3335 | |
| // and https://github.com/golang/go/issues/18151 | |
| // This is equivalent to filepath.Base(path) | |
| idx := strings.LastIndexByte(path, '/') | |
| if idx == -1 { | |
| return path | |
| } | |
| // And this walks back one more separater, which is | |
| // equivalent to filepath.Join(filepath.Base(filepath.Dir(path)), filepath.Base(path)) | |
| idx = strings.LastIndexByte(path[:idx], '/') | |
| if idx == -1 { | |
| return path | |
| } | |
| return path[idx+1:] | |
| } | |
| func trimmedPath(path string, cwd string) string { | |
| // Note that the go compiler always uses forward | |
| // slashes, even if the compiler was run on Windows. | |
| // See https://github.com/golang/go/issues/3335 | |
| // and https://github.com/golang/go/issues/18151 | |
| // This sort out the filepath on a Windows env and do nothing on Unix ones | |
| // This way Rel and Base will work | |
| path = strings.ReplaceAll(path, "/", filepath.Separator) | |
| // if the file path appears to be under the current | |
| // working directory, then we're probably running | |
| // in a dev environment, and we can show the | |
| // path of the source file relative to the | |
| // working directory | |
| if cwd != "" && strings.HasPrefix(path, cwd) { | |
| if ff, err := filepath.Rel(cwd, path); err == nil { | |
| return ff | |
| } | |
| } | |
| return filepath.Base(path) | |
| } |
There was a problem hiding this comment.
I was following the pattern used in zap, which in theory should be a little more efficient, since it doesn't need to worry about cleaning the path, splitting and re-joining, etc.
The performance difference may be negligible. I'll benchmark it. But in order to get something like "console-slog/encoder.go", I think I need to do more than just filepath.Base(path). That will just return the filename alone, like encoder.go. To get console-slog/encoder.go, I need to do something like filepath.Join(filepath.Base(filepath.Dir(path)), filepath.Base(path))
I've been playing with this quite a bit the last couple days, and I'm coming around to just preferring the full file path printed as an attribute at the end of the line, rather than in the header. Even truncated, the values are quite long, and vary a lot, which ends up moving the first column of the message around a lot.
Consider these sample screenshots. This first prints the full file path in the header area (unless the source file is relative to cwd, but it isn't for these log lines...)
Note how the message field is unaligned, and pushed way out to the right, so scanning the lines for a particular message is a little tricky.
In this sample, the path is trimmed. Little better to my eyes, less noise, though the message field is still unaligned:
This was an experiment which I abandoned. The idea was to truncate the headers so they all fit in a user configurable header section width. I didn't like the way this looked. The source paths get truncated so much they are almost useless. Plus, their length is so variable, it would be hard for the user to know what is a good value for the header width. There's no color here, because I didn't get around to making this work with color yet. It gives you an idea what it looks like with the message field is aligned though.
So then I switched gears. I just made the source a normal attribute, added to the end of the attributes. It shows the relative path if it relative to the cwd, otherwise the full path. The header section can either be fixed or variable, depending on a setting in HeaderOpts. This is variable width:
This is fixed width:
Also note that I compiled with the -trimpath build flag, so source paths starting with module names and versions, rather than file paths. When trimming, that results in some cases like [email protected]/logger_writer.go:49 (truncated from github.com/ThalesGroup/[email protected]/logger_writer.go:49)
I tend to prefer either the truncated source in a variable header, e.g.:
12:49:23.628 ERR yugoservice/installers.go:118 yugoservice > Failed to install audit log file writer err=file name not provided
12:49:23.628 INF sallyport.git/sallyport.go:284 sallyport > starting quorum expiry watcher sleep duration=30s
Or the fixed header with the source as an attr, e.g.:
12:55:03.454 ERR yugoservice > Failed to install audit log file writer err=file name not provided source=gitlab.protectv.local/ncryptify/yugo.git/[email protected]/yugoservice/installers.go:118
12:55:03.454 INF sallyport > starting quorum expiry watcher sleep duration=30s source=gitlab.protectv.local/ncryptify/sallyport.git/sallyport.go:284
But of course, any time a fixed header is used, it leaves the developer to guess what an optimal header width should be...
There was a problem hiding this comment.
I'm sorry, I didn't see you replied me
I'm impressed by the way you share you ideas and how you show your iterative solution.
I also prefer to see the file location at the beginning.
But I woultsay it's a matter of taste. Some may prefer it at the end.
|
Oh, also, I don't have a windows machine to generate sample values for a test. Do you have any samples? |
Only make the source file path relative to the current working directly if the source file is a child path of the current working directory.
Typically, the source file path will only be a child of the CWD when the code is being run from its own project folder. But if the executable moved somewhere else first, or packaged in a container, or compiled with the -trimpath option, then this relative path logic doesn't work.
If the file path isn't related to the CWT, then trim to just one path element above the file name, which is typically the go package name, e.g. "io/reader.go"
Address #16