Apply code review guides

This commit is contained in:
Trask Stalnaker 2025-10-26 14:42:06 -07:00
parent a63bba4cda
commit b812efcc55
13 changed files with 58 additions and 60 deletions

View File

@ -7,9 +7,9 @@ package io.opentelemetry.contrib.awsxray;
import static java.util.Objects.requireNonNull;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
import static org.assertj.core.api.AssertionsForClassTypes.assertThatCode;
import static org.awaitility.Awaitility.await;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.mockito.Mockito.mock;
import com.google.common.io.ByteStreams;
@ -179,16 +179,15 @@ class AwsXrayRemoteSamplerTest {
try (AwsXrayRemoteSampler sampler = AwsXrayRemoteSampler.newBuilder(Resource.empty()).build()) {
// Setting span exporter should only work once
sampler.setSpanExporter(mock(SpanExporter.class));
assertThrows(
IllegalStateException.class, () -> sampler.setSpanExporter(mock(SpanExporter.class)));
assertThatThrownBy(() -> sampler.setSpanExporter(mock(SpanExporter.class)))
.isInstanceOf(IllegalStateException.class);
}
}
@Test
void adaptSamplingWithoutSpanExporter() {
assertThrows(
IllegalStateException.class,
() -> sampler.adaptSampling(mock(ReadableSpan.class), mock(SpanData.class)));
assertThatThrownBy(() -> sampler.adaptSampling(mock(ReadableSpan.class), mock(SpanData.class)))
.isInstanceOf(IllegalStateException.class);
}
@Test
@ -224,7 +223,8 @@ class AwsXrayRemoteSamplerTest {
AwsXrayAdaptiveSamplingConfig config =
AwsXrayAdaptiveSamplingConfig.builder().setVersion(1.0).build();
sampler.setAdaptiveSamplingConfig(config);
assertThrows(IllegalStateException.class, () -> sampler.setAdaptiveSamplingConfig(config));
assertThatThrownBy(() -> sampler.setAdaptiveSamplingConfig(config))
.isInstanceOf(IllegalStateException.class);
}
}

View File

@ -7,7 +7,7 @@ package io.opentelemetry.contrib.awsxray;
import static io.opentelemetry.semconv.HttpAttributes.HTTP_RESPONSE_STATUS_CODE;
import static org.assertj.core.api.Assertions.assertThat;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;
@ -710,7 +710,8 @@ class XrayRulesSamplerTest {
AwsXrayAdaptiveSamplingConfig config =
AwsXrayAdaptiveSamplingConfig.builder().setVersion(1.0).build();
sampler.setAdaptiveSamplingConfig(config);
assertThrows(IllegalStateException.class, () -> sampler.setAdaptiveSamplingConfig(config));
assertThatThrownBy(() -> sampler.setAdaptiveSamplingConfig(config))
.isInstanceOf(IllegalStateException.class);
}
@Test

View File

@ -5,9 +5,8 @@
package io.opentelemetry.contrib.sampler.cel;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertNotEquals;
import dev.cel.common.CelAbstractSyntaxTree;
import dev.cel.common.CelValidationException;
@ -35,7 +34,7 @@ final class CelBasedSamplingExpressionTest {
CelBasedSamplingExpression celExpression =
new CelBasedSamplingExpression(ast, Sampler.alwaysOn());
String expected = "CelBasedSamplingExpression{expression='1 == 1', delegate=AlwaysOnSampler}";
assertEquals(expected, celExpression.toString());
assertThat(celExpression.toString()).isEqualTo(expected);
}
@Test
@ -45,28 +44,28 @@ final class CelBasedSamplingExpressionTest {
CelCompilerFactory.standardCelCompilerBuilder().build().compile("1 == 1").getAst(),
Sampler.alwaysOn());
assertEquals(celExpressionOneEqualsOne1, celExpressionOneEqualsOne1);
assertNotEquals(celExpressionOneEqualsOne1, null);
assertThat(celExpressionOneEqualsOne1).isEqualTo(celExpressionOneEqualsOne1);
assertThat(celExpressionOneEqualsOne1).isNotEqualTo(null);
CelBasedSamplingExpression celExpressionOneEqualsOne2 =
new CelBasedSamplingExpression(
CelCompilerFactory.standardCelCompilerBuilder().build().compile("1 == 1").getAst(),
Sampler.alwaysOn());
assertEquals(celExpressionOneEqualsOne1, celExpressionOneEqualsOne2);
assertThat(celExpressionOneEqualsOne1).isEqualTo(celExpressionOneEqualsOne2);
CelBasedSamplingExpression celExpressionTwoEqualsTwo =
new CelBasedSamplingExpression(
CelCompilerFactory.standardCelCompilerBuilder().build().compile("2 == 2").getAst(),
Sampler.alwaysOn());
assertNotEquals(celExpressionOneEqualsOne1, celExpressionTwoEqualsTwo);
assertThat(celExpressionOneEqualsOne1).isNotEqualTo(celExpressionTwoEqualsTwo);
CelBasedSamplingExpression celExpressionOneEqualsOneSamplerOff =
new CelBasedSamplingExpression(
CelCompilerFactory.standardCelCompilerBuilder().build().compile("1 == 1").getAst(),
Sampler.alwaysOff());
assertNotEquals(celExpressionOneEqualsOne1, celExpressionOneEqualsOneSamplerOff);
assertThat(celExpressionOneEqualsOne1).isNotEqualTo(celExpressionOneEqualsOneSamplerOff);
}
@Test
@ -78,13 +77,13 @@ final class CelBasedSamplingExpressionTest {
int expectedHashCode1 = celExpression1.hashCode();
int expectedHashCode2 = celExpression1.hashCode();
assertEquals(expectedHashCode1, expectedHashCode2);
assertThat(expectedHashCode1).isEqualTo(expectedHashCode2);
CelBasedSamplingExpression celExpression2 =
new CelBasedSamplingExpression(
CelCompilerFactory.standardCelCompilerBuilder().build().compile("1 == 1").getAst(),
Sampler.alwaysOn());
assertEquals(expectedHashCode1, celExpression2.hashCode());
assertThat(expectedHashCode1).isEqualTo(celExpression2.hashCode());
}
}

