-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Store bug reports in separate directories #3150
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Admittedly I'm not familiar with Go and its norms, but I find this quite difficult to understand, especially lines like if _, err := os.Stat(fpath); err == nil
which has an awful lot going on for a single line.
@@ -37,8 +37,9 @@ func respond(code int, w http.ResponseWriter) { | |||
w.Write([]byte("{}")) | |||
} | |||
|
|||
func gzipAndSave(data []byte, fpath string) error { | |||
fpath = filepath.Join("bugs", fpath) | |||
func gzipAndSave(data []byte, dirname, fpath string) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unclear about what is the norm with Go, but does dirname not get a type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does. Go allows you to omit types on function declarations if subsequent ones are the same type. E.g:
func foo(a int, b, c string) {
// ...
}
Is the same as:
func foo(a int, b string, c string) {
// ...
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's... terrible, but ok.
respond(500, w) | ||
return | ||
} | ||
for i, log := range p.Logs { | ||
if err := gzipAndSave([]byte(log.Lines), fmt.Sprintf("%s-%d.log.gz", prefix, i)); err != nil { | ||
if err := gzipAndSave([]byte(log.Lines), prefix, fmt.Sprintf("logs-%d.log.gz", i)); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reason for saving each one as a separate file rather than concatenating them all together (perhaps even with a separator)? Do the logs have timestamps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also I would suggest some level of slightly deeper nesting, like at least YYYY-MM-DD/HHMMSS/ otherwise you're going to find that before long, the directory kills your terminal if you type ls
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(and you already know my view on using timestamps as IDs, and not particularly fine-grained timestamps at that)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reason for saving each one as a separate file rather than concatenating them all together (perhaps even with a separator)? Do the logs have timestamps?
The logs have timestamps. We cannot concatenate all the files together as that breaks causality. The number of files corresponds to the number of sessions where a session is effectively a browser tab. vdh wants to be able to see logs for each browser tab side-by-side rather than muxxed together. A session's position in the list of files is governed by the most-recent log timestamp.
Also I would suggest some level of slightly deeper nesting, like at least YYYY-MM-DD/HHMMSS/ otherwise you're going to find that before long, the directory kills your terminal if you type ls.
Good idea 👍
(and you already know my view on using timestamps as IDs, and not particularly fine-grained timestamps at that)
Well given we are now representing timestamps as directories, I don't mind having a monotonically increasing integer as part of the filename, though it might be weird to see 2 bug reports in the same directory?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, I'm assuming you'd put the monotonically increasing integer in the last directory name. Good point about multiple sessions.
Now nested as |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough
And remove the long filenames now that there is a directory above them.