Skip to content
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

Added list_jvms option to list available JVMs (that this process can … #100

Closed
wants to merge 6 commits into from
Closed

Added list_jvms option to list available JVMs (that this process can … #100

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
4ddf9fa
Added list_jvms option to list available JVMs (that this process can …
cslee00 May 14, 2016
4a83168
Adjustements based on pull request feedback
cslee00 Jun 4, 2016
100c26f
Fix crash on OpenJDK
cslee00 Jun 4, 2016
6715343
Fix crash on OpenJDK
cslee00 Jun 4, 2016
0b44718
Fix crash on OpenJDK
cslee00 Jun 4, 2016
60f3744
Fix crash on OpenJDK
cslee00 Jun 4, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions src/main/java/org/datadog/jmxfetch/App.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@

import com.beust.jcommander.JCommander;
import com.beust.jcommander.ParameterException;
import com.sun.tools.attach.VirtualMachineDescriptor;

@SuppressWarnings("unchecked")
public class App {
Expand Down Expand Up @@ -320,8 +321,19 @@ public void init(boolean forceNewConnection) {
clearInstances(instances);
clearInstances(brokenInstances);


Reporter reporter = appConfig.getReporter();

String action = appConfig.getAction();

if( action.equals( AppConfig.ACTION_LIST_JVMS )) {
List<VirtualMachineDescriptor> descriptors = com.sun.tools.attach.VirtualMachine.list();
Copy link
Member

Choose a reason for hiding this comment

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

Can we import com.sun.tools.attach.VirtualMachine along with com.sun.tools.attach.VirtualMachineDescriptor in the header ?

for( VirtualMachineDescriptor descriptor : descriptors ) {
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about adding a top message before listing alls JVMs to emphasize that only the ones associated to the user are listed. Something like,

List of JVMs for $user:
  JVM id $id: $name
  [...]

System.out.println( "\tJVM id " + descriptor.id() + ": '" + descriptor.displayName() + "'" );
}
return;
}

Copy link
Member

Choose a reason for hiding this comment

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

I think this whole block should be moved to the main() method instead, i.e. here for instance.

Iterator<Entry<String, YamlParser>> it = configs.entrySet().iterator();
while (it.hasNext()) {
Map.Entry<String, YamlParser> entry = it.next();
Expand Down
5 changes: 3 additions & 2 deletions src/main/java/org/datadog/jmxfetch/AppConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,15 @@
@Parameters(separators = "=")
class AppConfig {
public static final String ACTION_COLLECT = "collect";
public static final String ACTION_LIST_JVMS = "list_jvms";
public static final String ACTION_LIST_EVERYTHING = "list_everything";
public static final String ACTION_LIST_COLLECTED = "list_collected_attributes";
public static final String ACTION_LIST_MATCHING = "list_matching_attributes";
public static final String ACTION_LIST_NOT_MATCHING = "list_not_matching_attributes";
public static final String ACTION_LIST_LIMITED = "list_limited_attributes";
public static final String ACTION_HELP = "help";
public static final HashSet<String> ACTIONS = new HashSet<String>(Arrays.asList(ACTION_COLLECT, ACTION_LIST_EVERYTHING,
ACTION_LIST_COLLECTED, ACTION_LIST_MATCHING, ACTION_LIST_NOT_MATCHING, ACTION_LIST_LIMITED, ACTION_HELP));
ACTION_LIST_COLLECTED, ACTION_LIST_MATCHING, ACTION_LIST_NOT_MATCHING, ACTION_LIST_LIMITED, ACTION_HELP, ACTION_LIST_JVMS));

@Parameter(names = {"--help", "-h"},
description = "Display this help page",
Expand Down Expand Up @@ -81,7 +82,7 @@ class AppConfig {

@Parameter(description = "Action to take, should be in [help, collect, " +
"list_everything, list_collected_attributes, list_matching_attributes, " +
"list_not_matching_attributes, list_limited_attributes]",
"list_not_matching_attributes, list_limited_attributes, list_jvms]",
Copy link
Member

Choose a reason for hiding this comment

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

Same as below 😄 : https://github.com/DataDog/jmxfetch/blob/master/src/test/java/org/datadog/jmxfetch/TestParsingJCommander.java#L330-L333

required = true)
private List<String> action = null;

Expand Down
10 changes: 9 additions & 1 deletion src/main/java/org/datadog/jmxfetch/AttachApiConnection.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,10 @@

import java.io.File;
import java.io.IOException;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.LinkedHashMap;
import java.util.List;

import javax.management.remote.JMXServiceURL;

Expand Down Expand Up @@ -35,6 +37,7 @@ private JMXServiceURL getAddress(LinkedHashMap<String, Object> connectionParams)
private String getJMXUrlForProcessRegex(String processRegex) throws com.sun.tools.attach.AttachNotSupportedException, IOException {
for (com.sun.tools.attach.VirtualMachineDescriptor vmd : com.sun.tools.attach.VirtualMachine.list()) {
if (vmd.displayName().matches(processRegex)) {
LOGGER.info("Matched JVM '" + vmd.displayName() + "' against regex '" + processRegex + "'");
com.sun.tools.attach.VirtualMachine vm = com.sun.tools.attach.VirtualMachine.attach(vmd);
String connectorAddress = vm.getAgentProperties().getProperty(CONNECTOR_ADDRESS);
//If jmx agent is not running in VM, load it and return the connector url
Expand All @@ -49,7 +52,12 @@ private String getJMXUrlForProcessRegex(String processRegex) throws com.sun.tool
return connectorAddress;
}
}
throw new IOException("Cannot find JVM matching regex: " + processRegex);

List<String> jvms = new ArrayList<String>();
for (com.sun.tools.attach.VirtualMachineDescriptor vmd : com.sun.tools.attach.VirtualMachine.list()) {
jvms.add( vmd.displayName() );
Copy link
Member

Choose a reason for hiding this comment

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

This could be done within the previous loop for loop, c.f. https://github.com/DataDog/jmxfetch/pull/100/files#diff-2e06a50bff42ed16d309e1802b58faecR38

}
throw new IOException("Cannot find JVM matching regex: '" + processRegex + "'; available JVMs (for this user account): " + jvms );
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this test requires the same update https://github.com/DataDog/jmxfetch/blob/master/src/test/java/org/datadog/jmxfetch/TestServiceChecks.java#L116

Copy link
Member

Choose a reason for hiding this comment

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

This log line is directly forwarded to the dd-agent info command, i.e.

    jmx
    ---
      - instance #foobar [ERROR]: "Cannot connect to instance process_regex: test Cannot find JVM matching regex: 'test'; available JVMs (for this user account): [org.datadog.jmxfetch.App --check jmx.yaml --check_period 15000 --conf_directory /etc/dd-agent/conf.d --log_level INFO --log_location /var/log/datadog/jmxfetch.log --reporter statsd:localhost:8125 --status_location /opt/datadog-agent/run/jmx_status.yaml collect]" collected 0 metrics
      - Collected 0 metrics, 0 events & 0 service checks

I think it's difficult to understand what it means when being agnostic to the code. What do you think about keeping the old message ? Alternatively the message could invite the user to run the list_jvms command to list all available JVMs.

}

private void loadJMXAgent(com.sun.tools.attach.VirtualMachine vm) throws IOException {
Expand Down