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

Add at: in Top-N alloc site output #15616

Merged

Conversation

travisdowns
Copy link
Member

@travisdowns travisdowns commented Dec 14, 2023

When we OOM we output the top-N alloc sites and a one-line backtrace.

However, sesatar-addr2line fails to recognize it because the one-line regex requires backtrace: or another suitable token in order to understand that it's a one-line backtrace.

So we add at:.

Fixes #15615.

Backports Required

  • none - not a bug fix
  • none - this is a backport
  • none - issue does not exist in previous branches
  • none - papercut/not impactful enough to backport
  • v23.3.x
  • v23.2.x
  • v23.1.x
  • v22.3.x

Release Notes

  • none

@vbotbuildovich
Copy link
Collaborator

new failures in https://buildkite.com/redpanda/redpanda/builds/42811#018c6a69-3acc-4358-859c-dbdf82ab7b2b:

"rptest.tests.recovery_mode_test.DisablingPartitionsTest.test_apis"

@vbotbuildovich
Copy link
Collaborator

@StephanDollberg
Copy link
Member

StephanDollberg commented Dec 15, 2023

It works fine as is but you need to strip the preceeding size and count as advised on https://vectorizedio.atlassian.net/wiki/spaces/CORE/pages/394756097/Heap+Memory+Sampling#On-OOM-Top-Allocation-Site-Printing

For example with something like:

cut -d ' ' -f3- - | /path/to/seastar/scripts/seastar-addr2line -e /path/to/redpanda

Are you saying that if you put backtrace: there it ignores everything in the line before that (for example also the timestamp etc)?

@StephanDollberg
Copy link
Member

If seastar-addr2line is actually that smart I think we can also prefix the size and count respectively to make it even more clear.

@travisdowns
Copy link
Member Author

travisdowns commented Dec 15, 2023

It works fine as is but you need to strip the preceeding size and count as advised on

It is good that it's documented though it would be even better if it "just worked" which is generally the intent with seastar-addr2line (as examining its rich soup of regexes will reveal). This especially important for our "on the fly" decoding which is implemented in CI for ducktape, which means the logs will have backtraces already decoded, which is much less painful than tracking down the binary and doing it yourself. (This feature is currently disabled to another bug but we can turn it on soon).

Are you saying that if you put backtrace: there it ignores everything in the line before that (for example also the timestamp etc)?

Yes. The regex is here:

re.compile(fr"^((?:.*(?:(?:at|backtrace):?|:))?(?:\s+))?({token}(?:\s+{token})*)(?:\).*|\s*)$", flags=re.IGNORECASE)

If at or backtrace are present, anything earlier in the line is matched with .* if the part after looks like a "one line" backtrace. Without the magic string, the backtrace must have nothing except optional whitespace before the sequence of frames.

If seastar-addr2line is actually that smart I think we can also prefix the size and count respectively to make it even more clear.

If you mean like backtrace: {} {} {} ? It won't work with the existing regex, but we could add to the regex (though it raises the complication of upstreaming since it's RP specific). Is your concern that the table headings aren't as correct? Would at: or at be better?

@travisdowns
Copy link
Member Author

travisdowns commented Dec 16, 2023

Failure was: #15617

Rebased.

@StephanDollberg
Copy link
Member

Yes. The regex is here

Ah brilliant, didn't know it was that smart.

Is your concern that the table headings aren't as correct?

No concern really just that we can make it simpler by doing size: {} count: {} backtrace: {} and then we can just remove the header?

@travisdowns
Copy link
Member Author

/ci-repeat 1

Move the format implementation out of the header for
ss::memory::allocation_site.
@travisdowns
Copy link
Member Author

travisdowns commented Jan 15, 2024

@StephanDollberg here's the output for the new upload:

Alloc sites legend:
    size: the estimated total size of all allocations at this stack (i.e., adjusted up from observed samples)
    count: the number of live samples at this stack (i.e., NOT adjusted up from observed samples)
    at: the backtrace for this allocation site
Top-N alloc sites:
size: 2670032930 count: 890 at: 0x73430da 0x6fc99b3 0x6fd7bcb 0x6dc62d7 0x48a15d8 0x48b68d3 0x7208353 0x7207d28 0x7207c9c 0x47e4c37 0x7211481 0x71c83bc 0x71dc29f 0x71db5bd 0x71d8e1b 0x71d5997 0x71d2ed8 0x4407545 0x4407464 0x7091e1f 0x7095466 0x7092726 0x6f97cac 0x6f95e38 0x436e639 0x4366919 /lib/x86_64-linux-gnu/libc.so.6+0x29d8f /lib/x86_64-linux-gnu/libc.so.6+0x29e3f 0x43667a4
size: 237002923 count: 79 at: 0x73430da 0x6fe3e21 0x6fcbd17 0x6fbd1d8 0x456abfb 0x456aefc 0x7091e1f 0x7095466 0x7092726 0x6f97cac 0x6f95e38 0x436e639 0x4366919 /lib/x86_64-linux-gnu/libc.so.6+0x29d8f /lib/x86_64-linux-gnu/libc.so.6+0x29e3f 0x43667a4
size: 33554432 count: 2 at: 0x73430da 0x6fc15fd 0x6fcc5b6 0x6fbd1d8 0x6cf1166 0x6cf1508 0x6cf0a05 0x436f7f0 0x438e10c 0x719209b
size: 16777216 count: 1 at: 0x73430da 0x6fc15fd 0x6fcc5b6 0x6fbd1d8 0x6cedaac 0x436f7dd 0x438e10c 0x719209b

WDYT?

I switched from backtrace to at since it's shorter (possibly relevant in "one line" logging mode), also accepted by the regex, and pretty clear. I added the legend as well.

When we OOM we output the top-N alloc sites and a one-line backtrace.

However, sesatar-addr2line fails to recognize it because the one-line
regex requires backtrace: or at: in order to understand that it's a
one-line backtrace.

So we add 'at:' in the string, which is a recognized token to
introduce a backtrace.

Fixes redpanda-data#15615.
@travisdowns travisdowns changed the title Add backtrace: in Top-N alloc site output Add at: in Top-N alloc site output Jan 15, 2024
@travisdowns travisdowns merged commit e5bfd7b into redpanda-data:dev Jan 15, 2024
22 checks passed
@vbotbuildovich
Copy link
Collaborator

/backport v23.3.x

@vbotbuildovich
Copy link
Collaborator

/backport v23.2.x

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

seastar-addr2line doesn't parse memory profile log lines
3 participants