8000 Add support for grafana-pyroscope to besu by joshuafernandes · Pull Request #8510 · hyperledger/besu · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Add support for grafana-pyroscope to besu #8510

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
May 8, 2025
Merged

Add support for grafana-pyroscope to besu #8510

merged 3 commits into from
May 8, 2025
+13 −0

Conversation

joshuafernandes
Copy link
Contributor
@joshuafernandes joshuafernandes commented Apr 3, 2025 •
edited
Loading

PR description

Add support for grafana-pyroscope to besu

To use, set the following env vars in compose, k8s etc

JAVA_OPTS="-XX:-Inline -javaagent:/opt/pyroscope/lib/pyroscope.jar"
PYROSCOPE_SERVER_ADDRESS=http://my_pyroscope_endpoint:4040

image

Thanks for sending a pull request! Have you done the following?

  • Checked out our contribution guidelines?
  • Considered documentation and added the doc-change-required label to this PR if updates are required.
  • Considered the changelog and included an update if required.
  • For database changes (e.g. KeyValueSegmentIdentifier) considered compatibility and performed forwards and backwards compatibility tests

Locally, you can run these tests to catch failures early:

  • spotless: ./gradlew spotlessApply
  • unit tests: ./gradlew build
  • acceptance tests: ./gradlew acceptanceTest
  • integration tests: ./gradlew integrationTest
  • reference tests: ./gradlew ethereum:referenceTests:referenceTests

@joshuafernandes joshuafernandes changed the title add support for grafana-pyroscope to besu Add support for grafana-pyroscope to besu Apr 3, 2025
@joshuafernandes joshuafernandes requested review from ahamlat and matkt April 3, 2025 01:23
@joshuafernandes joshuafernandes force-pushed the grafana-pyroscope branch 2 times, most recently from abbe624 to 640c189 Compare April 3, 2025 01:30
@joshuafernandes
add support for grafana-pyroscope to besu
ffe4c24 
Signed-off-by: Joshua Fernandes <joshua.fernandes@consensys.net>
@macfarla macfarla requested a review from lu-pinto April 3, 2025 01:31
ahamlat
ahamlat reviewed Apr 3, 2025
View reviewed changes
Copy link
Contributor
@ahamlat ahamlat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can have a non negligible performance impact as we're disabling inlining. Is it possible to without disabling inlining, otherwise we need to not make it default, but enable it when needed.

@joshuafernandes
Copy link
Contributor Author
joshuafernandes commented Apr 4, 2025 •
edited
Loading

This can have a non negligible performance impact as we're disabling inlining. Is it possible to without disabling inlining, otherwise we need to not make it default, but enable it when needed.

Lost me @ahamlat . There is 2 ways to inject pyro in:

  1. Above method via inlining (Note this isn't set as default - you have to set JAVA_OPTS as above on the running env first). All this PR does is have the jar ready to use but the operator needs to update JAVA_OPTS to turn it on and PYROSCOPE_SERVER_ADDRESS to tell it where to sent data to
  2. Via gradle and you control it as needed - see docs https://grafana.com/docs/pyroscope/latest/configure-client/language-sdks/java/

@ahamlat
Copy link
Contributor
ahamlat commented Apr 4, 2025 •
edited
Loading

I agree that the Java options are on the user side, but we can add that -XX:-Inline helps to keep a stacktrace that similar to the code, but we can also not add it and still have Pyroscope working fine.
Maybe other options that will be needed from the use side are -XX:+UnlockDiagnosticVMOptions -XX:+DebugNonSafepoints, that helps all the profilers to have more accurate profiling data.
Can we hold off this until we're able to test it in our Grafana stack ?

@github-actions GitHub Actions
Copy link
github-actions bot commented May 5, 2025

This pr is stale because it has been open for 30 days with no activity.

@github-actions github-actions bot added the Stale label May 5, 2025
ahamlat
ahamlat approved these changes May 6, 2025
View reviewed changes
Copy link
Contributor
@ahamlat ahamlat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@github-actions github-actions bot removed the Stale label May 7, 2025
@macfarla
Copy link
Contributor
macfarla commented May 8, 2025

maybe worth a changelog entry

macfarla added 2 commits May 9, 2025 05:57
@macfarla
Merge branch 'main' of github.com:hyperledger/besu into grafana-pyros…
5a38fec 
…cope
@macfarla
changelog
772d643 
Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>
@macfarla macfarla added the doc-change-required Indicates an issue or PR that requires doc to be updated label May 8, 2025
@macfarla macfarla enabled auto-merge (squash) May 8, 2025 20:06
@macfarla macfarla merged commit b8cd4c8 into main May 8, 2025
48 checks passed
@macfarla macfarla deleted the grafana-pyroscope branch May 8, 2025 20:27
@alexandratran alexandratran mentioned this pull request May 19, 2025
Open
@alexandratran alexandratran removed the doc-change-required Indicates an issue or PR that requires doc to be updated label May 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ahamlat ahamlat ahamlat approved these changes

@matkt matkt Awaiting requested review from matkt

@lu-pinto lu-pinto Awaiting requested review from lu-pinto

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@joshuafernandes @ahamlat @macfarla @alexandratran
0