Resource.Builder for convenient work with Resource class #3065
Conversation
piotr-sumo
requested review from
anuraaga,
arminru,
bogdandrutu,
carlosalberto,
jkwatson,
Oberon00,
pavolloffay,
thisthat and
tylerbenson
as code owners
| return builder().putAll(this); | ||
| } | ||
|
|
||
| public static class Builder { |
There was a problem hiding this comment.
We don't use inner classes, can you move to ResourceBuilder?
| @@ -168,4 +168,89 @@ private static boolean isValid(String name) { | |||
| private static boolean isValidAndNotEmpty(AttributeKey<?> name) { | |||
| return !name.getKey().isEmpty() && isValid(name.getKey()); | |||
| } | |||
|
|
|||
| public static Resource.Builder builder() { | |||
There was a problem hiding this comment.
Let's add javadoc, you can probably mostly copy/paste from Attributes
| // then | ||
| Resource resource = builder.build(); | ||
| // yes, it's supposed to be the reference check | ||
| assertThat(resource != Resource.getDefault()).isTrue(); |
There was a problem hiding this comment.
We probably don't really need this assertion, but if including, assertThat(resource).isNotSameAs(...)
| } | ||
|
|
||
| @Test | ||
| void shouldBuilderCopyResource() { |
There was a problem hiding this comment.
We'll need to add test coverage of all the new methods
| * @param resource {@link Resource} whoose attributes will be copied | ||
| * @return this Builder | ||
| */ | ||
| public Builder putAll(@Nullable Resource resource) { |
There was a problem hiding this comment.
We should have defensive null checks for all the methods but we don't need to accept @Nullable here I think
| return new Resource.Builder(); | ||
| } | ||
|
|
||
| public Builder toBuilder() { |
There was a problem hiding this comment.
If we add this to incubator first, we would add ResourceBuilder.from(Resource and not add these methods yet. This class seems genuinely useful though and maybe we can go with it without incubator. @jkwatson what do you think?
|
@anuraaga I've implemented all requested changes. I'm waiting for the info about the incubator - shall I move the code there or not. |
sdk/common/src/main/java/io/opentelemetry/sdk/resources/ResourceBuilder.java
Show resolved
Hide resolved
|
My personal opinion is that this isn't a big enough change to warrant going through the incubation process. @bogdandrutu since this is an API addition, your opinion would be valuable. |
|
@jkwatson I've addressed your comments. |
|
Thanks @piotr-sumo |
|
|
||
| /** Create the {@link Resource} from this. */ | ||
| public Resource build() { | ||
| return new AutoValue_Resource(attributesBuilder.build()); |
There was a problem hiding this comment.
should this just call Resource.create() rather than having an extra reference to the autovalue implementation ?
There was a problem hiding this comment.
Thanks for hint, I've fixed this.
|
Thanks @piotr-sumo ! |
|
@anuraaga I'm happy to contribute 😄 |
Fixes #2975