Skip to content

Commit 9e68011

Browse files
dsgrievedsgrieve
authored
jfr-connection: jfr-connection: wrong parameter sent to JFR DiagnosticCommand (#1492)
1 parent dc66266 commit 9e68011

File tree

2 files changed

+98
-96
lines changed

2 files changed

+98
-96
lines changed

‎jfr-connection/src/main/java/io/opentelemetry/contrib/jfr/connection/FlightRecorderDiagnosticCommandConnection.java‎

Lines changed: 62 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -7,18 +7,21 @@
77

88
import java.io.IOException;
99
import java.io.InputStream;
10-
import java.lang.management.ManagementFactory;
11-
import java.lang.management.RuntimeMXBean;
1210
import java.time.Instant;
1311
import java.util.ArrayList;
12+
import java.util.Arrays;
1413
import java.util.Collections;
1514
import java.util.List;
1615
import java.util.Objects;
16+
import java.util.Optional;
1717
import java.util.regex.Matcher;
1818
import java.util.regex.Pattern;
1919
import java.util.stream.Collectors;
2020
import javax.management.InstanceNotFoundException;
21+
import javax.management.IntrospectionException;
2122
import javax.management.MBeanException;
23+
import javax.management.MBeanInfo;
24+
import javax.management.MBeanOperationInfo;
2225
import javax.management.MBeanServerConnection;
2326
import javax.management.MalformedObjectNameException;
2427
import javax.management.ObjectInstance;
@@ -37,6 +40,8 @@ final class FlightRecorderDiagnosticCommandConnection implements FlightRecorderC
3740
"com.sun.management:type=DiagnosticCommand";
3841
private static final String JFR_START_REGEX = "Started recording (\\d+?)\\.";
3942
private static final Pattern JFR_START_PATTERN = Pattern.compile(JFR_START_REGEX, Pattern.DOTALL);
43+
private static final String JFR_CHECK_REGEX = "(?:recording|name)=(\\d+)";
44+
private static final Pattern JFR_CHECK_PATTERN = Pattern.compile(JFR_CHECK_REGEX, Pattern.DOTALL);
4045

4146
// All JFR commands take String[] parameters
4247
private static final String[] signature = new String[] {"[Ljava.lang.String;"};
@@ -59,9 +64,7 @@ static FlightRecorderConnection connect(MBeanServerConnection mBeanServerConnect
5964
mBeanServerConnection.getObjectInstance(new ObjectName(DIAGNOSTIC_COMMAND_OBJECT_NAME));
6065
ObjectName objectName = objectInstance.getObjectName();
6166

62-
if (jdkHasUnlockCommercialFeatures(mBeanServerConnection)) {
63-
assertCommercialFeaturesUnlocked(mBeanServerConnection, objectName);
64-
}
67+
assertCommercialFeaturesUnlocked(mBeanServerConnection, objectName);
6568

6669
return new FlightRecorderDiagnosticCommandConnection(
6770
mBeanServerConnection, objectInstance.getObjectName());
@@ -123,21 +126,22 @@ public long startRecording(
123126
Object[] params = formOptions(recordingOptions, recordingConfiguration);
124127

125128
// jfrStart returns "Started recording 2." and some more stuff, but all we care about is the
126-
// name of the recording.
129+
// id of the recording.
130+
String jfrStart;
127131
try {
128-
String jfrStart =
129-
(String) mBeanServerConnection.invoke(objectName, "jfrStart", params, signature);
130-
String name;
132+
jfrStart = (String) mBeanServerConnection.invoke(objectName, "jfrStart", params, signature);
131133
Matcher matcher = JFR_START_PATTERN.matcher(jfrStart);
132134
if (matcher.find()) {
133-
name = matcher.group(1);
134-
return Long.parseLong(name);
135+
String id = matcher.group(1);
136+
return Long.parseLong(id);
135137
}
136138
} catch (InstanceNotFoundException | ReflectionException | MBeanException e) {
137139
throw JfrConnectionException.canonicalJfrConnectionException(getClass(), "startRecording", e);
138140
}
139141
throw JfrConnectionException.canonicalJfrConnectionException(
140-
getClass(), "startRecording", new IllegalStateException("Failed to parse jfrStart output"));
142+
getClass(),
143+
"startRecording",
144+
new IllegalStateException("Failed to parse: '" + jfrStart + "'"));
141145
}
142146

143147
private static Object[] formOptions(
@@ -156,10 +160,33 @@ private static Object[] formOptions(
156160
return mkParamsArray(params);
157161
}
158162

163+
//
164+
// Whether to use the 'name' or 'recording' parameter depends on the JVM.
165+
// Use JFR.check to determine which one to use.
166+
//
167+
private String getRecordingParam(long recordingId) throws JfrConnectionException, IOException {
168+
String jfrCheck;
169+
try {
170+
Object[] params = new Object[] {new String[] {}};
171+
jfrCheck = (String) mBeanServerConnection.invoke(objectName, "jfrCheck", params, signature);
172+
Matcher matcher = JFR_CHECK_PATTERN.matcher(jfrCheck);
173+
while (matcher.find()) {
174+
String id = matcher.group(1);
175+
if (id.equals(Long.toString(recordingId))) {
176+
return matcher.group(0);
177+
}
178+
}
179+
} catch (InstanceNotFoundException | MBeanException | ReflectionException e) {
180+
throw JfrConnectionException.canonicalJfrConnectionException(getClass(), "jfrCheck", e);
181+
}
182+
throw JfrConnectionException.canonicalJfrConnectionException(
183+
getClass(), "jfrCheck", new IllegalStateException("Failed to parse: '" + jfrCheck + "'"));
184+
}
185+
159186
@Override
160187
public void stopRecording(long id) throws JfrConnectionException {
161188
try {
162-
Object[] params = mkParams("name=" + id);
189+
Object[] params = mkParams(getRecordingParam(id));
163190
mBeanServerConnection.invoke(objectName, "jfrStop", params, signature);
164191
} catch (InstanceNotFoundException | MBeanException | ReflectionException | IOException e) {
165192
throw JfrConnectionException.canonicalJfrConnectionException(getClass(), "stopRecording", e);
@@ -169,7 +196,7 @@ public void stopRecording(long id) throws JfrConnectionException {
169196
@Override
170197
public void dumpRecording(long id, String outputFile) throws IOException, JfrConnectionException {
171198
try {
172-
Object[] params = mkParams("filename=" + outputFile, "name=" + id);
199+
Object[] params = mkParams("filename=" + outputFile, getRecordingParam(id));
173200
mBeanServerConnection.invoke(objectName, "jfrDump", params, signature);
174201
} catch (InstanceNotFoundException | MBeanException | ReflectionException e) {
175202
throw JfrConnectionException.canonicalJfrConnectionException(getClass(), "dumpRecording", e);
@@ -197,41 +224,34 @@ public void closeRecording(long id) {
197224
"closeRecording not available through the DiagnosticCommand connection");
198225
}
199226

200-
// Do this check separate from assertCommercialFeatures because reliance
201-
// on System properties makes it difficult to test.
202-
static boolean jdkHasUnlockCommercialFeatures(MBeanServerConnection mBeanServerConnection) {
203-
try {
204-
RuntimeMXBean runtimeMxBean =
205-
ManagementFactory.getPlatformMXBean(mBeanServerConnection, RuntimeMXBean.class);
206-
String javaVmVendor = runtimeMxBean.getVmVendor();
207-
String javaVersion = runtimeMxBean.getVmVersion();
208-
return javaVmVendor.contains("Oracle Corporation")
209-
&& javaVersion.matches("(?:^1\\.8|9|10).*");
210-
} catch (IOException e) {
211-
return false;
212-
}
213-
}
214-
215227
// visible for testing
216228
static void assertCommercialFeaturesUnlocked(
217229
MBeanServerConnection mBeanServerConnection, ObjectName objectName)
218230
throws IOException, JfrConnectionException {
219231

220232
try {
221-
Object unlockedMessage =
222-
mBeanServerConnection.invoke(objectName, "vmCheckCommercialFeatures", null, null);
223-
if (unlockedMessage instanceof String) {
224-
boolean unlocked = ((String) unlockedMessage).contains("unlocked");
225-
if (!unlocked) {
226-
throw JfrConnectionException.canonicalJfrConnectionException(
227-
FlightRecorderDiagnosticCommandConnection.class,
228-
"assertCommercialFeaturesUnlocked",
229-
new UnsupportedOperationException(
230-
"Unlocking commercial features may be required. This must be explicitly enabled by adding -XX:+UnlockCommercialFeatures"));
231-
}
233+
Object[] params = new Object[] {new String[] {}};
234+
MBeanInfo mBeanInfo = mBeanServerConnection.getMBeanInfo(objectName);
235+
if (mBeanInfo == null) {
236+
throw JfrConnectionException.canonicalJfrConnectionException(
237+
FlightRecorderDiagnosticCommandConnection.class,
238+
"assertCommercialFeaturesUnlocked",
239+
new NullPointerException("Could not get MBeanInfo for " + objectName));
240+
}
241+
Optional<MBeanOperationInfo> operation =
242+
Arrays.stream(mBeanInfo.getOperations())
243+
.filter(it -> "vmUnlockCommercialFeatures".equals(it.getName()))
244+
.findFirst();
245+
246+
if (operation.isPresent()) {
247+
mBeanServerConnection.invoke(objectName, "vmUnlockCommercialFeatures", params, signature);
232248
}
233-
} catch (InstanceNotFoundException | MBeanException | ReflectionException ignored) {
234-
// If the MBean doesn't have the vmCheckCommercialFeatures method, then we can't check it.
249+
} catch (InstanceNotFoundException
250+
| IntrospectionException
251+
| MBeanException
252+
| ReflectionException e) {
253+
throw JfrConnectionException.canonicalJfrConnectionException(
254+
FlightRecorderDiagnosticCommandConnection.class, "assertCommercialFeaturesUnlocked", e);
235255
}
236256
}
237257

‎jfr-connection/src/test/java/io/opentelemetry/contrib/jfr/connection/FlightRecorderDiagnosticCommandConnectionTest.java‎

Lines changed: 36 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -7,74 +7,26 @@
77

88
import static org.junit.jupiter.api.Assertions.assertEquals;
99
import static org.junit.jupiter.api.Assertions.assertThrows;
10+
import static org.junit.jupiter.api.Assertions.fail;
1011
import static org.mockito.ArgumentMatchers.any;
1112
import static org.mockito.ArgumentMatchers.anyString;
1213
import static org.mockito.Mockito.mock;
13-
import static org.mockito.Mockito.mockStatic;
1414
import static org.mockito.Mockito.when;
1515

16-
import com.google.errorprone.annotations.Keep;
1716
import java.lang.management.ManagementFactory;
18-
import java.lang.management.RuntimeMXBean;
19-
import java.util.stream.Stream;
17+
import java.nio.file.Files;
18+
import java.nio.file.Path;
19+
import javax.management.MBeanServer;
2020
import javax.management.MBeanServerConnection;
2121
import javax.management.ObjectName;
2222
import org.junit.jupiter.api.Test;
23-
import org.junit.jupiter.params.ParameterizedTest;
24-
import org.junit.jupiter.params.provider.Arguments;
25-
import org.junit.jupiter.params.provider.MethodSource;
26-
import org.mockito.MockedStatic;
27-
import org.mockito.invocation.InvocationOnMock;
28-
import org.mockito.stubbing.Answer;
2923

3024
class FlightRecorderDiagnosticCommandConnectionTest {
3125

32-
@Keep
33-
static Stream<Arguments> assertJdkHasUnlockCommercialFeatures() {
34-
return Stream.of(
35-
Arguments.of("Oracle Corporation", "1.8.0_401", true),
36-
Arguments.of("AdoptOpenJDK", "1.8.0_282", false),
37-
Arguments.of("Oracle Corporation", "10.0.2", true),
38-
Arguments.of("Oracle Corporation", "9.0.4", true),
39-
Arguments.of("Oracle Corporation", "11.0.22", false),
40-
Arguments.of("Microsoft", "11.0.13", false),
41-
Arguments.of("Microsoft", "17.0.3", false),
42-
Arguments.of("Oracle Corporation", "21.0.3", false));
43-
}
44-
45-
@ParameterizedTest
46-
@MethodSource
47-
void assertJdkHasUnlockCommercialFeatures(String vmVendor, String vmVersion, boolean expected)
48-
throws Exception {
49-
50-
MBeanServerConnection mBeanServerConnection = mock(MBeanServerConnection.class);
51-
52-
try (MockedStatic<ManagementFactory> mockedStatic = mockStatic(ManagementFactory.class)) {
53-
mockedStatic
54-
.when(
55-
() -> ManagementFactory.getPlatformMXBean(mBeanServerConnection, RuntimeMXBean.class))
56-
.thenAnswer(
57-
new Answer<RuntimeMXBean>() {
58-
@Override
59-
public RuntimeMXBean answer(InvocationOnMock invocation) {
60-
RuntimeMXBean mockedRuntimeMxBean = mock(RuntimeMXBean.class);
61-
when(mockedRuntimeMxBean.getVmVendor()).thenReturn(vmVendor);
62-
when(mockedRuntimeMxBean.getVmVersion()).thenReturn(vmVersion);
63-
return mockedRuntimeMxBean;
64-
}
65-
});
66-
67-
boolean actual =
68-
FlightRecorderDiagnosticCommandConnection.jdkHasUnlockCommercialFeatures(
69-
mBeanServerConnection);
70-
assertEquals(expected, actual, "Expected " + expected + " for " + vmVendor + " " + vmVersion);
71-
}
72-
}
73-
7426
@Test
7527
void assertCommercialFeaturesUnlocked() throws Exception {
76-
ObjectName objectName = mock(ObjectName.class);
77-
MBeanServerConnection mBeanServerConnection = mockMbeanServer(objectName, "unlocked");
28+
MBeanServer mBeanServerConnection = ManagementFactory.getPlatformMBeanServer();
29+
ObjectName objectName = new ObjectName("com.sun.management:type=DiagnosticCommand");
7830
FlightRecorderDiagnosticCommandConnection.assertCommercialFeaturesUnlocked(
7931
mBeanServerConnection, objectName);
8032
}
@@ -124,6 +76,36 @@ void startRecordingParsesIdCorrectly() throws Exception {
12476
assertEquals(id, 99);
12577
}
12678

79+
@Test
80+
void endToEndTest() throws Exception {
81+
82+
MBeanServerConnection mBeanServer = ManagementFactory.getPlatformMBeanServer();
83+
FlightRecorderConnection flightRecorderConnection =
84+
FlightRecorderDiagnosticCommandConnection.connect(mBeanServer);
85+
RecordingOptions recordingOptions =
86+
new RecordingOptions.Builder().disk("true").duration("5s").build();
87+
RecordingConfiguration recordingConfiguration = RecordingConfiguration.PROFILE_CONFIGURATION;
88+
Path tempFile = Files.createTempFile("recording", ".jfr");
89+
90+
try (Recording recording =
91+
flightRecorderConnection.newRecording(recordingOptions, recordingConfiguration)) {
92+
93+
recording.start();
94+
try {
95+
Thread.sleep(1000);
96+
} catch (InterruptedException e) {
97+
Thread.currentThread().interrupt();
98+
}
99+
recording.dump(tempFile.toString());
100+
recording.stop();
101+
} finally {
102+
if (!Files.exists(tempFile)) {
103+
fail("Recording file not found");
104+
}
105+
Files.deleteIfExists(tempFile);
106+
}
107+
}
108+
127109
MBeanServerConnection mockMbeanServer(
128110
ObjectName objectName, String vmCheckCommercialFeaturesResponse) throws Exception {
129111
MBeanServerConnection mBeanServerConnection = mock(MBeanServerConnection.class);

0 commit comments

Comments
 (0)