Add internal profiling to debug tool #32423
Open
+784
−318
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
SangJunBak
force-pushed
the
jun/debug-tool/add-internal-profiling
branch
2 times, most recently
from
7a2d70c to
754036d
Compare
- Instead of waiting until port forwarding, we read from stderr until we know for sure port forwarding has started - Renamed `create_kubectl_port_forwarder` to `create_pg_wire_port_forwarder` since we'll be port forwarding more than once - Removed the retry mechanism given we don't see the port forwarding disconnecting too often and we can't do it AND initial acknowledgement together easily - Tie child process to a struct instead of a tokio spawn.
We create an initialization function that preprocesses as much as possible and maps args to context values.
I kept the trait but realized the enum is a bit redundant, especially since we have the Context enum
SangJunBak
force-pushed
the
jun/debug-tool/add-internal-profiling
branch
from
754036d to
a2867a8
Compare
Given we're always creating a PathBuf from start time, it makes sense to just pass the PathBuf itself
- Use stdout instead of stderr - Remove sharing of context - Fix bug where process was being killed on connecting to port forward
- Introduced InternalHttpDumpClient for downloading and saving heap profile data and Prometheus metrics from internal HTTP endpoints. - Changes service for port forwarding from balancerd to environmentd for better consistency with metrics - Refactor "find" methods for port forwarding to be more modular. Separates out port finding / port forwarder creation from service finding
- Introduced a new asynchronous function `get_container_ip` to fetch the IP address of a Docker container using its ID. Doing this means we don't need the user to map ports when doing `DOCKER RUN` - Changes mz_connection_url for the emulator to an optional argument and global now that we have the container's IP
- Update CLI flags in docs and clippy/lint errors
ParkMyCar
approved these changes
May 14, 2025
| @@ -1,7 +1,7 @@ | |||
| [package] | |||
| name = "mz-debug" | |||
| description = "Debug tool for self-managed Materialize." | |||
| version = "0.1.0" | |||
| version = "0.2.0" | |||
There was a problem hiding this comment.
bumping the version like this is a good habit, but out of curiosity is it necessary? do we publish it anywhere?
There was a problem hiding this comment.
Not necessary, just something manual I do whenever I decide to publish haha. We do publish it as a curlable binary that we expose via the docs!
| static INTERNAL_HOST_ADDRESS: &str = "127.0.0.1"; | ||
| static INTERNAL_HTTP_PORT: i32 = 6878; | ||
|
|
||
| /// A struct that handles downloading and saving profile data from HTTP endpoints |
There was a problem hiding this comment.
nit: we always try to end comments with periods, in other words, make them complete sentences.
| output_path: &Path, | ||
| ) -> Result<(), anyhow::Error> { | ||
| // Try HTTPS first, then fall back to HTTP if that fails | ||
| let mut url = format!("https://{}", relative_url); |
There was a problem hiding this comment.
Instead of manually formatting the URL like this I would opt to use the url crate and then the set_scheme(...) method on the Url type
|
|
||
| /// Downloads and saves heap profile data | ||
| pub async fn dump_heap_profile(&self, relative_url: &str, service_name: &str) -> Result<()> { | ||
| let output_dir = self.context.base_path.join("profiles"); |
There was a problem hiding this comment.
Maybe these directory names should be constants that are grouped at the top of the file? That way it's easier to determine what folders the debug tool might create? Not blocking but just a thought
| ContainerServiceDumper::Docker(dumper) => dumper.dump_container_resources().await, | ||
| } | ||
| } | ||
| enum DebugModeContext { |
There was a problem hiding this comment.
Nice! I like how this turned out!
5 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
See commit messages for details
Motivation
"Scrape prometheus metrics" and "Scrape profiling" from https://github.com/MaterializeInc/database-issues/issues/8908
Tips for reviewer
Checklist
$T ⇔ Proto$Tmapping (possibly in a backwards-incompatible way), then it is tagged with aT-protolabel.