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

Better trimming of the source file path #17

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 39 additions & 6 deletions encoding.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"log/slog"
"path/filepath"
"runtime"
"strings"
"time"
)

Expand Down Expand Up @@ -77,13 +78,8 @@ func (e encoder) writeTimestamp(buf *buffer, tt time.Time) {

func (e encoder) writeSource(buf *buffer, pc uintptr, cwd string) {
frame, _ := runtime.CallersFrames([]uintptr{pc}).Next()
if cwd != "" {
if ff, err := filepath.Rel(cwd, frame.File); err == nil {
frame.File = ff
}
}
e.withColor(buf, e.opts.Theme.Source(), func() {
buf.AppendString(frame.File)
buf.AppendString(trimmedPath(frame.File, cwd))
buf.AppendByte(':')
buf.AppendInt(int64(frame.Line))
})
Expand Down Expand Up @@ -190,3 +186,40 @@ func (e encoder) writeLevel(buf *buffer, l slog.Level) {
e.writeColoredString(buf, str, style)
buf.AppendByte(' ')
}

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
}
}
Comment on lines +196 to +200

Choose a reason for hiding this comment

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

does the prefix logic match on Windows ?

Could you add a test about this ?

Thanks

Choose a reason for hiding this comment

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

Let's continue here

#17 (comment)

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

https://github.com/actions/runner-images


// 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:]
}
Comment on lines +190 to +225

Choose a reason for hiding this comment

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

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

Comment on lines +190 to +225

Choose a reason for hiding this comment

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

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 ?

Suggested change
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)
}

Copy link
Author

Choose a reason for hiding this comment

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

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...)
image

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:
image

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.
image

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:
image

This is fixed width:
image

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...

Choose a reason for hiding this comment

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

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.

Loading