Skip to content

Fix/declarative config no exceptions #8079

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

Open
Bhagirath00 wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Fix/declarative config no exceptions #8079

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
df9a72b
Implement Environment Variable Context Propagation carriers in api/in…
Bhagirath00 Feb 12, 2026
6eed41c
Update DeclarativeConfigProperties to return null on type mismatch
Bhagirath00 Feb 13, 2026
2012cd9
Address feedback: clean up branch and improve safety in DeclarativeCo…
Bhagirath00 Feb 16, 2026
153c5d1
Improve null check messages for config property name and default value
Bhagirath00 Feb 18, 2026
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
59 changes: 25 additions & 34 deletions ...ator/src/main/java/io/opentelemetry/api/incubator/config/DeclarativeConfigProperties.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import io.opentelemetry.common.ComponentLoader;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Set;
import javax.annotation.Nullable;

Expand Down Expand Up @@ -46,8 +47,8 @@ static Map<String, Object> toMap(DeclarativeConfigProperties declarativeConfigPr
/**
* Returns a {@link String} configuration property.
*
* @return null if the property has not been configured
* @throws DeclarativeConfigException if the property is not a valid scalar string
* @return null if the property has not been configured or if the property is not a valid scalar
* string
*/
@Nullable
String getString(String name);
Expand All @@ -56,19 +57,19 @@ static Map<String, Object> toMap(DeclarativeConfigProperties declarativeConfigPr
* Returns a {@link String} configuration property.
*
* @return a {@link String} configuration property or {@code defaultValue} if a property with
* {@code name} has not been configured
* @throws DeclarativeConfigException if the property is not a valid scalar string
* {@code name} has not been configured or is not a valid scalar string
*/
default String getString(String name, String defaultValue) {
Copy link
Contributor

@robsunday robsunday Feb 18, 2026

Choose a reason for hiding this comment

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

For consistency I'd recommend adding here also:
Objects.requireNonNull(defaultValue, "Null default value");

Copy link
Contributor Author

@Bhagirath00 Bhagirath00 Feb 18, 2026

Choose a reason for hiding this comment

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

Makes sense, added the null check for defaultValue too.

Copy link
Member

@jack-berg jack-berg Mar 10, 2026

Choose a reason for hiding this comment

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

We need to get consistent about whether or not we do this. We rely on javax.annotation.Nullable annotation, the assumption that params and returns are non-null unless otherwise specified, leveraging the net.ltgt.nullaway error prone plugin to validate at build time. Of course, at runtime there is no guarantee that callers adhere to these annotations, and so we have code like this which checks for null despite the params being non-null according to the annotations.

We need to be consistent about:

  • When and why do we add additional null checks
  • Where do we do this (i.e. in interface default methods or in the actual implementation)
  • What is the behavior when the null annotation is violated (throw or noop)

Let's hold off here until we get some repo level guidance on this. ConfigProperties, the env var / sys property analog of DeclarativeConfigProperties, has the exact same method and does not do this null check.

Copy link
Member

@jack-berg jack-berg Mar 10, 2026

Choose a reason for hiding this comment

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

Created #8173 to track.

Objects.requireNonNull(defaultValue, "Null default value");
Copy link
Member

@jack-berg jack-berg Mar 12, 2026

Choose a reason for hiding this comment

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

Just to be clear, I'm recommending we pull this until #8173 is resolved. The YamlDeclarativeConfigProperties null checks are fine, since that's where I think our guidance ends up

Suggested change
Objects.requireNonNull(defaultValue, "Null default value");

return defaultIfNull(getString(name), defaultValue);
}

/**
* Returns a {@link Boolean} configuration property. Implementations should use the same rules as
* {@link Boolean#parseBoolean(String)} for handling the values.
*
* @return null if the property has not been configured
* @throws DeclarativeConfigException if the property is not a valid scalar boolean
* @return null if the property has not been configured or if the property is not a valid scalar
* boolean
*/
@Nullable
Boolean getBoolean(String name);
Expand All @@ -77,8 +78,7 @@ default String getString(String name, String defaultValue) {
* Returns a {@link Boolean} configuration property.
*
* @return a {@link Boolean} configuration property or {@code defaultValue} if a property with
* {@code name} has not been configured
* @throws DeclarativeConfigException if the property is not a valid scalar boolean
* {@code name} has not been configured or is not a valid scalar boolean
*/
default boolean getBoolean(String name, boolean defaultValue) {
return defaultIfNull(getBoolean(name), defaultValue);
Expand All @@ -90,8 +90,8 @@ default boolean getBoolean(String name, boolean defaultValue) {
* <p>If the underlying config property is {@link Long}, it is converted to {@link Integer} with
* {@link Long#intValue()} which may result in loss of precision.
*
* @return null if the property has not been configured
* @throws DeclarativeConfigException if the property is not a valid scalar integer
* @return null if the property has not been configured or if the property is not a valid scalar
* integer
*/
@Nullable
Integer getInt(String name);
Expand All @@ -103,8 +103,7 @@ default boolean getBoolean(String name, boolean defaultValue) {
* {@link Long#intValue()} which may result in loss of precision.
*
* @return a {@link Integer} configuration property or {@code defaultValue} if a property with
* {@code name} has not been configured
* @throws DeclarativeConfigException if the property is not a valid scalar integer
* {@code name} has not been configured or is not a valid scalar integer
*/
default int getInt(String name, int defaultValue) {
return defaultIfNull(getInt(name), defaultValue);
Expand All @@ -113,8 +112,8 @@ default int getInt(String name, int defaultValue) {
/**
* Returns a {@link Long} configuration property.
*
* @return null if the property has not been configured
* @throws DeclarativeConfigException if the property is not a valid scalar long
* @return null if the property has not been configured or if the property is not a valid scalar
* long
*/
@Nullable
Long getLong(String name);
Expand All @@ -123,8 +122,7 @@ default int getInt(String name, int defaultValue) {
* Returns a {@link Long} configuration property.
*
* @return a {@link Long} configuration property or {@code defaultValue} if a property with {@code
* name} has not been configured
* @throws DeclarativeConfigException if the property is not a valid scalar long
* name} has not been configured or is not a valid scalar long
*/
default long getLong(String name, long defaultValue) {
return defaultIfNull(getLong(name), defaultValue);
Expand All @@ -133,8 +131,8 @@ default long getLong(String name, long defaultValue) {
/**
* Returns a {@link Double} configuration property.
*
* @return null if the property has not been configured
* @throws DeclarativeConfigException if the property is not a valid scalar double
* @return null if the property has not been configured or if the property is not a valid scalar
* double
*/
@Nullable
Double getDouble(String name);
Expand All @@ -143,8 +141,7 @@ default long getLong(String name, long defaultValue) {
* Returns a {@link Double} configuration property.
*
* @return a {@link Double} configuration property or {@code defaultValue} if a property with
* {@code name} has not been configured
* @throws DeclarativeConfigException if the property is not a valid scalar double
* {@code name} has not been configured or is not a valid scalar double
*/
default double getDouble(String name, double defaultValue) {
return defaultIfNull(getDouble(name), defaultValue);
Expand All @@ -158,8 +155,8 @@ default double getDouble(String name, double defaultValue) {
* @param scalarType the scalar type, one of {@link String}, {@link Boolean}, {@link Long} or
* {@link Double}
* @return a {@link List} configuration property, or null if the property has not been configured
* @throws DeclarativeConfigException if the property is not a valid sequence of scalars, or if
* {@code scalarType} is not supported
* or if the property is not a valid sequence of scalars
* @throws DeclarativeConfigException if {@code scalarType} is not supported
*/
@Nullable
<T> List<T> getScalarList(String name, Class<T> scalarType);
Expand All @@ -172,8 +169,7 @@ default double getDouble(String name, double defaultValue) {
* @param scalarType the scalar type, one of {@link String}, {@link Boolean}, {@link Long} or
* {@link Double}
* @return a {@link List} configuration property or {@code defaultValue} if a property with {@code
* name} has not been configured
* @throws DeclarativeConfigException if the property is not a valid sequence of scalars
* name} has not been configured or is not a valid sequence of scalars
*/
default <T> List<T> getScalarList(String name, Class<T> scalarType, List<T> defaultValue) {
return defaultIfNull(getScalarList(name, scalarType), defaultValue);
Expand All @@ -183,8 +179,7 @@ default <T> List<T> getScalarList(String name, Class<T> scalarType, List<T> defa
* Returns a {@link DeclarativeConfigProperties} configuration property.
*
* @return a map-valued configuration property, or {@code null} if {@code name} has not been
* configured
* @throws DeclarativeConfigException if the property is not a mapping
* configured or is not a mapping
*/
@Nullable
DeclarativeConfigProperties getStructured(String name);
Expand All @@ -193,8 +188,7 @@ default <T> List<T> getScalarList(String name, Class<T> scalarType, List<T> defa
* Returns a list of {@link DeclarativeConfigProperties} configuration property.
*
* @return a map-valued configuration property, or {@code defaultValue} if {@code name} has not
* been configured
* @throws DeclarativeConfigException if the property is not a mapping
* been configured or is not a mapping
*/
default DeclarativeConfigProperties getStructured(
String name, DeclarativeConfigProperties defaultValue) {
Expand All @@ -210,8 +204,7 @@ default DeclarativeConfigProperties getStructured(
* but empty vs. not set, use {@link #getStructured(String)}.
*
* @return a map-valued configuration property, or an empty {@link DeclarativeConfigProperties}
* instance if {@code name} has not been configured
* @throws DeclarativeConfigException if the property is not a mapping
* instance if {@code name} has not been configured or is not a mapping
*/
default DeclarativeConfigProperties get(String name) {
return getStructured(name, empty());
Expand All @@ -221,8 +214,7 @@ default DeclarativeConfigProperties get(String name) {
* Returns a list of {@link DeclarativeConfigProperties} configuration property.
*
* @return a list of map-valued configuration property, or {@code null} if {@code name} has not
* been configured
* @throws DeclarativeConfigException if the property is not a sequence of mappings
* been configured or is not a sequence of mappings
*/
@Nullable
List<DeclarativeConfigProperties> getStructuredList(String name);
Expand All @@ -231,8 +223,7 @@ default DeclarativeConfigProperties get(String name) {
* Returns a list of {@link DeclarativeConfigProperties} configuration property.
*
* @return a list of map-valued configuration property, or {@code defaultValue} if {@code name}
* has not been configured
* @throws DeclarativeConfigException if the property is not a sequence of mappings
* has not been configured or is not a sequence of mappings
*/
default List<DeclarativeConfigProperties> getStructuredList(
String name, List<DeclarativeConfigProperties> defaultValue) {
Expand Down
11 changes: 10 additions & 1 deletion .../io/opentelemetry/sdk/extension/incubator/fileconfig/YamlDeclarativeConfigProperties.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,18 +156,21 @@ private static boolean isMap(Object object) {
@Nullable
@Override
public String getString(String name) {
Objects.requireNonNull(name, "Null configuration property name");
return stringOrNull(simpleEntries.get(name), name);
}

@Nullable
@Override
public Boolean getBoolean(String name) {
Objects.requireNonNull(name, "Null configuration property name");
return booleanOrNull(simpleEntries.get(name), name);
}

@Nullable
@Override
public Integer getInt(String name) {
Objects.requireNonNull(name, "Null configuration property name");
Object value = simpleEntries.get(name);
if (value instanceof Integer) {
return (Integer) value;
Expand All @@ -184,19 +187,22 @@ public Integer getInt(String name) {
@Nullable
@Override
public Long getLong(String name) {
Objects.requireNonNull(name, "Null configuration property name");
return longOrNull(simpleEntries.get(name), name);
}

@Nullable
@Override
public Double getDouble(String name) {
Objects.requireNonNull(name, "Null configuration property name");
return doubleOrNull(simpleEntries.get(name), name);
}

@Nullable
@Override
@SuppressWarnings("unchecked")
public <T> List<T> getScalarList(String name, Class<T> scalarType) {
Objects.requireNonNull(name, "Null configuration property name");
if (!SUPPORTED_SCALAR_TYPES.contains(scalarType)) {
throw new DeclarativeConfigException(
"Unsupported scalar type "
Expand Down Expand Up @@ -291,12 +297,14 @@ private static Double doubleOrNull(@Nullable Object value, String name) {
@Nullable
@Override
public DeclarativeConfigProperties getStructured(String name) {
Objects.requireNonNull(name, "Null configuration property name");
return mapEntries.get(name);
}

@Nullable
@Override
public List<DeclarativeConfigProperties> getStructuredList(String name) {
Objects.requireNonNull(name, "Null configuration property name");
List<YamlDeclarativeConfigProperties> value = listEntries.get(name);
if (value != null) {
return Collections.unmodifiableList(value);
Expand All @@ -315,7 +323,8 @@ public Set<String> getPropertyKeys() {

@Override
public String toString() {
StringJoiner joiner = new StringJoiner(", ", "YamlDeclarativeConfigProperties{", "}");
StringJoiner joiner =
new StringJoiner(", ", YamlDeclarativeConfigProperties.class.getSimpleName() + "{", "}");
simpleEntries.forEach((key, value) -> joiner.add(key + "=" + value));
listEntries.forEach((key, value) -> joiner.add(key + "=" + value));
mapEntries.forEach((key, value) -> joiner.add(key + "=" + value));
Expand Down
28 changes: 27 additions & 1 deletion ...opentelemetry/sdk/extension/incubator/fileconfig/YamlDeclarativeConfigPropertiesTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,8 @@ void additionalProperties() {
.isEqualTo(Arrays.asList(1.1d, 2.2d));
assertThat(otherProps.getScalarList("bool_list_key", Boolean.class))
.isEqualTo(Arrays.asList(true, false));
// If reading a scalar list which is mixed, entries which are not aligned with the requested
// If reading a scalar list which is mixed, entries which are not aligned with
// the requested
// type are filtered out
assertThat(otherProps.getScalarList("mixed_list_key", String.class))
.isEqualTo(Collections.singletonList("val1"));
Expand Down Expand Up @@ -230,8 +231,13 @@ void wrongType() {
assertThat(otherProps.getDouble("str_key")).isNull();
assertThat(otherProps.getBoolean("str_key")).isNull();
assertThat(otherProps.getScalarList("str_key", String.class)).isNull();
assertThat(otherProps.getScalarList("str_list_key", Long.class)).isNull();
assertThat(otherProps.getScalarList("str_list_key", Boolean.class)).isNull();
assertThat(otherProps.getScalarList("str_list_key", Double.class)).isNull();
assertThat(otherProps.getStructured("str_key")).isNull();
assertThat(otherProps.getStructuredList("str_key")).isNull();
assertThat(otherProps.getStructured("str_list_key")).isNull();
assertThat(otherProps.getStructuredList("map_key")).isNull();

assertWarning("Ignoring value for key [int_key] because it is Integer instead of String: 1");
assertWarning(
Expand All @@ -240,6 +246,26 @@ void wrongType() {
"Ignoring value for key [str_key] because it is String instead of Double: str_value");
assertWarning(
"Ignoring value for key [str_key] because it is String instead of Boolean: str_value");
assertWarning(
"Ignoring value for key [str_list_key] because it is String instead of Long: val1");
}

@Test
void wrongTypeWithDefault() {
DeclarativeConfigProperties otherProps = structuredConfigProps.getStructured("other");
assertThat(otherProps).isNotNull();

assertThat(otherProps.getString("int_key", "default")).isEqualTo("default");
assertThat(otherProps.getInt("str_key", 100)).isEqualTo(100);
assertThat(otherProps.getLong("str_key", 100L)).isEqualTo(100L);
assertThat(otherProps.getDouble("str_key", 1.1)).isEqualTo(1.1);
assertThat(otherProps.getBoolean("str_key", true)).isTrue();
assertThat(
otherProps.getScalarList("str_key", String.class, Collections.singletonList("default")))
.isEqualTo(Collections.singletonList("default"));
assertThat(otherProps.getStructured("str_key", empty())).isEqualTo(empty());
assertThat(otherProps.getStructuredList("str_key", Collections.emptyList()))
.isEqualTo(Collections.emptyList());
}

private void assertWarning(String message) {
Expand Down
Loading