DSL vs imperative configuration and disk buffering DSL config

[bencehornak]

I've recently faced the dilemma of using the new DSL configuration API (i.e. OpenTelemetryRumInitializer): I wanted to make use of the "reasonable defaults" that it comes with, however, there was just one thing I wanted to override: the disk buffering configuration by increasing the exporting interval. Since the related disk buffering properties (maxFileAgeForWriteMillis, minFileAgeForReadMillis, maxFileAgeForReadMillis) are not exposed (you can only enable or disable it currently, see below), this meant that I had no choice, but to fully ditch the DSL API and include all the boilerplate in my project that the android-agent module is supposed to hide.

opentelemetry-android/android-agent/src/main/kotlin/io/opentelemetry/android/agent/dsl/DiskBufferingConfigurationSpec.kt

Lines 13 to 32 in ad76f89

	* Type-safe config DSL that controls how disk buffering of exported telemetry should behave.
	*/
	@OpenTelemetryDslMarker
	class DiskBufferingConfigurationSpec internal constructor(
	private val rumConfig: OtelRumConfig,
	) : CanBeEnabledAndDisabled {
	internal var enabled: Boolean = true
	set(value) {
	field = value
	rumConfig.setDiskBufferingConfig(DiskBufferingConfig.create(enabled = value))
	}
	init {
	rumConfig.setDiskBufferingConfig(DiskBufferingConfig.create(enabled))
	}
	override fun enabled(enabled: Boolean) {
	this.enabled = enabled
	}
	}

So I have some questions related to my recent experiences:

1. Are you planning to make the DSL configuration be on par with the imperative configuration API (i.e. RumBuilder)?
2. If not:
   1. Would it make sense to document both APIs as viable alternatives with pros and cons in the README (currently the usage of RumBuilder requires a lot of reverse-engineering due to the absence of docs)?
   2. The OpenTelemetryRumBuilder API must be officially part of the stable API (as opposed to the disclaimer, that it is internal), right?
   3. What do you think about exposing the RumBuilder object of the OpenTelemetryRumInitializer DSL entry point class in some way to allow further customizations before the OpenTelemetryRum object is built?

Thanks for your feedback in advance!

[LikeTheSalad]

Hi @bencehornak

It's not something we've written anywhere AFAIK, but my understanding is that we'd like to go with option 1, to the point where RumBuilder would only be needed for very niche scenarios. Ideally, we should have everything needed in the DSL, is just that we don't know what's fully needed, and I don't think it's wise to add too much stuff upfront due to maintenance concerns, so feedback like this one is the missing piece before we decide to add more config options. So thanks for raising this issue!

Just to make sure, your use case needs to override all 3 parameters? Would that change if this disk buffering PR gets merged?

[bencehornak]

@LikeTheSalad I see, the approach sounds reasonable to me. I'd even add this statement to the README in some call-out, because I'm sure I'm not the only one being confused about the two syntaxes 😄

Regarding the disk buffering parameters, I'd rather introduce some higher-level parameters for the DSL config, which are more understandable to the layperson. The 3 referenced parameters (maxFileAgeForWriteMillis, minFileAgeForReadMillis, maxFileAgeForReadMillis) are very technical in nature (and on a side note, they are Java style numbers instead of the Kotlin Duration type with no suffix), and they require you to go layers deep beneath the surface of the Android SDK, to the disk-buffering lib's complex DESIGN document to fully grasp what's going on with these three parameters. No offense against the DESIGN document, it's probably the right choice to make these complexities visible to the embedding SDK, but my point is that to the users of the OTel Android SDK this low-level complexity should not be visible (unless somebody really knows what they are doing).

So I'd suggest to have easily understandable parameters, such as (the values are the ones I'm using for my use-case, I didn't mean to suggest new defaults):

diskBuffering {
  exportingFrequency(15.minutes)
  discardAfter(7.days)
}

The SDK should derive the low-level parameters from those then:

maxFileAgeForWriteMillis = exportingFrequency.inWholeMilliseconds
// minFileAgeForReadMillis won't be needed after https://github.com/open-telemetry/opentelemetry-java-contrib/pull/2897
maxFileAgeForReadMillis = discardAfter.inWholeMilliseconds

What do you think?

[LikeTheSalad]

Hi @bencehornak

I see what you mean about the low-level details doing more harm than good when it comes to ease of use. I like the approach you mentioned of going with exportingFrequency and discardAfter. The options are also granular enough so that we can add more if needed in the future (for example, an option to define the max disk space usage before starting to discard the oldest data). For very niche scenarios, users can still go with RumBuilder in case they need more than that.

If there are no concerns about this approach from @open-telemetry/android-approvers, then I think we can move forward with it.

[bencehornak]

small correction to my previous suggestion: strictly speaking, exportingPeriod sounds more correct, because frequency should be the reciprocal of this value (and have the unit 1/s).