-
Notifications
You must be signed in to change notification settings - Fork 71
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
skip aws logging on successful upload #762
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.
Is that slice going to become too large? Should we put a limit on number of messages?
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 guess all of them will have the same timestamp in the logs. Is that okay?
That is probably a good idea. I have no idea how many logs they put out but it seems to be a lot |
func (l *S3Logger) Log(args ...interface{}) { | ||
msg := "aws sdk:" | ||
for range len(args) { | ||
msg += " %v" |
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.
Tin my experience, building log strings this way cause enough men copies that it can have a significant performance impact. strings.Builder should be more efficient
|
||
l.mu.Lock() | ||
if len(l.msgs) >= 10 { | ||
l.msgs = append(l.msgs[1:], msg) |
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.
If my understanding of go slices is correct, this will cause the array backing to slice to grow indefinitely until the logger gets destroyed as the beginning of the array will never be reclaimed. We may want to either copy the leftover part of the slice to the beginning of the array, or use ring buffer implementation?
No description provided.