DeclarativeConfigPropertiesBridge - Handling time unit in the Duration type properties

[robsunday]

Current implementation of Duration type properties does't support time unit provided as a part of property value.
This PR adds a support of time units, as described in Java doc of ConfigProperties::getDuration

The implementation is mostly based on the implementation from DefaultConfigProperties class.
It supports additionally 'us' and 'ns' time units besides the units mentioned in the Java doc of ConfigProperties::getDuration

[zeitlinger]

the spec says that durations must be integers:

• https://github.com/open-telemetry/opentelemetry-configuration/blob/bd511a6a16b321a7bb3135fa783dda1fbd40706f/schema/common.json#L66
• I can't find that mentioned in https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/configuration/data-model.md - but the JSON schema is still very clear

so we'd need to change the spec first.

[robsunday]

The change I made is applicable for properties defined in instrumentation/development.java YAML node, so I think we do not need to update the schema, unless we want a full consistency.
I needed this modification to support some existing code that takes Duration property from instance of ConfigProperties.
Existing code also set some default for this property with specification of time unit. So getDuration() did not work when the code got DeclarativeConfigPropertiesBridge as an instance of ConfigProperties interface.

[zeitlinger]

The change I made is applicable for properties defined in instrumentation/development.java YAML node, so I think we do not need to update the schema, unless we want a full consistency.

good point for discussion

I needed this modification to support some existing code that takes Duration property from instance of ConfigProperties.

can you provide more context?

Existing code also set some default for this property with specification of time unit. So getDuration() did not work when the code got DeclarativeConfigPropertiesBridge as an instance of ConfigProperties interface.

the default logic should work already:

https://github.com/open-telemetry/opentelemetry-java/blob/78a917da2e8f4bc3645f4fb10361e3e844aab9fb/sdk-extensions/autoconfigure-spi/src/main/java/io/opentelemetry/sdk/autoconfigure/spi/ConfigProperties.java#L158

  default Duration getDuration(String name, Duration defaultValue) {
    return defaultIfNull(getDuration(name), defaultValue);
  }

[zeitlinger]

Related: open-telemetry/opentelemetry-configuration#282

[robsunday]

can you provide more context?

So I'm implementing support for declarative config in the Splunk Java agent. We have a lot of existing code and I need to make sure that it correctly works with declarative config. If possible I'd like to avoid making changes to this existing code.
There is our Splunk specific config property that is of type Duration, and can be specified with time unit, like: 10s
I use DeclarativeConfigPropertiesBridge for convenience, but current implementation throws an exception when specified value is actually a String consisting of a number and a time unit.
This is the reason why I made this change.

the default logic should work already:

You are right, however the existing code I mentioned use properties supplier to provide default value of this Duration config property. I blindly ported this code, but actually it was not necessary because all the places that were retrieving this value was indeed using getDuration with default anyway (I did not realize that).
Regardless of this, I think we need DeclarativeConfigPropertiesBridge::getDuration to support value with time unit.

[zeitlinger]

let's discuss this in the declarative config call later today

I've also just created a PoC for our distro - and your feedback is highly welcome 😄

see #14563

[zeitlinger]

@robsunday can you add the exception you got with the current code?