View File

@ -13,9 +13,7 @@ import static io.opentelemetry.contrib.disk.buffering.internal.storage.TestData.
import static io.opentelemetry.contrib.disk.buffering.internal.storage.TestData.THIRD_LOG_RECORD;
import static io.opentelemetry.contrib.disk.buffering.internal.storage.TestData.getConfiguration;
import static org.assertj.core.api.Assertions.assertThat;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertNull;
import static org.junit.jupiter.api.Assertions.fail;
import static org.assertj.core.api.Assertions.fail;
import io.opentelemetry.contrib.disk.buffering.internal.serialization.deserializers.SignalDeserializer;
import io.opentelemetry.contrib.disk.buffering.internal.serialization.serializers.SignalSerializer;
@ -65,7 +63,7 @@ class StorageTest {
forwardToReadTime();
ReadableResult<LogRecordData> readResult = storage.readNext(DESERIALIZER);
assertNotNull(readResult);
assertThat(readResult).isNotNull();
assertThat(readResult.getContent()).containsExactly(FIRST_LOG_RECORD, SECOND_LOG_RECORD);
assertThat(destinationDir.list()).hasSize(1);
@ -73,7 +71,7 @@ class StorageTest {
readResult.delete();
readResult.close();
ReadableResult<LogRecordData> readResult2 = storage.readNext(DESERIALIZER);
assertNotNull(readResult2);
assertThat(readResult2).isNotNull();
assertThat(readResult2.getContent()).containsExactly(THIRD_LOG_RECORD);
assertThat(destinationDir.list()).hasSize(1);
@ -88,7 +86,7 @@ class StorageTest {
// Read again when no more data is available (delete file)
readResult2.close();
assertNull(storage.readNext(DESERIALIZER));
assertThat(storage.readNext(DESERIALIZER)).isNull();
assertThat(destinationDir.list()).isEmpty();
}
@ -100,7 +98,7 @@ class StorageTest {
forwardToReadTime();
// Reading
assertNull(storage.readNext(DESERIALIZER));
assertThat(storage.readNext(DESERIALIZER)).isNull();
// Writing
assertThat(write(Collections.singletonList(THIRD_LOG_RECORD))).isFalse();
@ -125,7 +123,7 @@ class StorageTest {
// Read
ReadableResult<LogRecordData> result = storage.readNext(DESERIALIZER);
assertNotNull(result);
assertThat(result).isNotNull();
assertThat(result.getContent()).containsExactly(THIRD_LOG_RECORD);
assertThat(destinationDir.list())
.containsExactlyInAnyOrder(
@ -156,7 +154,7 @@ class StorageTest {
// Read
ReadableResult<LogRecordData> result = storage.readNext(DESERIALIZER);
assertNotNull(result);
assertThat(result).isNotNull();
assertThat(result.getContent()).containsExactly(FIRST_LOG_RECORD, SECOND_LOG_RECORD);
assertThat(destinationDir.list())
.containsExactlyInAnyOrder(
@ -166,7 +164,7 @@ class StorageTest {
// Read again
ReadableResult<LogRecordData> result2 = storage.readNext(DESERIALIZER);
assertNotNull(result2);
assertThat(result2).isNotNull();
assertThat(result2.getContent()).containsExactly(THIRD_LOG_RECORD);
assertThat(destinationDir.list()).containsExactly(String.valueOf(secondFileWriteTime));
result2.close();
@ -188,7 +186,7 @@ class StorageTest {
currentTimeMillis.set(4000 + MIN_FILE_AGE_FOR_READ_MILLIS);
// Read
assertNull(storage.readNext(DESERIALIZER));
assertThat(storage.readNext(DESERIALIZER)).isNull();
assertThat(destinationDir.list()).containsExactly("4000"); // it tries 3 times max per call.
}

View File

@ -12,7 +12,7 @@ import static io.opentelemetry.contrib.disk.buffering.internal.storage.TestData.
import static io.opentelemetry.contrib.disk.buffering.internal.storage.TestData.getConfiguration;
import static java.util.concurrent.TimeUnit.MILLISECONDS;
import static org.assertj.core.api.Assertions.assertThat;
import static org.junit.jupiter.api.Assertions.fail;
import static org.assertj.core.api.Assertions.fail;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;

View File

@ -26,14 +26,14 @@ public class ClientCallbackHandler implements CallbackHandler {
* @param password - authenticating password (plaintext)
* @param realm - authenticating realm
*/
public ClientCallbackHandler(final String username, final String password, final String realm) {
public ClientCallbackHandler(String username, String password, String realm) {
this.username = username;
this.password = password != null ? password.toCharArray() : null;
this.realm = realm;
}
@Override
public void handle(final Callback[] callbacks) throws UnsupportedCallbackException {
public void handle(Callback[] callbacks) throws UnsupportedCallbackException {
for (Callback callback : callbacks) {
if (callback instanceof NameCallback) {
((NameCallback) callback).setName(this.username);

View File

@ -8,11 +8,11 @@ package io.opentelemetry.contrib.jmxmetrics;
public class ConfigurationException extends RuntimeException {
private static final long serialVersionUID = 0L;
public ConfigurationException(final String message, final Throwable cause) {
public ConfigurationException(String message, Throwable cause) {
super(message, cause);
}
public ConfigurationException(final String message) {
public ConfigurationException(String message) {
super(message);
}
}

View File

@ -119,7 +119,7 @@ public class GroovyMetricEnvironment {
*
* @param config - used to establish exporter type (logging by default) and connection info
*/
public GroovyMetricEnvironment(final JmxConfig config) {
public GroovyMetricEnvironment(JmxConfig config) {
this(
config,
"io.opentelemetry.contrib.jmxmetrics",
@ -131,7 +131,7 @@ public class GroovyMetricEnvironment {
meterProvider.forceFlush().join(10, TimeUnit.SECONDS);
}
protected static Attributes mapToAttributes(@Nullable final Map<String, String> labelMap) {
protected static Attributes mapToAttributes(@Nullable Map<String, String> labelMap) {
if (labelMap == null) {
return Attributes.empty();
}

View File

@ -44,7 +44,7 @@ public class GroovyRunner {
List<String> scriptSources = new ArrayList<>();
try {
if (config.targetSystems.size() != 0) {
for (final String target : config.targetSystems) {
for (String target : config.targetSystems) {
String systemResourcePath = "target-systems/" + target + ".groovy";
scriptSources.add(getTargetSystemResourceAsString(systemResourcePath));
}
@ -59,7 +59,7 @@ public class GroovyRunner {
this.scripts = new ArrayList<>();
try {
for (final String source : scriptSources) {
for (String source : scriptSources) {
this.scripts.add(new GroovyShell().parse(source));
}
} catch (CompilationFailedException e) {
@ -74,18 +74,18 @@ public class GroovyRunner {
new OtelHelper(jmxClient, this.groovyMetricEnvironment, config.aggregateAcrossMBeans);
binding.setVariable("otel", otelHelper);
for (final Script script : scripts) {
for (Script script : scripts) {
script.setBinding(binding);
}
}
private static String getFileAsString(final String fileName) throws IOException {
private static String getFileAsString(String fileName) throws IOException {
try (InputStream is = new FileInputStream(fileName)) {
return getFileFromInputStream(is);
}
}
private String getTargetSystemResourceAsString(final String targetSystem) throws IOException {
private String getTargetSystemResourceAsString(String targetSystem) throws IOException {
URL res = getClass().getClassLoader().getResource(targetSystem);
if (res == null) {
throw new ConfigurationException("Failed to load " + targetSystem);
@ -98,15 +98,15 @@ public class GroovyRunner {
}
private static String getTargetSystemResourceFromJarAsString(URL res) throws IOException {
final String[] array = res.toString().split("!");
String[] array = res.toString().split("!");
if (array.length != 2) {
throw new ConfigurationException(
"Invalid path for target system resource from jar: " + res.toString());
}
final Map<String, String> env = Collections.emptyMap();
Map<String, String> env = Collections.emptyMap();
try {
try (final FileSystem fs = FileSystems.newFileSystem(URI.create(array[0]), env)) {
try (FileSystem fs = FileSystems.newFileSystem(URI.create(array[0]), env)) {
Path path = fs.getPath(array[1]);
try (InputStream is = Files.newInputStream(path)) {
return getFileFromInputStream(is);
@ -117,7 +117,7 @@ public class GroovyRunner {
}
}
private static String getFileFromInputStream(final InputStream is) throws IOException {
private static String getFileFromInputStream(InputStream is) throws IOException {
if (is == null) {
return null;
}
@ -134,7 +134,7 @@ public class GroovyRunner {
}
public void run() {
for (final Script script : scripts) {
for (Script script : scripts) {
script.run();
}
}

View File

@ -33,7 +33,7 @@ public class JmxClient {
private final boolean registrySsl;
@Nullable private JMXConnector jmxConn;
JmxClient(final JmxConfig config) throws MalformedURLException {
JmxClient(JmxConfig config) throws MalformedURLException {
this.url = new JMXServiceURL(config.serviceUrl);
this.username = config.username;
this.password = config.password;
@ -65,7 +65,7 @@ public class JmxClient {
env.put(
"jmx.remote.sasl.callback.handler",
new ClientCallbackHandler(this.username, this.password, this.realm));
} catch (final ReflectiveOperationException e) {
} catch (ReflectiveOperationException e) {
logger.warning("SASL unsupported in current environment: " + e.getMessage());
}
@ -83,7 +83,7 @@ public class JmxClient {
* @param objectName ObjectName to query
* @return the sorted list of applicable ObjectName instances found by server
*/
public List<ObjectName> query(final ObjectName objectName) {
public List<ObjectName> query(ObjectName objectName) {
MBeanServerConnection mbsc = getConnection();
if (mbsc == null) {
return Collections.emptyList();

View File

@ -80,7 +80,7 @@ class JmxConfig {
final boolean aggregateAcrossMBeans;
JmxConfig(final Properties props) {
JmxConfig(Properties props) {
properties = new Properties();
// putAll() instead of using constructor defaults
// to ensure they will be recorded to underlying map
@ -138,7 +138,7 @@ class JmxConfig {
this(new Properties());
}
private int getProperty(final String key, final int dfault) {
private int getProperty(String key, int dfault) {
String propVal = properties.getProperty(key);
if (propVal == null) {
return dfault;
@ -153,7 +153,7 @@ class JmxConfig {
/**
* Similar to getProperty(key, defaultValue) but sets the property to default if not in object.
*/
private String getAndSetProperty(final String key, final String defaultValue) {
private String getAndSetProperty(String key, String defaultValue) {
String propVal = properties.getProperty(key, defaultValue);
if (propVal.equals(defaultValue)) {
properties.setProperty(key, defaultValue);
@ -161,7 +161,7 @@ class JmxConfig {
return propVal;
}
private int getAndSetProperty(final String key, final int defaultValue) {
private int getAndSetProperty(String key, int defaultValue) {
int propVal = getProperty(key, defaultValue);
if (propVal == defaultValue) {
properties.setProperty(key, String.valueOf(defaultValue));
@ -202,7 +202,7 @@ class JmxConfig {
* @param s - {@link String} to evaluate
* @return - if s is null or without non-whitespace chars.
*/
static boolean isBlank(final String s) {
static boolean isBlank(String s) {
if (s == null) {
return true;
}

View File

@ -24,7 +24,7 @@ class JmxMetrics {
private final GroovyRunner runner;
private final JmxConfig config;
JmxMetrics(final JmxConfig config) {
JmxMetrics(JmxConfig config) {
this.config = config;
JmxClient jmxClient;
@ -60,7 +60,7 @@ class JmxMetrics {
exec.shutdown();
}
private static JmxConfig getConfigFromArgs(final String[] args) {
private static JmxConfig getConfigFromArgs(String[] args) {
if (args.length != 0 && (args.length != 2 || !args[0].equalsIgnoreCase("-config"))) {
System.out.println(
"Usage: java io.opentelemetry.contrib.jmxmetrics.JmxMetrics "
@ -105,11 +105,11 @@ class JmxMetrics {
*
* @param args - must be of the form "-config {jmx_config_path,'-'}"
*/
public static void main(final String[] args) {
public static void main(String[] args) {
JmxConfig config = getConfigFromArgs(args);
config.validate();
final JmxMetrics jmxMetrics = new JmxMetrics(config);
JmxMetrics jmxMetrics = new JmxMetrics(config);
jmxMetrics.start();
Runtime.getRuntime()

View File

@ -9,7 +9,7 @@ import static org.assertj.core.api.Assertions.assertThat;
import org.junit.jupiter.api.Test;
public class MavenUtilsTests {
class MavenUtilsTests {
@Test
public void getPluginArtifactIdShortName_builtinPluginName() {