Refactor `io.opentelemetry.instrumentation.resources.ContainerResourc…

…e` to avoid using null (open-telemetry#6889) While I was looking at issues open-telemetry#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#6694. Co-authored-by: Trask Stalnaker <[email protected]>
helios · Oct 31, 2022 · 776083d · 776083d
1 parent f560046
commit 776083d
Show file tree

Hide file tree

Showing 2 changed files with 74 additions and 93 deletions.
diff --git a/...s/library/src/main/java/io/opentelemetry/instrumentation/resources/ContainerResource.java b/...s/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<String> extractContainerId(Path cgroupFilePath) {
     if (!Files.exists(cgroupFilePath) || !Files.isReadable(cgroupFilePath)) {
-      return null;
+      return Optional.empty();
     }
     try (Stream<String> lines = Files.lines(cgroupFilePath)) {
-      Optional<String> 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<String> 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/...brary/src/test/java/io/opentelemetry/instrumentation/resources/ContainerResourceTest.java b/...brary/src/test/java/io/opentelemetry/instrumentation/resources/ContainerResourceTest.java
@@ -7,92 +7,82 @@
 
 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;
 import java.io.IOException;
 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<Arguments> 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) {