From 6fb1f002417548e942f130bc88e79747aa63239c Mon Sep 17 00:00:00 2001 From: Etienne Dysli Metref Date: Wed, 19 Oct 2022 05:00:43 +0200 Subject: [PATCH] Refactor `io.opentelemetry.instrumentation.resources.ContainerResource` to avoid using null (#6889) While I was looking at issues open-telemetry/opentelemetry-java-instrumentation#6694 and open-telemetry/opentelemetry-java#2337, I saw that the code in `io.opentelemetry.instrumentation.resources.ContainerResource` used `null` several times as return value which isn't safe. Nowadays, `Optional` is better suited to signal the absence of a result, so I refactored `ContainerResource` to use `Optional`s instead of null. On the way, I also refactored this class's unit tests into parameterised tests to reduce test code duplication. These improvements should help implementing a solution to open-telemetry/opentelemetry-java-instrumentation#6694. Co-authored-by: Trask Stalnaker --- .../resources/ContainerResource.java | 47 +++---- .../resources/ContainerResourceTest.java | 120 ++++++++---------- 2 files changed, 74 insertions(+), 93 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(); } } 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) {