Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,22 @@ default DeclarativeConfigProperties getStructured(
return defaultIfNull(getStructured(name), defaultValue);
}

/**
* Returns a {@link DeclarativeConfigProperties} configuration property, or {@link #empty()} if a
* property with {@code name} has not been configured.
*
* <p>This is syntactic sugar for the common operation of calling {@code
* config.getStructured(name, empty())}. If you need to distinguish between a property being set
* 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
Copy link
Member

Choose a reason for hiding this comment

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

wdyt of not throwing exception for this (and other) methods which have default values?

Copy link
Member

Choose a reason for hiding this comment

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

If we took this approach I think we'd want to go further and get rid of the the throws altogether. It would be strange if calling the non-default overload threw (rather than just returning null) while the default did not.

Going in this direction means we remove the ability to detect / handle schema type mismatches. It seems like a version of the "fail fast" vs. "handle gracefully and trudge on".

But hold on... the default YamlDeclarativeConfigProperties implementation does warn you if there is a type mismatch, but never throws! So all of these warnings are fake!

I do wonder if we should provide abilities to detect schema type mismatches and fail fast. API could look like:

boolean isStringOrNull(String name);
boolean isBooleanOrNull(String name);

Copy link
Member

Choose a reason for hiding this comment

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

Discussed in the 12/18 java SIG and decided that:

  • The opentelemetry-java-instrumentation has a preference for failing graceful (at least for now - we discussed both options in detail)
  • We should work to create actionable warning logs if the a config property type mismatch occurs.
    • Current error messages only use the local context of the node. I.e. Ignoring value for key [foo] because it is String instead of Integer: value
    • Good error messages would include the full path Ignoring value for property [full.path.to.foo] because it is String instead of Integer: value
  • It would be good to still allow for instrumentation libraries to fail fast. If the API defaults to leniency, then we'd need to provide additional APIs for facilitate this. Can add these when needed.

The work to strip out the @throws DeclarativeConfigException to reflect current behavior, and to improve error log messages can be done in separate PRs.

*/
default DeclarativeConfigProperties get(String name) {
return getStructured(name, empty());
}

/**
* Returns a list of {@link DeclarativeConfigProperties} configuration property.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@ void treeWalking() {
// Access string at .foo.bar.baz without null checking and without exception.
assertThat(
structuredConfigProps
.getStructured("foo", empty())
.get("foo") // short for getStructured("foo", empty())
.getStructured("bar", empty())
.getString("baz"))
.isNull();
Expand Down