Skip to content

Commit 63361cc

Browse files
David Grievejeanbisutti
David Grieve
and
jeanbisutti
authored
jfr-connection: fixes for using diagnostic command to start a recording (#1352)
Co-authored-by: Jean Bisutti <[email protected]>
1 parent 6e7673b commit 63361cc

File tree

6 files changed

+156
-34
lines changed

6 files changed

+156
-34
lines changed

‎jfr-connection/build.gradle.kts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,4 +9,5 @@ otelJava.moduleName.set("io.opentelemetry.contrib.jfr.connection")
99
dependencies {
1010
testImplementation("org.openjdk.jmc:common:8.3.1")
1111
testImplementation("org.openjdk.jmc:flightrecorder:8.3.1")
12+
testImplementation("org.mockito:mockito-inline")
1213
}

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

Lines changed: 32 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,8 @@
77

88
import java.io.IOException;
99
import java.io.InputStream;
10+
import java.lang.management.ManagementFactory;
11+
import java.lang.management.RuntimeMXBean;
1012
import java.time.Instant;
1113
import java.util.ArrayList;
1214
import java.util.Collections;
@@ -33,7 +35,7 @@
3335
final class FlightRecorderDiagnosticCommandConnection implements FlightRecorderConnection {
3436
private static final String DIAGNOSTIC_COMMAND_OBJECT_NAME =
3537
"com.sun.management:type=DiagnosticCommand";
36-
private static final String JFR_START_REGEX = "Started recording (.+?)\\. .*";
38+
private static final String JFR_START_REGEX = "Started recording (\\d+?)\\.";
3739
private static final Pattern JFR_START_PATTERN = Pattern.compile(JFR_START_REGEX, Pattern.DOTALL);
3840

3941
// All JFR commands take String[] parameters
@@ -56,7 +58,11 @@ static FlightRecorderConnection connect(MBeanServerConnection mBeanServerConnect
5658
ObjectInstance objectInstance =
5759
mBeanServerConnection.getObjectInstance(new ObjectName(DIAGNOSTIC_COMMAND_OBJECT_NAME));
5860
ObjectName objectName = objectInstance.getObjectName();
59-
assertCommercialFeaturesUnlocked(mBeanServerConnection, objectName);
61+
62+
if (jdkHasUnlockCommercialFeatures(mBeanServerConnection)) {
63+
assertCommercialFeaturesUnlocked(mBeanServerConnection, objectName);
64+
}
65+
6066
return new FlightRecorderDiagnosticCommandConnection(
6167
mBeanServerConnection, objectInstance.getObjectName());
6268
} catch (MalformedObjectNameException e) {
@@ -179,7 +185,7 @@ public void stopRecording(long id) throws JfrConnectionException {
179185
@Override
180186
public void dumpRecording(long id, String outputFile) throws IOException, JfrConnectionException {
181187
try {
182-
Object[] params = mkParams("filename=" + outputFile, "recording=" + id, "compress=true");
188+
Object[] params = mkParams("filename=" + outputFile, "name=" + id);
183189
mBeanServerConnection.invoke(objectName, "jfrDump", params, signature);
184190
} catch (InstanceNotFoundException | MBeanException | ReflectionException e) {
185191
throw JfrConnectionException.canonicalJfrConnectionException(getClass(), "dumpRecording", e);
@@ -210,26 +216,41 @@ public void closeRecording(long id) {
210216
throw new UnsupportedOperationException("closeRecording not supported on Java 8");
211217
}
212218

219+
// Do this check separate from assertCommercialFeatures because reliance
220+
// on System properties makes it difficult to test.
221+
static boolean jdkHasUnlockCommercialFeatures(MBeanServerConnection mBeanServerConnection) {
222+
try {
223+
RuntimeMXBean runtimeMxBean =
224+
ManagementFactory.getPlatformMXBean(mBeanServerConnection, RuntimeMXBean.class);
225+
String javaVmVendor = runtimeMxBean.getVmVendor();
226+
String javaVersion = runtimeMxBean.getVmVersion();
227+
return javaVmVendor.contains("Oracle Corporation")
228+
&& javaVersion.matches("(?:^1\\.8|9|10).*");
229+
} catch (IOException e) {
230+
return false;
231+
}
232+
}
233+
213234
// visible for testing
214235
static void assertCommercialFeaturesUnlocked(
215236
MBeanServerConnection mBeanServerConnection, ObjectName objectName)
216237
throws IOException, JfrConnectionException {
238+
217239
try {
218240
Object unlockedMessage =
219241
mBeanServerConnection.invoke(objectName, "vmCheckCommercialFeatures", null, null);
220242
if (unlockedMessage instanceof String) {
221243
boolean unlocked = ((String) unlockedMessage).contains("unlocked");
222244
if (!unlocked) {
223-
throw new UnsupportedOperationException(
224-
"Unlocking commercial features may be required. This must be explicitly enabled by adding -XX:+UnlockCommercialFeatures");
245+
throw JfrConnectionException.canonicalJfrConnectionException(
246+
FlightRecorderDiagnosticCommandConnection.class,
247+
"assertCommercialFeaturesUnlocked",
248+
new UnsupportedOperationException(
249+
"Unlocking commercial features may be required. This must be explicitly enabled by adding -XX:+UnlockCommercialFeatures"));
225250
}
226251
}
227-
} catch (InstanceNotFoundException
228-
| MBeanException
229-
| ReflectionException
230-
| UnsupportedOperationException e) {
231-
throw JfrConnectionException.canonicalJfrConnectionException(
232-
FlightRecorderDiagnosticCommandConnection.class, "assertCommercialFeaturesUnlocked", e);
252+
} catch (InstanceNotFoundException | MBeanException | ReflectionException ignored) {
253+
// If the MBean doesn't have the vmCheckCommercialFeatures method, then we can't check it.
233254
}
234255
}
235256

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -207,7 +207,7 @@ public Builder disk(String disk) {
207207
* <li>"d" (days)
208208
* </ul>
209209
*
210-
* For example, {@code "2 h"}, {@code "3 m"}.
210+
* For example, {@code "2h"}, {@code "3m"}.
211211
*
212212
* <p>If the value is {@code null}, {@code duration} will be set to the default value, which is
213213
* "no limit".
@@ -388,7 +388,7 @@ private static String validateDuration(Option option, String durationString) {
388388
case "m":
389389
case "h":
390390
case "d":
391-
return Long.toString(value) + " " + units;
391+
return value + units;
392392
default:
393393
// fall through
394394
}

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

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,14 +10,67 @@
1010
import static org.mockito.ArgumentMatchers.any;
1111
import static org.mockito.ArgumentMatchers.anyString;
1212
import static org.mockito.Mockito.mock;
13+
import static org.mockito.Mockito.mockStatic;
1314
import static org.mockito.Mockito.when;
1415

16+
import com.google.errorprone.annotations.Keep;
17+
import java.lang.management.ManagementFactory;
18+
import java.lang.management.RuntimeMXBean;
19+
import java.util.stream.Stream;
1520
import javax.management.MBeanServerConnection;
1621
import javax.management.ObjectName;
1722
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;
1829

1930
class FlightRecorderDiagnosticCommandConnectionTest {
2031

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+
2174
@Test
2275
void assertCommercialFeaturesUnlocked() throws Exception {
2376
ObjectName objectName = mock(ObjectName.class);

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

Lines changed: 21 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -46,18 +46,18 @@ void testGetNameDefault() {
4646
@Keep
4747
static Stream<Arguments> testGetMaxAge() {
4848
return Stream.of(
49-
Arguments.of("3 ns", "3 ns"),
50-
Arguments.of("3 us", "3 us"),
51-
Arguments.of("3 ms", "3 ms"),
52-
Arguments.of("3 s", "3 s"),
53-
Arguments.of("3 m", "3 m"),
54-
Arguments.of("3 h", "3 h"),
55-
Arguments.of("3 h", "3 h"),
56-
Arguments.of("+3 d", "3 d"),
57-
Arguments.of("3ms", "3 ms"),
49+
Arguments.of("3 ns", "3ns"),
50+
Arguments.of("3 us", "3us"),
51+
Arguments.of("3 ms", "3ms"),
52+
Arguments.of("3 s", "3s"),
53+
Arguments.of("3 m", "3m"),
54+
Arguments.of("3 h", "3h"),
55+
Arguments.of("3 h", "3h"),
56+
Arguments.of("+3 d", "3d"),
57+
Arguments.of("3ms", "3ms"),
5858
Arguments.of("0", "0"),
5959
Arguments.of("", "0"),
60-
Arguments.of((String) null, "0"));
60+
Arguments.of(null, "0"));
6161
}
6262

6363
@ParameterizedTest
@@ -209,15 +209,15 @@ void testGetDiskBadValue() {
209209
@Keep
210210
private static Stream<Arguments> testGetDuration() {
211211
return Stream.of(
212-
Arguments.of("3 ns", "3 ns"),
213-
Arguments.of("3 us", "3 us"),
214-
Arguments.of("3 ms", "3 ms"),
215-
Arguments.of("3 s", "3 s"),
216-
Arguments.of("3 m", "3 m"),
217-
Arguments.of("3 h", "3 h"),
218-
Arguments.of("3 h", "3 h"),
219-
Arguments.of("+3 d", "3 d"),
220-
Arguments.of("3ms", "3 ms"),
212+
Arguments.of("3 ns", "3ns"),
213+
Arguments.of("3 us", "3us"),
214+
Arguments.of("3 ms", "3ms"),
215+
Arguments.of("3 s", "3s"),
216+
Arguments.of("3 m", "3m"),
217+
Arguments.of("3 h", "3h"),
218+
Arguments.of("3 h", "3h"),
219+
Arguments.of("+3 d", "3d"),
220+
Arguments.of("3ms", "3ms"),
221221
Arguments.of("0", "0"),
222222
Arguments.of("", "0"),
223223
Arguments.of(null, "0"));
@@ -263,12 +263,12 @@ void testGetDurationNegative(String badValue) {
263263
void testGetRecordingOptions() {
264264
Map<String, String> expected = new HashMap<>();
265265
expected.put("name", "test");
266-
expected.put("maxAge", "3 m");
266+
expected.put("maxAge", "3m");
267267
expected.put("maxSize", "1048576");
268268
expected.put("dumpOnExit", "true");
269269
expected.put("destination", "test.jfr");
270270
expected.put("disk", "true");
271-
expected.put("duration", "120 s");
271+
expected.put("duration", "120s");
272272
RecordingOptions opts =
273273
new RecordingOptions.Builder()
274274
.name("test")

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

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,7 @@
4848
import org.junit.jupiter.params.aggregator.ArgumentsAggregator;
4949
import org.junit.jupiter.params.provider.Arguments;
5050
import org.junit.jupiter.params.provider.MethodSource;
51+
import org.junit.jupiter.params.provider.ValueSource;
5152
import org.junit.platform.commons.logging.Logger;
5253
import org.junit.platform.commons.logging.LoggerFactory;
5354

@@ -329,6 +330,13 @@ void assertRecordingOptionsAreSetInFlightRecorderMXBean(
329330
String getter =
330331
"get" + key.substring(0, 1).toUpperCase(Locale.ROOT) + key.substring(1);
331332
String expected = (String) compositeData.get("value");
333+
334+
// Special case for duration values. The FlightRecorderMXBean wants "<number><unit>"
335+
// but returns "<number> <unit>", so we need to normalize the expected value.
336+
if (expected != null && expected.matches("([-+]?\\d+)\\s*(\\w*)")) {
337+
expected = expected.replaceAll("\\s", "");
338+
}
339+
332340
try {
333341
Method method = RecordingOptions.class.getMethod(getter);
334342
String actual = (String) method.invoke(recordingOptions);
@@ -505,4 +513,43 @@ void assertRecordingCloneState() {
505513
fail("Error thrown by MBean server or FlightRecorderMXBean: ", badBean);
506514
}
507515
}
516+
517+
@ParameterizedTest
518+
@ValueSource(strings = {"5 s", "5m"})
519+
void assertDiagnosticCommandCanRecordWithDuration(String duration) {
520+
// Issue #1338
521+
MBeanServerConnection mBeanServer = ManagementFactory.getPlatformMBeanServer();
522+
try {
523+
FlightRecorderConnection connection =
524+
FlightRecorderDiagnosticCommandConnection.connect(mBeanServer);
525+
assertCanRecord(connection, duration);
526+
} catch (IOException | JfrConnectionException e) {
527+
fail(e);
528+
}
529+
}
530+
531+
@ParameterizedTest
532+
@ValueSource(strings = {"5 s", "5m"})
533+
void assertMbeanCanRecordWithDuration(String duration) {
534+
// Ensure fix for issue #1338 doesn't regress MBean recording
535+
MBeanServerConnection mBeanServer = ManagementFactory.getPlatformMBeanServer();
536+
try {
537+
FlightRecorderConnection connection = FlightRecorderConnection.connect(mBeanServer);
538+
assertCanRecord(connection, duration);
539+
} catch (IOException | JfrConnectionException e) {
540+
fail(e);
541+
}
542+
}
543+
544+
private static void assertCanRecord(FlightRecorderConnection connection, String duration)
545+
throws JfrConnectionException {
546+
RecordingOptions recordingOptions = new RecordingOptions.Builder().duration(duration).build();
547+
RecordingConfiguration recordingConfiguration = RecordingConfiguration.PROFILE_CONFIGURATION;
548+
try (Recording recording = connection.newRecording(recordingOptions, recordingConfiguration)) {
549+
recording.start();
550+
recording.stop();
551+
} catch (IOException e) {
552+
// Recording is autoclose
553+
}
554+
}
508555
}

0 commit comments

Comments
 (0)