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

Truncate msg length in trace #1508

Closed
wants to merge 4 commits into from
Closed

Conversation

menghanl
Copy link
Contributor

@menghanl menghanl commented Sep 7, 2017

fixes #695

trace.go Outdated
if l > len(x) {
return x
}
return x[:l]
Copy link

Choose a reason for hiding this comment

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

This truncation is incorrect if the string contains UTF-8 multibyte characters. It can produce invalid UTF-8 sequences.

Copy link
Member

Choose a reason for hiding this comment

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

This is true, but is that a problem?

@irfansharif
Copy link
Contributor

If we have a large slice and then only hold on to a reference pointing to just the first x elements of the slice (slice[:x]), Go still holds onto the large slice (/backing array) in memory. (The same is true for strings). The following snippet for example doesn't allocate/copy per iteration, s is pinned in memory throughout:

func main() {
    s := "abcdefghi"
    a := make([]string, 100)
    for i := 0; ; i++ {
        a[i%100] = s[:3]
    }
}

Presumably what we want here is an explicit copy instead.

@menghanl
Copy link
Contributor Author

menghanl commented Sep 7, 2017

@irfansharif I added a commit to make a copy of the substring. PTAL.

@irfansharif
Copy link
Contributor

yup, that works.

trace.go Outdated
return fmt.Sprintf("sent: %v", p.msg)
func newPayload(sent bool, msg interface{}) stringer {
if sent {
return stringer(truncate(fmt.Sprintf("sent: %v", msg), truncateSize))
Copy link
Member

Choose a reason for hiding this comment

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

For huge amounts of data and msgs that are natively Stringers, it's probably best to call truncate(stringerMsg.String(), truncateSize) inside fmt.Sprintf() to avoid an allocation and copy of the data that will be removed anyway. But this is probably a pretty trivial savings.

@menghanl
Copy link
Contributor Author

menghanl commented Sep 7, 2017

The change in this PR doesn't work because it makes trace.LazyLog not lazy any more.

After this change, with trace on, even if /debug/requests page is never rendered, each request and response will be serialized when LazyLog is called. This adds large unnecessary overhead to each RPC.

Closing this PR because of that.

@menghanl menghanl closed this Sep 7, 2017
@menghanl menghanl deleted the truncate_trace branch October 25, 2017 22:43
@lock lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tracing keeps entire requests in memory, doesn’t truncate
4 participants