From 860c7160a29031f54c119a75f272f594f5a8b519 Mon Sep 17 00:00:00 2001 From: Etienne Dysli Metref Date: Sun, 16 Oct 2022 00:02:22 +0200 Subject: [PATCH 1/2] Refactor ContainerResource to avoid using nulls Refactor class `ContainerResource` so that its methods return `Optional`s instead of `null`. This makes for safer code that doesn't rely on `null` to signal the absence of result. Moreover, `Optional.map()` and `Optional.orElseGet()` use the same functional style as `Stream`, which is well readable. Issue-Id: open-telemetry/opentelemetry-java-instrumentation#6694 Issue-Id: open-telemetry/opentelemetry-java#2337 --- .../resources/ContainerResource.java | 47 ++++++++----------- 1 file changed, 19 insertions(+), 28 deletions(-) diff --git a/instrumentation/resources/library/src/main/java/io/opentelemetry/instrumentation/resources/ContainerResource.java b/instrumentation/resources/library/src/main/java/io/opentelemetry/instrumentation/resources/ContainerResource.java index df988e7e0ac2..03040aa9c9da 100644 --- a/instrumentation/resources/library/src/main/java/io/opentelemetry/instrumentation/resources/ContainerResource.java +++ b/instrumentation/resources/library/src/main/java/io/opentelemetry/instrumentation/resources/ContainerResource.java @@ -12,12 +12,10 @@ import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.Paths; -import java.util.Objects; import java.util.Optional; import java.util.logging.Level; import java.util.logging.Logger; import java.util.stream.Stream; -import javax.annotation.Nullable; import org.codehaus.mojo.animal_sniffer.IgnoreJRERequirement; /** Factory for {@link Resource} retrieving Container ID information. */ @@ -35,13 +33,11 @@ private static Resource buildSingleton(String uniqueHostNameFileName) { // package private for testing static Resource buildResource(Path path) { - String containerId = extractContainerId(path); - - if (containerId == null || containerId.isEmpty()) { - return Resource.empty(); - } else { - return Resource.create(Attributes.of(ResourceAttributes.CONTAINER_ID, containerId)); - } + return extractContainerId(path) + .map( + containerId -> + Resource.create(Attributes.of(ResourceAttributes.CONTAINER_ID, containerId))) + .orElseGet(Resource::empty); } /** Returns resource with container information. */ @@ -57,33 +53,28 @@ public static Resource get() { * @return containerId */ @IgnoreJRERequirement - @Nullable - private static String extractContainerId(Path cgroupFilePath) { + private static Optional extractContainerId(Path cgroupFilePath) { if (!Files.exists(cgroupFilePath) || !Files.isReadable(cgroupFilePath)) { - return null; + return Optional.empty(); } try (Stream lines = Files.lines(cgroupFilePath)) { - Optional value = - lines - .filter(line -> !line.isEmpty()) - .map(ContainerResource::getIdFromLine) - .filter(Objects::nonNull) - .findFirst(); - if (value.isPresent()) { - return value.get(); - } + return lines + .filter(line -> !line.isEmpty()) + .map(ContainerResource::getIdFromLine) + .filter(Optional::isPresent) + .findFirst() + .orElse(Optional.empty()); } catch (Exception e) { logger.log(Level.WARNING, "Unable to read file", e); } - return null; + return Optional.empty(); } - @Nullable - private static String getIdFromLine(String line) { + private static Optional getIdFromLine(String line) { // This cgroup output line should have the container id in it int lastSlashIdx = line.lastIndexOf('/'); if (lastSlashIdx < 0) { - return null; + return Optional.empty(); } String containerId; @@ -105,16 +96,16 @@ private static String getIdFromLine(String line) { endIdx = lastSection.length(); } if (startIdx > endIdx) { - return null; + return Optional.empty(); } containerId = lastSection.substring(startIdx, endIdx); } if (OtelEncodingUtils.isValidBase16String(containerId) && !containerId.isEmpty()) { - return containerId; + return Optional.of(containerId); } else { - return null; + return Optional.empty(); } } From 034a2c1eefddf4c71c16e8cbd5e16af060e9e380 Mon Sep 17 00:00:00 2001 From: Etienne Dysli Metref Date: Sun, 16 Oct 2022 00:11:48 +0200 Subject: [PATCH 2/2] Refactor ContainerResourceTest to use parameterized tests Several test cases in `ContainerResourceTest` have the same outcome, so instead of duplicating the assertions inside each test method, take advantage of `@ParameterizedTest` to inject inputs and expected outputs multiple times into the same test. This improves test code readability by reducing test method length and making identical cases more obvious. Also rename test methods to better express the tested code's expected behaviour. Issue-Id: open-telemetry/opentelemetry-java-instrumentation#6694 Issue-Id: open-telemetry/opentelemetry-java#2337 --- .../resources/ContainerResourceTest.java | 120 ++++++++---------- 1 file changed, 55 insertions(+), 65 deletions(-) diff --git a/instrumentation/resources/library/src/test/java/io/opentelemetry/instrumentation/resources/ContainerResourceTest.java b/instrumentation/resources/library/src/test/java/io/opentelemetry/instrumentation/resources/ContainerResourceTest.java index 826270c02f78..82408b102dc4 100644 --- a/instrumentation/resources/library/src/test/java/io/opentelemetry/instrumentation/resources/ContainerResourceTest.java +++ b/instrumentation/resources/library/src/test/java/io/opentelemetry/instrumentation/resources/ContainerResourceTest.java @@ -7,6 +7,7 @@ import static io.opentelemetry.instrumentation.resources.ContainerResource.buildResource; import static org.assertj.core.api.Assertions.assertThat; +import static org.junit.jupiter.params.provider.Arguments.arguments; import io.opentelemetry.sdk.resources.Resource; import io.opentelemetry.semconv.resource.attributes.ResourceAttributes; @@ -14,85 +15,74 @@ import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.nio.file.Path; +import java.util.stream.Stream; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.io.TempDir; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; +import org.junit.jupiter.params.provider.ValueSource; class ContainerResourceTest { - @Test - void buildResource_Invalid(@TempDir Path tempFolder) throws IOException { - // invalid containerId (non-hex) - Path cgroup = - createCgroup( - tempFolder.resolve("cgroup1"), - "13:name=systemd:/podruntime/docker/kubepods/ac679f8a8319c8cf7d38e1adf263bc08d23zzzz"); - assertThat(buildResource(cgroup)).isEqualTo(Resource.empty()); - - // unrecognized format (last "-" is after last ".") - cgroup = - createCgroup( - tempFolder.resolve("cgroup1"), - "13:name=systemd:/podruntime/docker/kubepods/ac679f8.a8319c8cf7d38e1adf263bc08-d23zzzz"); - assertThat(buildResource(cgroup)).isEqualTo(Resource.empty()); - - // test invalid file - cgroup = tempFolder.resolve("DoesNotExist"); + @ParameterizedTest + @ValueSource( + strings = { + // invalid containerId (non-hex) + "13:name=systemd:/podruntime/docker/kubepods/ac679f8a8319c8cf7d38e1adf263bc08d23zzzz", + // unrecognized format (last "-" is after last ".") + "13:name=systemd:/podruntime/docker/kubepods/ac679f8.a8319c8cf7d38e1adf263bc08-d23zzzz" + }) + void buildResource_returnsEmptyResource_whenContainerIdIsInvalid( + String line, @TempDir Path tempFolder) throws IOException { + Path cgroup = createCgroup(tempFolder.resolve("cgroup"), line); assertThat(buildResource(cgroup)).isEqualTo(Resource.empty()); } @Test - void buildResource_Valid(@TempDir Path tempFolder) throws IOException { - // with suffix - Path cgroup = - createCgroup( - tempFolder.resolve("cgroup1"), - "13:name=systemd:/podruntime/docker/kubepods/ac679f8a8319c8cf7d38e1adf263bc08d23.aaaa"); - assertThat(getContainerId(buildResource(cgroup))) - .isEqualTo("ac679f8a8319c8cf7d38e1adf263bc08d23"); - - // with prefix and suffix - Path cgroup2 = - createCgroup( - tempFolder.resolve("cgroup2"), - "13:name=systemd:/podruntime/docker/kubepods/crio-dc679f8a8319c8cf7d38e1adf263bc08d23.stuff"); - assertThat(getContainerId(buildResource(cgroup2))) - .isEqualTo("dc679f8a8319c8cf7d38e1adf263bc08d23"); + void buildResource_returnsEmptyResource_whenFileDoesNotExist(@TempDir Path tempFolder) { + Path cgroup = tempFolder.resolve("DoesNotExist"); + assertThat(buildResource(cgroup)).isEqualTo(Resource.empty()); + } - // just container id - Path cgroup3 = - createCgroup( - tempFolder.resolve("cgroup3"), - "13:name=systemd:/pod/d86d75589bf6cc254f3e2cc29debdf85dde404998aa128997a819ff991827356"); - assertThat(getContainerId(buildResource(cgroup3))) - .isEqualTo("d86d75589bf6cc254f3e2cc29debdf85dde404998aa128997a819ff991827356"); + @ParameterizedTest + @MethodSource("validLines") + void buildResource_extractsContainerIdFromValidLines( + String line, String expectedContainerId, @TempDir Path tempFolder) throws IOException { + Path cgroup = createCgroup(tempFolder.resolve("cgroup"), line); + assertThat(getContainerId(buildResource(cgroup))).isEqualTo(expectedContainerId); + } - // with prefix - Path cgroup4 = - createCgroup( - tempFolder.resolve("cgroup4"), + static Stream validLines() { + return Stream.of( + // with suffix + arguments( + "13:name=systemd:/podruntime/docker/kubepods/ac679f8a8319c8cf7d38e1adf263bc08d23.aaaa", + "ac679f8a8319c8cf7d38e1adf263bc08d23"), + // with prefix and suffix + arguments( + "13:name=systemd:/podruntime/docker/kubepods/crio-dc679f8a8319c8cf7d38e1adf263bc08d23.stuff", + "dc679f8a8319c8cf7d38e1adf263bc08d23"), + // just container id + arguments( + "13:name=systemd:/pod/d86d75589bf6cc254f3e2cc29debdf85dde404998aa128997a819ff991827356", + "d86d75589bf6cc254f3e2cc29debdf85dde404998aa128997a819ff991827356"), + // with prefix + arguments( "//\n" + "1:name=systemd:/podruntime/docker/kubepods/docker-dc579f8a8319c8cf7d38e1adf263bc08d23" + "2:name=systemd:/podruntime/docker/kubepods/docker-dc579f8a8319c8cf7d38e1adf263bc08d23" - + "3:name=systemd:/podruntime/docker/kubepods/docker-dc579f8a8319c8cf7d38e1adf263bc08d23"); - assertThat(getContainerId(buildResource(cgroup4))) - .isEqualTo("dc579f8a8319c8cf7d38e1adf263bc08d23"); - - // with two dashes in prefix - Path cgroup5 = - createCgroup( - tempFolder.resolve("cgroup5"), - "11:perf_event:/kubepods.slice/kubepods-burstable.slice/kubepods-burstable-pod4415fd05_2c0f_4533_909b_f2180dca8d7c.slice/cri-containerd-713a77a26fe2a38ebebd5709604a048c3d380db1eb16aa43aca0b2499e54733c.scope"); - assertThat(getContainerId(buildResource(cgroup5))) - .isEqualTo("713a77a26fe2a38ebebd5709604a048c3d380db1eb16aa43aca0b2499e54733c"); - - // with colon, env: k8s v1.24.0, the cgroupDriver by systemd(default), and container is - // cri-containerd v1.6.8 - Path cgroup6 = - createCgroup( - tempFolder.resolve("cgroup6"), - "11:devices:/system.slice/containerd.service/kubepods-pod87a18a64_b74a_454a_b10b_a4a36059d0a3.slice:cri-containerd:05c48c82caff3be3d7f1e896981dd410e81487538936914f32b624d168de9db0"); - assertThat(getContainerId(buildResource(cgroup6))) - .isEqualTo("05c48c82caff3be3d7f1e896981dd410e81487538936914f32b624d168de9db0"); + + "3:name=systemd:/podruntime/docker/kubepods/docker-dc579f8a8319c8cf7d38e1adf263bc08d23", + "dc579f8a8319c8cf7d38e1adf263bc08d23"), + // with two dashes in prefix + arguments( + "11:perf_event:/kubepods.slice/kubepods-burstable.slice/kubepods-burstable-pod4415fd05_2c0f_4533_909b_f2180dca8d7c.slice/cri-containerd-713a77a26fe2a38ebebd5709604a048c3d380db1eb16aa43aca0b2499e54733c.scope", + "713a77a26fe2a38ebebd5709604a048c3d380db1eb16aa43aca0b2499e54733c"), + // with colon, env: k8s v1.24.0, the cgroupDriver by systemd(default), and container is + // cri-containerd v1.6.8 + arguments( + "11:devices:/system.slice/containerd.service/kubepods-pod87a18a64_b74a_454a_b10b_a4a36059d0a3.slice:cri-containerd:05c48c82caff3be3d7f1e896981dd410e81487538936914f32b624d168de9db0", + "05c48c82caff3be3d7f1e896981dd410e81487538936914f32b624d168de9db0")); } private static String getContainerId(Resource resource) {