Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor io.opentelemetry.instrumentation.resources.ContainerResource to avoid using null #6889

Merged
merged 3 commits into from
Oct 19, 2022
Merged

Refactor io.opentelemetry.instrumentation.resources.ContainerResource to avoid using null #6889

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
860c716
Refactor ContainerResource to avoid using nulls
edysli Oct 15, 2022
034a2c1
Refactor ContainerResourceTest to use parameterized tests
edysli Oct 15, 2022
fe3190a
Merge remote-tracking branch 'upstream/main' into less_nulls_in_Conta…
trask Oct 19, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 19 additions & 28 deletions ...s/library/src/main/java/io/opentelemetry/instrumentation/resources/ContainerResource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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. */
Expand All @@ -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. */
Expand All @@ -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());
Comment on lines +63 to +66
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't

Suggested change
.map(ContainerResource::getIdFromLine)
.filter(Optional::isPresent)
.findFirst()
.orElse(Optional.empty());
.flatMap(ContainerResource::getIdFromLine);

work here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, unfortunately flatMap() won't work here because ContainerResource::getIdFromLine returns Optional<String> and we end up with Stream<Optional<String>>. It would work as the last step though, to "pop" the Optional<Optional<String>>:

Suggested change
.map(ContainerResource::getIdFromLine)
.filter(Optional::isPresent)
.findFirst()
.orElse(Optional.empty());
.map(ContainerResource::getIdFromLine)
.filter(Optional::isPresent)
.findFirst()
.flatMap(identity());

Is the last line clearer this way?

} 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;
Expand All @@ -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();
}
}

Expand Down
120 changes: 55 additions & 65 deletions ...brary/src/test/java/io/opentelemetry/instrumentation/resources/ContainerResourceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down