Refactor advice API

[jack-berg]

The explicit bucket boundaries advice API is now stable: open-telemetry/opentelemetry-specification#3694

Before it stabilized, there was a change made to adjust advice from encapsulating 1 or more parameters to being a class / category of parameter. The idea was to be able to offer simplified ergonomics to users:

// From this:
meter.histogramBuilder("foo")
  .setAdvice(advice -> advice
    .setExplicitBucketBoundaries(...)
    .setAttributes(..));

// to something like this:
meter.histogramBuilder("foo")
  .setExplicitBucketBoundariesAdvice(...)
  .setAttributesAdvice(...);

Would like to try to get our explicit bucket boundary stable, but first want to explore this as an option and collect feedback.

[codecov]

Codecov Report

Patch has no changes to coverable lines.

Files Changed	Coverage
...ncubator/metrics/ExtendedDoubleCounterBuilder.java	ø
.../incubator/metrics/ExtendedDoubleGaugeBuilder.java	ø
...ubator/metrics/ExtendedDoubleHistogramBuilder.java	0.00%
...or/metrics/ExtendedDoubleUpDownCounterBuilder.java	ø
.../incubator/metrics/ExtendedLongCounterBuilder.java	ø
...on/incubator/metrics/ExtendedLongGaugeBuilder.java	ø
...ncubator/metrics/ExtendedLongHistogramBuilder.java	0.00%
...ator/metrics/ExtendedLongUpDownCounterBuilder.java	ø
...io/opentelemetry/sdk/metrics/SdkDoubleCounter.java	ø
...a/io/opentelemetry/sdk/metrics/SdkDoubleGauge.java	ø
... and 6 more

📢 Thoughts on this report? Let us know!.

[trask]

// From this:
meter.histogramBuilder("foo")
  .setAdvice(advice -> advice
    .setExplicitBucketBoundaries(...)
    .setAttributes(..));

// to something like this:
meter.histogramBuilder("foo")
  .setExplicitBucketBoundariesAdvice(...)
  .setAttributesAdvice(...);

the second one looks simpler, so 👍🤷‍♂️ (but no really strong feelings)

one thing that's nice about the single setAdvice() method is that it gives one place to explain the whole advice story, but since the advice story differs a bit per advice type, maybe that's not much of an advantage(?)

[jack-berg]

Yeah I'm kind of a partial to the API proposed here. Another reason to go with separate methods is that the pattern of passing in a consumer to configure parameters may not be intuitive for every user.

[trask]

Another reason to go with separate methods is that the pattern of passing in a consumer to configure parameters may not be intuitive for every user.

ya, I've noticed confusion about the autoconfiguration builder which uses this pattern

[jack-berg]

@open-telemetry/java-approvers PTAL. Would like to get this in for the next release so we can stabilize the API in the following. Or, potentially stabilize in the next release with this updated API.

[jkwatson]

can we return this external interface without causing trouble for people using this class without the incubator code present?

[jack-berg]

The incubator code is always present on the classpath. Its just an implementation dependency instead of api, so users have to add it as a dependency of their own to access the extended behavior. Including the incubator as a implementation dependency is acceptable because its not part of the public API of the SDK, and it doesn't include any transitive dependencies that would make it questionable.

[jkwatson]

Seems reasonable to me. one question about returning an interface from another, optional module.