-
Notifications
You must be signed in to change notification settings - Fork 471
Add pprof profile offline symbolization tool #32496
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
Conversation
0dc3c62
to
3d04676
Compare
@@ -36,6 +36,7 @@ pdoc==15.0.3 | |||
# We can revert back to standard pg8000 versions once https://github.com/tlocke/pg8000/pull/161 is released | |||
pg8000@git+https://github.com/tlocke/pg8000@46c00021ade1d19466b07ed30392386c5f0a6b8e | |||
prettytable==3.16.0 | |||
protobuf==5.29.3 |
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.
Specifically used this version since that's the version used in profile.proto
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.
This will be auto-upgraded by dependabot. Is that a problem?
|
||
```bash | ||
# Set up AWS credentials | ||
export AWS_PROFILE=mz-cloud-production-engineering-on-call |
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.
So for me, this is the only AWS profile with access to that bucket. For some reason, mz-core-admin
doesn't work. Let me know if I should change it but I assume this is the same for most and although it's kinda suspicious using an overprivileged profile, I'd rather have one that works than not
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.
I guess some devs won't have access to this profile. We should probably make the S3 bucket more available.
@@ -0,0 +1,233 @@ | |||
// Copyright 2016 Google Inc. All Rights Reserved. |
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.
As said in the readme, this is received from a CURL. Not sure if we need to do something to account for this apache license.
A potential worry is the pprof.gz
provided doesn't match this proto, but given protobufs are supposed to be backwards compatible and because this .proto
file (one used for pprof files) is most likely stable, I thought it'd be okay
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.
Noticing this is failing the check-copyright lint test. I can fix this via changing the header to
// Copyright Materialize, Inc. and contributors. All rights reserved.
//
// Use of this software is governed by the Business Source License
// included in the LICENSE file.
//
// As of the Change Date specified in that file, in accordance with
// the Business Source License, use of this software will be governed
// by the Apache License, Version 2.0.
but this seems quite suspicious to just override a copyright header like that. Any advice?
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.
I have added an ignore for the copyright check in the linters.
647f017
to
6bbea6f
Compare
- Introduced a new tool for symbolizing and visualizing pprof profiles using debug symbols from S3. - Updated `requirements.txt` to include `protobuf==5.29.3`.
6bbea6f
to
fa2e919
Compare
…rom copyright check
3ee9379
to
e352783
Compare
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.
Maybe I'm a bit confused of what the purpose of this is. Is it just doing the same as the instructions in "Analyzing pprof for a release build" section of https://www.notion.so/materialize/analyzing-pprof-for-a-release-build-3fa5a68aef994d90b3c94bca6eea4da8 ? That always worked well for me, I'm not sure if we need another tool for it.
|
||
# macOS | ||
bin/pyactivate -m ci.deploy_mz-debug.macos | ||
|
||
# Linux | ||
bin/pyactivate -m ci.deploy_mz-debug.linux | ||
``` | ||
where AWS_PROFILE is the profile with access to the materialize-binaries S3 bucket in the Materialize Core account. |
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.
I don't think devs have access to the Materialize Core account normally? At least I don't.
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 I always used to use is https://debuginfo.dev.materialize.com/buildid/d6a8b86c62ce2b7a6fd146a048ebba77/executable, which is publicly available. I think that's easier instead of using S3.
) | ||
|
||
# Install graphviz in the container | ||
subprocess.run( |
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.
Shouldn't this just be a Dockerfile?
@def- oh no way I didn't know this existed! I think if the public URL didn't exist, I'd argue for a tool. But given we'll most likely need a doc that points to this tool after someone generates the Another thing that isn't clear is the binaries in the debuginfo s3 bucket don't seem stripped (i.e. materilaized:latest's binary is over a gigabyte) but an extra step that the guide doesn't account for is not only supplying the path to the |
On second thought: I think a good meet in the middle solution would be to generate a bash script per profile in the debug tool and expect users (us when debugging internally) to have pprof, graphviz, and curl installed. That way users can just run the script and don't have to look up a notion doc. I have an ongoing notion doc for using the debug tool and I'll be sure to document the script as well as the manual way (the notion doc you posted). I'll create a separate PR for this and close this one out! |
One thing that would be nice in the script is to be able to pass all kinds of parameters to pprof that it supports. |
See commit messages for details
Motivation
Followup of #32423 and https://github.com/MaterializeInc/database-issues/issues/8908 where the debug tool scrapes the profiles and us, internally, can run this python script internally to spin up a docker container with a UI similar to pprof.me.
To easily test:
Open to suggestions given it's my first time writing a script like this!
Note: We still need to implement CPU profiling in an unsymbolized format then backport it into LTS. We only have memory profiles.
We also have to figure out why the latest binary for the emulator is like over a gigabyte if they're supposed to be stripped 😅
Tips for reviewer
Checklist
$T ⇔ Proto$T
mapping (possibly in a backwards-incompatible way), then it is tagged with aT-proto
label.