Refactor io.opentelemetry.instrumentation.resources.ContainerResource to avoid using null
#6889
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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#6694 Issue-Id: open-telemetry/opentelemetry-java#2337
edysli
force-pushed
the
less_nulls_in_ContainerResource
branch
from
2eb89f9
to
29cbbfa
Compare
mateuszrzeszutek
approved these changes
Oct 17, 2022
Comment on lines
+63
to
+66
| .map(ContainerResource::getIdFromLine) | ||
| .filter(Optional::isPresent) | ||
| .findFirst() | ||
| .orElse(Optional.empty()); |
There was a problem hiding this comment.
Wouldn't
Suggested change
| .map(ContainerResource::getIdFromLine) | |
| .filter(Optional::isPresent) | |
| .findFirst() | |
| .orElse(Optional.empty()); | |
| .flatMap(ContainerResource::getIdFromLine); |
work here?
There was a problem hiding this comment.
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?
.../library/src/test/java/io/opentelemetry/instrumentation/resources/ContainerResourceTest.java
Outdated
Show resolved
Hide resolved
In general I agree with this statement; but keep in mind that this project prefers to use
Looks pretty good! 🚀 Thanks! |
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#6694 Issue-Id: open-telemetry/opentelemetry-java#2337
edysli
force-pushed
the
less_nulls_in_ContainerResource
branch
from
29cbbfa
to
034a2c1
Compare
|
Thank you very much for the review @mateuszrzeszutek ! Would you mind labelling this PR with "hacktoberfest-accepted" so it counts toward my Hacktoberfest 2022 efforts? 😁 |
dmarkwat
pushed a commit
to dmarkwat/opentelemetry-java-instrumentation
that referenced
this pull request
Oct 22, 2022
…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 <trask.stalnaker@gmail.com>
trask
added a commit
that referenced
this pull request
Oct 23, 2022
This was discussed a long while back, but never documented (and recently came up in #6889) Co-authored-by: Fabrizio Ferri-Benedetti <fferribenedetti@splunk.com>
LironKS
pushed a commit
to helios/opentelemetry-java-instrumentation
that referenced
this pull request
Oct 23, 2022
…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 <trask.stalnaker@gmail.com>
LironKS
pushed a commit
to helios/opentelemetry-java-instrumentation
that referenced
this pull request
Oct 31, 2022
…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 <trask.stalnaker@gmail.com>
LironKS
pushed a commit
to helios/opentelemetry-java-instrumentation
that referenced
this pull request
Oct 31, 2022
This was discussed a long while back, but never documented (and recently came up in open-telemetry#6889) Co-authored-by: Fabrizio Ferri-Benedetti <fferribenedetti@splunk.com>
LironKS
pushed a commit
to helios/opentelemetry-java-instrumentation
that referenced
this pull request
Dec 4, 2022
…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 <trask.stalnaker@gmail.com>
LironKS
pushed a commit
to helios/opentelemetry-java-instrumentation
that referenced
this pull request
Dec 4, 2022
This was discussed a long while back, but never documented (and recently came up in open-telemetry#6889) Co-authored-by: Fabrizio Ferri-Benedetti <fferribenedetti@splunk.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While I was looking at issues #6694 and open-telemetry/opentelemetry-java#2337, I saw that the code in
io.opentelemetry.instrumentation.resources.ContainerResourceusednullseveral times as return value which isn't safe. Nowadays,Optionalis better suited to signal the absence of a result, so I refactoredContainerResourceto useOptionals 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 #6694.