Skip to content

Add getStructured default method, add empty StructuredConfigProperties #6759

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

Merged
merged 1 commit into from
Oct 2, 2024
Merged

Add getStructured default method, add empty StructuredConfigProperties #6759

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
77 changes: 77 additions & 0 deletions ...java/io/opentelemetry/sdk/autoconfigure/spi/internal/EmptyStructuredConfigProperties.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.sdk.autoconfigure.spi.internal;

import java.util.Collections;
import java.util.List;
import java.util.Set;
import javax.annotation.Nullable;

/** Empty instance of {@link StructuredConfigProperties}. */
final class EmptyStructuredConfigProperties implements StructuredConfigProperties {

private static final EmptyStructuredConfigProperties INSTANCE =
new EmptyStructuredConfigProperties();

private EmptyStructuredConfigProperties() {}

static EmptyStructuredConfigProperties getInstance() {
return INSTANCE;
}

@Nullable
@Override
public String getString(String name) {
return null;
}

@Nullable
@Override
public Boolean getBoolean(String name) {
return null;
}

@Nullable
@Override
public Integer getInt(String name) {
return null;
}

@Nullable
@Override
public Long getLong(String name) {
return null;
}

@Nullable
@Override
public Double getDouble(String name) {
return null;
}

@Nullable
@Override
public <T> List<T> getScalarList(String name, Class<T> scalarType) {
return null;
}

@Nullable
@Override
public StructuredConfigProperties getStructured(String name) {
return null;
}

@Nullable
@Override
public List<StructuredConfigProperties> getStructuredList(String name) {
return null;
}

@Override
public Set<String> getPropertyKeys() {
return Collections.emptySet();

Check warning on line 75 in sdk-extensions/autoconfigure-spi/src/main/java/io/opentelemetry/sdk/autoconfigure/spi/internal/EmptyStructuredConfigProperties.java

View check run for this annotation

Codecov / codecov/patch

sdk-extensions/autoconfigure-spi/src/main/java/io/opentelemetry/sdk/autoconfigure/spi/internal/EmptyStructuredConfigProperties.java#L75 

Added line #L75 was not covered by tests
}
}
35 changes: 35 additions & 0 deletions ...main/java/io/opentelemetry/sdk/autoconfigure/spi/internal/StructuredConfigProperties.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,17 @@
*/
public interface StructuredConfigProperties {

/**
* Return an empty {@link StructuredConfigProperties} instance.
*
* <p>Useful for walking the tree without checking for null. For example, to access a string key
* nested at .foo.bar.baz, call: {@code config.getStructured("foo", empty()).getStructured("bar",
* empty()).getString("baz")}.
*/
static StructuredConfigProperties empty() {
return EmptyStructuredConfigProperties.getInstance();
}

/**
* Returns a {@link String} configuration property.
*
Expand Down Expand Up @@ -168,6 +179,18 @@ default <T> List<T> getScalarList(String name, Class<T> scalarType, List<T> defa
@Nullable
StructuredConfigProperties getStructured(String name);

/**
* Returns a {@link StructuredConfigProperties} configuration property.
*
* @return a map-valued configuration property, or {@code defaultValue} if {@code name} has not
* been configured
* @throws ConfigurationException if the property is not a mapping
*/
default StructuredConfigProperties getStructured(
String name, StructuredConfigProperties defaultValue) {
return defaultIfNull(getStructured(name), defaultValue);
}

/**
* Returns a list of {@link StructuredConfigProperties} configuration property.
*
Expand All @@ -178,6 +201,18 @@ default <T> List<T> getScalarList(String name, Class<T> scalarType, List<T> defa
@Nullable
List<StructuredConfigProperties> getStructuredList(String name);

/**
* Returns a list of {@link StructuredConfigProperties} configuration property.
*
* @return a list of map-valued configuration property, or {@code defaultValue} if {@code name}
* has not been configured
* @throws ConfigurationException if the property is not a sequence of mappings
*/
default List<StructuredConfigProperties> getStructuredList(
String name, List<StructuredConfigProperties> defaultValue) {
return defaultIfNull(getStructuredList(name), defaultValue);
}

Comment on lines +204 to +215
Copy link
Member

Choose a reason for hiding this comment

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

can you give an example of how you see this method being used?

Copy link
Member Author

Choose a reason for hiding this comment

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

There's a demonstration here in the unit test:

