chore: Added multiple scripts to trigger profiles#35546
Conversation
WalkthroughThe recent updates introduce three new Bash scripts to facilitate monitoring and debugging of Java applications running in Docker. The scripts enable automated recording of heap dumps, thread dumps, and Java Flight Recorder (JFR) profiling. These enhancements streamline the process of capturing critical performance data, aiding developers and system administrators in diagnosing memory usage and performance issues efficiently. Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
github-actions
bot
added
the
skip-changelog
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/10301842036. |
|
Deploy-Preview-URL: https://ce-35546.dp.appsmith.com |
pratapaprasanna
left a comment
pratapaprasanna
left a comment
There was a problem hiding this comment.
LGTM, My Suggestion is
Please check the permission . Lets not make files executable by default here.
rather modify permissions in Dockerfile
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (4)
- deploy/docker/fs/opt/appsmith/record-heap-dump.sh (1 hunks)
- deploy/docker/fs/opt/appsmith/record-thread-dump.sh (1 hunks)
- deploy/docker/fs/opt/appsmith/thread-profile-start.sh (1 hunks)
- deploy/docker/fs/opt/appsmith/thread-profile-stop.sh (1 hunks)
Additional comments not posted (4)
deploy/docker/fs/opt/appsmith/thread-profile-start.sh (4)
1-1: Good practice: Use of shebang.The script starts with a shebang (
#!/bin/bash), which is a good practice to ensure the script runs with the correct interpreter.
3-6: Excellent use of shell options for safety.The script uses
set -o errexit,set -o pipefail,set -o nounset, andset -o noglob. These options enhance the script's safety by ensuring it exits on error, handles pipes correctly, avoids using unset variables, and prevents filename expansion.
10-10: Verify process identification and command execution.The command
jcmd $(pgrep -f -- "-jar\sserver.jar") JFR.startrelies onpgrepto find the Java process. Ensure that the pattern uniquely identifies the correct process and thatjcmdis available in the environment.
8-9: Ensure directory creation is secure.The script creates a directory with
mkdir -p $location. Ensure that the base path/appsmith-stacks/heap_dumps/ad-hoc/is secure and not writable by unauthorized users to prevent potential security risks.
| #!/bin/bash | ||
|
|
||
| set -o errexit | ||
| set -o pipefail | ||
| set -o nounset | ||
| set -o noglob |
There was a problem hiding this comment.
Consider enhancing error handling and logging.
While the script is set to fail on errors with set -o errexit, it would be beneficial to include logging for better traceability and debugging. Additionally, consider handling scenarios where pgrep might return multiple PIDs or none at all.
#!/bin/bash
set -o errexit
set -o pipefail
set -o nounset
set -o noglob
process_id=$(pgrep -f -- "-jar\sserver.jar" || echo "")
if [ -z "$process_id" ]; then
echo "Error: No Java process found." >&2
exit 1
fi
jcmd $process_id JFR.dump name=profile
echo "JFR profile dumped successfully."| location=/appsmith-stacks/heap_dumps/ad-hoc/$(date "+%Y_%m_%d_%H_%S")/heap-profile; | ||
| mkdir -p $location; | ||
| jcmd $(pgrep -f -- "-jar\sserver.jar") GC.heap_dump filename=$location/${HOSTNAME}.log No newline at end of file |
There was a problem hiding this comment.
Enhance error handling and directory creation checks.
While the script is functional, adding checks for directory creation and process identification can improve robustness. Consider logging actions for better traceability.
location=/appsmith-stacks/heap_dumps/ad-hoc/$(date "+%Y_%m_%d_%H_%S")/heap-profile
mkdir -p "$location" || { echo "Error: Failed to create directory $location." >&2; exit 1; }
process_id=$(pgrep -f -- "-jar\sserver.jar" || echo "")
if [ -z "$process_id" ]; then
echo "Error: No Java process found." >&2
exit 1
fi
jcmd $process_id GC.heap_dump filename="$location/${HOSTNAME}.log"
echo "Heap dump recorded successfully at $location."| location=/appsmith-stacks/heap_dumps/ad-hoc/$(date "+%Y_%m_%d_%H_%S")/thread-profile; | ||
| mkdir -p $location; | ||
| jcmd $(pgrep -f -- "-jar\sserver.jar") Thread.print > $location/trace-${HOSTNAME}.log No newline at end of file |
There was a problem hiding this comment.
Improve error handling and directory creation checks.
To ensure robustness, add checks for directory creation and process identification. Logging actions can also aid in debugging and traceability.
location=/appsmith-stacks/heap_dumps/ad-hoc/$(date "+%Y_%m_%d_%H_%S")/thread-profile
mkdir -p "$location" || { echo "Error: Failed to create directory $location." >&2; exit 1; }
process_id=$(pgrep -f -- "-jar\sserver.jar" || echo "")
if [ -z "$process_id" ]; then
echo "Error: No Java process found." >&2
exit 1
fi
jcmd $process_id Thread.print > "$location/trace-${HOSTNAME}.log"
echo "Thread dump recorded successfully at $location."2 participants
Description
Tip
Add a TL;DR when the description is longer than 500 words or extremely technical (helps the content, marketing, and DevRel team).
Please also include relevant motivation and context. List any dependencies that are required for this change. Add links to Notion, Figma or any other documents that might be relevant to the PR.
Fixes #
Issue Numberor
Fixes
Issue URLWarning
If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.
Automation
/ok-to-test tags=""
🔍 Cypress test results
Warning
Tests have not run on the HEAD c684235 yet
Mon, 12 Aug 2024 13:59:02 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
record-heap-dump.sh: Automates heap dump creation.record-thread-dump.sh: Collects thread dumps for performance analysis.thread-profile.sh: Manages Java Flight Recorder profiling sessions.These scripts streamline the process of identifying memory usage and performance issues in Java applications, providing users with essential tools for effective monitoring.