You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by "yamanziadeh (via GitHub)" <gi...@apache.org> on 2025/07/15 20:36:30 UTC
[PR] Initial async-profiler Nodetool implementation [cassandra]
yamanziadeh opened a new pull request, #4255:
URL: https://github.com/apache/cassandra/pull/4255
Thanks for sending a pull request! Here are some tips if you're new here:
* Ensure you have added or run the [appropriate tests](https://cassandra.apache.org/_/development/testing.html) for your PR.
* Be sure to keep the PR description updated to reflect all changes.
* Write your PR title to summarize what this PR proposes.
* If possible, provide a concise example to reproduce the issue for a faster review.
* Read our [contributor guidelines](https://cassandra.apache.org/_/development/index.html)
* If you're making a documentation change, see our [guide to documentation contribution](https://cassandra.apache.org/_/development/documentation.html)
Commit messages should follow the following format:
```
<One sentence description, usually Jira title or CHANGES.txt summary>
<Optional lengthier description (context on patch)>
patch by <Authors>; reviewed by <Reviewers> for CASSANDRA-#####
Co-authored-by: Name1 <email1>
Co-authored-by: Name2 <email2>
```
The [Cassandra Jira](https://issues.apache.org/jira/projects/CASSANDRA/issues/)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSSIDECAR-254 - Include the AsyncProfiler Library and expose via JMX & `nodetool profile` [cassandra]
Posted by "JeetKunDoug (via GitHub)" <gi...@apache.org>.
JeetKunDoug commented on code in PR #4255:
URL: https://github.com/apache/cassandra/pull/4255#discussion_r2310306158
##########
src/java/org/apache/cassandra/profiler/AsyncProfilerMBean.java:
##########
@@ -0,0 +1,8 @@
+package org.apache.cassandra.profiler;
+
+public interface AsyncProfilerMBean {
+ void start(String event, String outputFormat);
Review Comment:
I do like the `executeUnsafe` method... trying to wrap my head around having a plain Map<String, String> for the options. Given JMX's limitations on using any kind of custom type across calling boundaries unless the custom type exists on both sides of the call, I can see it being a reasonable compromise as long as we carefully curate the options allowed to prevent potential security issues (like the output file issue you pointed out).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSSIDECAR-254 - Include the AsyncProfiler Library and expose via JMX & `nodetool profile` [cassandra]
Posted by "pauloricardomg (via GitHub)" <gi...@apache.org>.
pauloricardomg commented on code in PR #4255:
URL: https://github.com/apache/cassandra/pull/4255#discussion_r2302015065
##########
src/java/org/apache/cassandra/profiler/AsyncProfilerMBean.java:
##########
@@ -0,0 +1,8 @@
+package org.apache.cassandra.profiler;
+
+public interface AsyncProfilerMBean {
+ void start(String event, String outputFormat);
+ void stop(String outputFile);
Review Comment:
`outputFile` should not be part of the interface, since the nodetool caller does not know where the server is deployed and it can represent a security issue since it will allow a remote caller to create a file in the server host in a specific location. I think the output directory for async profiler should be specified in the server configuration and the server should generate a file with name based on the execution timestamp.
##########
src/java/org/apache/cassandra/tools/nodetool/Profile.java:
##########
@@ -0,0 +1,56 @@
+package org.apache.cassandra.tools.nodetool;
+
+import org.apache.cassandra.tools.NodeProbe;
+import org.apache.cassandra.profiler.AsyncProfilerMBean;
+
+import picocli.CommandLine.Command;
+import picocli.CommandLine.Option;
+
+@Command(name = "profile", description = "Run Async-Profiler on the cassandra process")
+public class Profile extends AbstractCommand {
+
+ @Option(names = {"-s", "--start"}, description = "Start profiling")
+ public boolean start;
+
+ @Option(names = {"-x", "--stop"}, description = "Stop profiling and dump output")
+ public boolean stop;
+
+ @Option(names = {"-e", "--event"}, description = "Event to profile (cpu, alloc, lock, wall, etc.)")
+ public String event = "cpu";
+
+ @Option(names = {"-r", "--raw"}, description = "Raw commands to execute")
+ public String raw;
+
+ @Option(names = {"-o", "--output"}, description = "Output file for profile dump")
+ public String outputFile = "/tmp/profile.html";
+
+ @Option(names = {"-f", "--format"}, description = "Output format (flamegraph, tree, traces, etc.)")
+ public String outputFormat = "flamegraph";
+
+ @Override
+ public void execute(NodeProbe probe) {
+ AsyncProfilerMBean profiler = probe.getAsyncProfilerProxy();
+ if (!profiler.isAvailable()) {
+ System.err.println("Async-profiler native library is not loaded or unavailable.");
+ return;
+ }
+
+ try {
+ if (start) {
Review Comment:
all Cassandra braces should be on a newline, throughout all files
##########
src/java/org/apache/cassandra/tools/profiler/AsyncProfilerService.java:
##########
@@ -0,0 +1,111 @@
+package org.apache.cassandra.tools.profiler;
+
+import one.profiler.AsyncProfiler;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.io.File; //checkstyle: permit this import
+import java.io.IOException;
+import java.util.Arrays;
+import java.util.Set;
+
+import static org.apache.cassandra.config.CassandraRelevantProperties.ASYNC_PROFILER_LIB_PATH;
+import static org.apache.cassandra.config.CassandraRelevantProperties.ASYNC_PROFILER_ENABLED;
+import static org.apache.cassandra.config.CassandraRelevantProperties.ASYNC_PROFILER_ADVANCED_MODE;
+
+public class AsyncProfilerService {
+ private static final Logger logger = LoggerFactory.getLogger(AsyncProfilerService.class);
+
+ private static final Set<String> VALID_EVENTS = Set.of("cpu", "alloc", "lock", "wall", "nativemem", "cache-misses");
+ private static final Set<String> VALID_FORMATS = Set.of("flat","traces","collapsed","flamegraph","tree","jfr","otlp");
+ private static final Character[] INVALID_OUTPUT_FILENAME_CHARS = {'"', '*', '<', '>', '?', '|'};
+
+ private static AsyncProfiler profilerInstance;
+
+ static {
+ try {
+ String asyncProfilerLibPath = new File(ASYNC_PROFILER_LIB_PATH.getString()).getAbsolutePath(); //checkstyle: permit this instantiation
Review Comment:
This OSX specific workaround might not be needed in newer versions of async-profiler, seems to have been fixed on https://github.com/async-profiler/async-profiler/issues/378
Can you try with the latest async profiler version to check if this is needed? Thanks
##########
src/java/org/apache/cassandra/tools/nodetool/Profile.java:
##########
@@ -0,0 +1,56 @@
+package org.apache.cassandra.tools.nodetool;
+
+import org.apache.cassandra.tools.NodeProbe;
+import org.apache.cassandra.profiler.AsyncProfilerMBean;
+
+import picocli.CommandLine.Command;
+import picocli.CommandLine.Option;
+
+@Command(name = "profile", description = "Run Async-Profiler on the cassandra process")
+public class Profile extends AbstractCommand {
+
+ @Option(names = {"-s", "--start"}, description = "Start profiling")
+ public boolean start;
+
+ @Option(names = {"-x", "--stop"}, description = "Stop profiling and dump output")
+ public boolean stop;
+
+ @Option(names = {"-e", "--event"}, description = "Event to profile (cpu, alloc, lock, wall, etc.)")
+ public String event = "cpu";
+
+ @Option(names = {"-r", "--raw"}, description = "Raw commands to execute")
+ public String raw;
+
+ @Option(names = {"-o", "--output"}, description = "Output file for profile dump")
+ public String outputFile = "/tmp/profile.html";
+
+ @Option(names = {"-f", "--format"}, description = "Output format (flamegraph, tree, traces, etc.)")
+ public String outputFormat = "flamegraph";
+
+ @Override
+ public void execute(NodeProbe probe) {
+ AsyncProfilerMBean profiler = probe.getAsyncProfilerProxy();
+ if (!profiler.isAvailable()) {
+ System.err.println("Async-profiler native library is not loaded or unavailable.");
+ return;
+ }
+
+ try {
+ if (start) {
Review Comment:
ie.:
```java
try
{
if (start)
{
}
}
catch (Exception e)
{
// handle
}
```
##########
src/java/org/apache/cassandra/profiler/AsyncProfilerMBean.java:
##########
@@ -0,0 +1,8 @@
+package org.apache.cassandra.profiler;
+
+public interface AsyncProfilerMBean {
+ void start(String event, String outputFormat);
Review Comment:
Instead of receiving only the `event` as parameter for `start`, I think it would be more extensible to receive a `Map<String,String> options` map, where `event` will be the initially supported [option](https://github.com/async-profiler/async-profiler/blob/master/docs/ProfilerOptions.md#options-applicable-to-any-output-format) in this map. Any other option should be rejected.
In the future when we want to support more advanced options we don't need to change the interface.
So this would become:
```java
void start(Map<String, String> options);
void stop();
void executeUnsafe(String); // to emphasize this is "use at your own risk"
boolean isAvailable();
```
LMK what you think
##########
src/java/org/apache/cassandra/profiler/AsyncProfilerMBean.java:
##########
@@ -0,0 +1,8 @@
+package org.apache.cassandra.profiler;
+
+public interface AsyncProfilerMBean {
+ void start(String event, String outputFormat);
+ void stop(String outputFile);
Review Comment:
In order to address this, we can have a `"cassandra.async_profiler.output_directory"` option and create a file in this directory, for example: `/tmp/cassandra-profiling/20252608T151934.html`.
##########
src/java/org/apache/cassandra/tools/profiler/AsyncProfilerService.java:
##########
@@ -0,0 +1,111 @@
+package org.apache.cassandra.tools.profiler;
+
+import one.profiler.AsyncProfiler;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.io.File; //checkstyle: permit this import
+import java.io.IOException;
+import java.util.Arrays;
+import java.util.Set;
+
+import static org.apache.cassandra.config.CassandraRelevantProperties.ASYNC_PROFILER_LIB_PATH;
+import static org.apache.cassandra.config.CassandraRelevantProperties.ASYNC_PROFILER_ENABLED;
+import static org.apache.cassandra.config.CassandraRelevantProperties.ASYNC_PROFILER_ADVANCED_MODE;
+
+public class AsyncProfilerService {
+ private static final Logger logger = LoggerFactory.getLogger(AsyncProfilerService.class);
+
+ private static final Set<String> VALID_EVENTS = Set.of("cpu", "alloc", "lock", "wall", "nativemem", "cache-misses");
+ private static final Set<String> VALID_FORMATS = Set.of("flat","traces","collapsed","flamegraph","tree","jfr","otlp");
+ private static final Character[] INVALID_OUTPUT_FILENAME_CHARS = {'"', '*', '<', '>', '?', '|'};
+
+ private static AsyncProfiler profilerInstance;
+
+ static {
+ try {
+ String asyncProfilerLibPath = new File(ASYNC_PROFILER_LIB_PATH.getString()).getAbsolutePath(); //checkstyle: permit this instantiation
+ profilerInstance = AsyncProfiler.getInstance(asyncProfilerLibPath);
+ } catch (Throwable t) {
+ System.out.println("async-profiler initialization ERROR");
+ t.printStackTrace();
+ profilerInstance = null;
+ }
+ }
+
+ public void start(String event, String outputFormat) {
+ checkProfilerInstance();
+ validateEvent(event);
+ validateFormat(outputFormat);
+
+ try {
+ String cmd = String.format("start,event=%s,output=%s", event, outputFormat);
Review Comment:
In order to prevent an async profile session from running indefinitely let's add a default timeout of 60s and make that changeable via the `timeout` [option](https://github.com/async-profiler/async-profiler/blob/master/docs/ProfilerOptions.md#options-applicable-to-any-output-format):
> The profiling duration, in seconds. The profiler will run for the specified period of time and then automatically stop.
The user should still be able to stop a session with a timeout with via the `stop` method.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSSIDECAR-254 - Include the AsyncProfiler Library and expose via JMX & `nodetool profile` [cassandra]
Posted by "JeetKunDoug (via GitHub)" <gi...@apache.org>.
JeetKunDoug commented on code in PR #4255:
URL: https://github.com/apache/cassandra/pull/4255#discussion_r2387840357
##########
src/java/org/apache/cassandra/profiler/AsyncProfilerMBean.java:
##########
@@ -0,0 +1,8 @@
+package org.apache.cassandra.profiler;
+
+public interface AsyncProfilerMBean {
+ void start(String event, String outputFormat);
Review Comment:
So here's a different option - what if we have 2 mbeans, one for the "safe" endpoint and another for the "unsafe" - that way, people who, for some environmental reasons, really don't want the unsafe version in their system can just delete the implementation .class file (or we could have a build option to include/exclude it, but I think that may be a pain). This would allow for someone to deploy a version of Cassandra that literally could not run the `executeUnsafe` if they didn't want it available.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSSIDECAR-254 - Include the AsyncProfiler Library and expose via JMX & `nodetool profile` [cassandra]
Posted by "rustyrazorblade (via GitHub)" <gi...@apache.org>.
rustyrazorblade commented on code in PR #4255:
URL: https://github.com/apache/cassandra/pull/4255#discussion_r2311341193
##########
src/java/org/apache/cassandra/profiler/AsyncProfilerMBean.java:
##########
@@ -0,0 +1,8 @@
+package org.apache.cassandra.profiler;
+
+public interface AsyncProfilerMBean {
+ void start(String event, String outputFormat);
+ void stop(String outputFile);
Review Comment:
I think it would be better to keep outputFile and not allow the user to pick arbitrary directories. I have profiling built into easy-cass-lab using timestamps, and I end up losing track of which one correlated to what. I regret having built it that way.
Without a custom filename, the artifacts quickly become meaningless.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSSIDECAR-254 - Include the AsyncProfiler Library and expose via JMX & `nodetool profile` [cassandra]
Posted by "JeetKunDoug (via GitHub)" <gi...@apache.org>.
JeetKunDoug commented on code in PR #4255:
URL: https://github.com/apache/cassandra/pull/4255#discussion_r2387830828
##########
src/java/org/apache/cassandra/profiler/AsyncProfilerMBean.java:
##########
@@ -0,0 +1,8 @@
+package org.apache.cassandra.profiler;
+
+public interface AsyncProfilerMBean {
+ void start(String event, String outputFormat);
+ void stop(String outputFile);
Review Comment:
I'd trust Jon on this - use the prop to specify the root directory, provide a filename as an option, but default to the timestamped file if no output filename is provided... just make sure you sanitize the input so we don't introduce a path traversal issue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSSIDECAR-254 - Include the AsyncProfiler Library and expose via JMX & `nodetool profile` [cassandra]
Posted by "JeetKunDoug (via GitHub)" <gi...@apache.org>.
JeetKunDoug commented on code in PR #4255:
URL: https://github.com/apache/cassandra/pull/4255#discussion_r2311337201
##########
src/java/org/apache/cassandra/config/CassandraRelevantProperties.java:
##########
@@ -60,6 +60,9 @@ public enum CassandraRelevantProperties
ALLOW_UNSAFE_REPLACE("cassandra.allow_unsafe_replace"),
ALLOW_UNSAFE_TRANSIENT_CHANGES("cassandra.allow_unsafe_transient_changes"),
APPROXIMATE_TIME_PRECISION_MS("cassandra.approximate_time_precision_ms", "2"),
+ ASYNC_PROFILER_ADVANCED_MODE("cassandra.async_profiler.advanced_mode", "false"),
+ ASYNC_PROFILER_ENABLED("cassandra.async_profiler.enabled", "true"),
Review Comment:
I assume there's essentially 0 overhead to having the mbean registered right?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org