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 <trask.stalnaker@gmail.com>
This commit is contained in:
Etienne Dysli Metref 2022-10-19 05:00:43 +02:00 committed by GitHub
parent 465d4d3f9b
commit 6fb1f00241
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 74 additions and 93 deletions

View File

@ -12,12 +12,10 @@ import io.opentelemetry.semconv.resource.attributes.ResourceAttributes;
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 @@ public final class ContainerResource {
// 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 final class ContainerResource {
* @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 @@ public final class ContainerResource {
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();
}
}

View File

@ -7,6 +7,7 @@ package io.opentelemetry.instrumentation.resources;
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.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");
void buildResource_returnsEmptyResource_whenFileDoesNotExist(@TempDir Path tempFolder) {
Path cgroup = tempFolder.resolve("DoesNotExist");
assertThat(buildResource(cgroup)).isEqualTo(Resource.empty());
}
// 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");
@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);
}
// just container id
Path cgroup3 =
createCgroup(
tempFolder.resolve("cgroup3"),
"13:name=systemd:/pod/d86d75589bf6cc254f3e2cc29debdf85dde404998aa128997a819ff991827356");
assertThat(getContainerId(buildResource(cgroup3)))
.isEqualTo("d86d75589bf6cc254f3e2cc29debdf85dde404998aa128997a819ff991827356");
// 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) {