add "get" method to DeclarativeConfigProperties

[zeitlinger]

Addresses open-telemetry/opentelemetry-java-instrumentation#15641 (comment)

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.13%. Comparing base (e3a377d) to head (b7ebfeb).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff            @@
##               main    #7923   +/-   ##
=========================================
  Coverage     90.12%   90.13%           
- Complexity     7318     7320    +2     
=========================================
  Files           824      824           
  Lines         22027    22028    +1     
  Branches       2177     2177           
=========================================
+ Hits          19852    19854    +2     
  Misses         1498     1498           
+ Partials        677      676    -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[trask]

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

[jack-berg]

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);

[jack-berg]

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.

[jack-berg]

I'm not completely convinced that the method name get implies that I'm asking for a structured node back. Can't think of any better names that keep good UX, and the javadoc provides details, so let's just go ahead with this.