-
Notifications
You must be signed in to change notification settings - Fork 2
chore: add persistent data for local pyroscope #1623
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
base: main
Are you sure you want to change the base?
Conversation
Reviewer's GuideThis PR refactors the local Pyroscope setup by introducing a named Docker volume for persistent data and expanding the hack script and Taskfile with granular start/stop/data commands, while adding a standalone compose file for Pyroscope-only deployments. Class diagram for pyroscope.sh command functionsclassDiagram
class pyroscope_sh {
+start()
+stop()
+wipe_data()
+info_data()
+start_pyroscope()
+stop_pyroscope()
}
Flow diagram for new pyroscope.sh command structureflowchart TD
A["pyroscope.sh"] --> B["start"]
A --> C["stop"]
A --> D["wipe-data"]
A --> E["info-data"]
A --> F["start-pyroscope"]
A --> G["stop-pyroscope"]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey there - I've reviewed your changes - here's some feedback:
- Rename bash functions to use underscores instead of hyphens (e.g. wipe_data, info_data) since hyphens aren’t valid in function names and will cause syntax errors.
- Add a confirmation step or safety check before running the destructive wipe-data command to avoid accidental volume removal.
- Refactor the duplicated start/stop logic for full vs. pyroscope-only modes by parameterizing the compose file path instead of repeating almost identical functions.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Rename bash functions to use underscores instead of hyphens (e.g. wipe_data, info_data) since hyphens aren’t valid in function names and will cause syntax errors.
- Add a confirmation step or safety check before running the destructive wipe-data command to avoid accidental volume removal.
- Refactor the duplicated start/stop logic for full vs. pyroscope-only modes by parameterizing the compose file path instead of repeating almost identical functions.
## Individual Comments
### Comment 1
<location> `images/virtualization-artifact/hack/pyroscope.sh:107-109` </location>
<code_context>
- run
+ "start")
+ start
+ echo "Pyroscope launched successfully."
+ echo "Open http://localhost:4040 in your browser."
+ ;;
+ "stop")
</code_context>
<issue_to_address>
**suggestion:** Consider making the success message conditional on actual launch success.
Check the exit status of the start command and print the success message only if it completes successfully.
```suggestion
start
if [ $? -eq 0 ]; then
echo "Pyroscope launched successfully."
echo "Open http://localhost:4040 in your browser."
else
echo "Pyroscope failed to launch." >&2
fi
```
</issue_to_address>
### Comment 2
<location> `images/virtualization-artifact/Taskfile.yaml:126-128` </location>
<code_context>
- pyroscope:local:wipe:vm-route-forge:
- desc: "Wipe local pyroscope for vm-route-forge"
- cmd: ./hack/pyroscope.sh wipe --namespace={{ .BaseNamespace }}
+ pyroscope:local:info-data:
+ desc: "Show local pyroscope dat info"
+ cmd: ./hack/pyroscope.sh info-data
</code_context>
<issue_to_address>
**nitpick (typo):** Typo in description: 'dat' should be 'data'.
Please update the description to use 'data' instead of 'dat'.
```suggestion
pyroscope:local:info-data:
desc: "Show local pyroscope data info"
cmd: ./hack/pyroscope.sh info-data
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
9d27f9c to
44eb9ed
Compare
Signed-off-by: Yaroslav Borbat <[email protected]>
3b73e10 to
865b580
Compare
Description
add persistent data for local pyroscope
Why do we need it, and what problem does it solve?
What is the expected result?
Checklist
Changelog entries