    // Validate common pattern of walking down tree path which is not defined
    // Access string at .foo.bar.baz without null checking and without exception.
    assertThat(
            structuredConfigProps
                .getStructured("foo", empty())
                .getStructured("bar", empty())
                .getString("baz"))
        .isNull();

I give the same example in the javadoc for the new empty() method, but should probably copy it here as well.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I meant specifically the list method, it's not clear to me if that variant is needed

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah sorry for misunderstanding.

At that point, it was just for completeness. Every other accessor has a "get or default" variant, and so it feels out of place for this one omission. Its true that it doesn't have utility for tree walking, but that doesn't mean it doesn't have utility at all. For example, this code for interpreting OTLP headers could be simplified to:

config.getStructuredList("headers", List.of()).forEach(header -> {
    String name = header.getString("name");
    String value = header.getString("value");
    if (name != null && value != null) {
      addHeader.accept(name, value);
    }
});

Copy link
Member

Choose a reason for hiding this comment

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

Every other accessor has a "get or default" variant

good point, I hadn't thought about the existing methods already having the default variant

/**
* Returns a set of all configuration property keys.
*
Expand Down
38 changes: 38 additions & 0 deletions .../opentelemetry/sdk/extension/incubator/fileconfig/YamlStructuredConfigPropertiesTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

package io.opentelemetry.sdk.extension.incubator.fileconfig;

import static io.opentelemetry.sdk.autoconfigure.spi.internal.StructuredConfigProperties.empty;
import static org.assertj.core.api.Assertions.assertThat;

import com.google.common.collect.ImmutableSet;
Expand Down Expand Up @@ -170,6 +171,18 @@ void additionalProperties() {
assertThat(listKeyProps2.getInt("int_key1")).isEqualTo(2);
}

@Test
void treeWalking() {
// Validate common pattern of walking down tree path which is not defined
// Access string at .foo.bar.baz without null checking and without exception.
assertThat(
structuredConfigProps
.getStructured("foo", empty())
.getStructured("bar", empty())
.getString("baz"))
.isNull();
}

@Test
void defaults() {
assertThat(structuredConfigProps.getString("foo", "bar")).isEqualTo("bar");
Expand All @@ -181,6 +194,9 @@ void defaults() {
structuredConfigProps.getScalarList(
"foo", String.class, Collections.singletonList("bar")))
.isEqualTo(Collections.singletonList("bar"));
assertThat(structuredConfigProps.getStructured("foo", empty())).isEqualTo(empty());
assertThat(structuredConfigProps.getStructuredList("foo", Collections.emptyList()))
.isEqualTo(Collections.emptyList());
}

@Test
Expand Down Expand Up @@ -209,4 +225,26 @@ void wrongType() {
assertThat(otherProps.getStructured("str_key")).isNull();
assertThat(otherProps.getStructuredList("str_key")).isNull();
}

@Test
void emptyProperties() {
assertThat(empty().getString("foo")).isNull();
assertThat(empty().getInt("foo")).isNull();
assertThat(empty().getLong("foo")).isNull();
assertThat(empty().getDouble("foo")).isNull();
assertThat(empty().getBoolean("foo")).isNull();
assertThat(empty().getScalarList("foo", String.class)).isNull();
assertThat(empty().getStructured("foo")).isNull();
assertThat(empty().getStructuredList("foo")).isNull();
assertThat(empty().getString("foo", "bar")).isEqualTo("bar");
assertThat(empty().getInt("foo", 1)).isEqualTo(1);
assertThat(empty().getLong("foo", 1)).isEqualTo(1);
assertThat(empty().getDouble("foo", 1.1)).isEqualTo(1.1);
assertThat(empty().getBoolean("foo", true)).isTrue();
assertThat(empty().getScalarList("foo", String.class, Collections.singletonList("bar")))
.isEqualTo(Collections.singletonList("bar"));
assertThat(empty().getStructured("foo", empty())).isEqualTo(empty());
assertThat(empty().getStructuredList("foo", Collections.emptyList()))
.isEqualTo(Collections.emptyList());
}
}
Loading