Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

1.0.0 RUM initialization API proposals #556

Open
LikeTheSalad opened this issue Aug 26, 2024 · 16 comments
Open

1.0.0 RUM initialization API proposals #556

LikeTheSalad opened this issue Aug 26, 2024 · 16 comments
Labels
v1.0.0-required This is required to reach v1.0.0 (stability)

Comments

@LikeTheSalad
Copy link
Contributor

In the last Android SIG meeting we discussed potential API surface options before going GA/stable. Ideally, the API should be easy to use for users who aren't familiar with OTel in general and/or don't have special requirements for their apps and are happy with the defaults provided by the Android agent, yet flexible enough to accommodate users with more expertise around OTel and/or with specific requirements that aren't available by default.

It was also mentioned that, when it comes to API surface, it's easier to add features later as the need arises rather than having to remove/modify prematurely added ones in the future due to the breaking changes/deprecations/refactorings, etc, that could arise as a consequence.

Based on the above, we decided to try and come up with some API surface proposals, keeping in mind the 2 types of users mentioned earlier, and show how would each of them make use of the API for different use cases.

This issue is a placeholder for said proposals so please feel free to add yours below.

@LikeTheSalad LikeTheSalad added the v1.0.0-required This is required to reach v1.0.0 (stability) label Aug 26, 2024
@LikeTheSalad LikeTheSalad changed the title 1.0.0 API proposals 1.0.0 RUM initialization API proposals Aug 26, 2024
@LikeTheSalad
Copy link
Contributor Author

LikeTheSalad commented Aug 26, 2024

Proposal

Screenshot 2024-08-26 at 15 52 50

Description

  • OpenTelemetryRumBuilder gets the bare minimum, low-level setters/adders for it to work. Also, the non-serializable config options from OtelRumConfig are moved over to OpenTelemetruRumBuilder.
  • A new type, EndpointConfig is created to allow users to provide their exporter endpoint parameters. Being an interface it could potentially be queried multiple times to allow for "dynamic endpoint params" in the future, while it can also provide final values, as its default impl SimpleEndpointConfig should do.
  • A new type, AndroidAgentBuilder, is created in the higher-level module android-agent and will take care of configuring the lower-level builder OpenTelemetryRumBuilder with all the defaults that we consider should be present in most Android apps. This new type will take care of:
    • Adding the app name, app version, and environment name to the OTel resources.
    • Enabling disk buffering and configuring it.
    • Configuring a SessionManager (not currently present, though it could be similar to the interface created for JS RUM) implementation that would apply the same session logic we have right now (time-based).
    • Creating OtlpHttp exporter implementations for spans and logs using the parameters provided by the provided EndpointConfig object.
    • Allowing for customization of the created exporters.
    • (Potentially) installing the default instrumentations without having to rely on SPIs.
    • (Potentially) providing config options for the default instrumentations.

Use cases

I just want the defaults

// Application's build.gradle.kts

dependencies {
  implementation("io.opentelemetry.android:android-agent:[version]")
}
// Initialization
val rum = AndroidAgentBuilder()
  .setAppName("My app")
  .setAppVersion("0.0.0")
  .setEnvironmentName("debug")
  .setEndPointConfig(EndpointConfig.getDefault("http://myvendor.example.com"))
  .build()

I want the defaults and I want to log the exported spans

// Application's build.gradle.kts

dependencies {
  implementation("io.opentelemetry.android:android-agent:[version]")
}
// Initialization
val rum = AndroidAgentBuilder()
  .setAppName("My app")
  .setAppVersion("0.0.0")
  .setEnvironmentName("debug")
  .setEndPointConfig(EndpointConfig.getDefault("http://myvendor.example.com"))
  .addSpanExporterCustomizer { exporter -> MyLoggerSpanExporterDelegator.wrap(exporter) }
  .build()

I want the defaults plus some extra instrumentations

// Application's build.gradle.kts

dependencies {
  implementation("io.opentelemetry.android:android-agent:[version]")
}
// Initialization
val rum = AndroidAgentBuilder()
  .setAppName("My app")
  .setAppVersion("0.0.0")
  .setEnvironmentName("debug")
  .setEndPointConfig(EndpointConfig.getDefault("http://myvendor.example.com"))
  .asRumBuilder()
  .addInstrumentation(myInstrumentation)
  .build()

I want the defaults plus metrics

// Application's build.gradle.kts

dependencies {
  implementation("io.opentelemetry.android:android-agent:[version]")
}
// Initialization
val rum = AndroidAgentBuilder()
  .setAppName("My app")
  .setAppVersion("0.0.0")
  .setEnvironmentName("debug")
  .setEndPointConfig(EndpointConfig.getDefault("http://myvendor.example.com"))
  .asRumBuilder()
  .setMetricExporter(myMetricExporter)
  .build()

I want the defaults but I want to use Zipkin instead of Otlp

// Application's build.gradle.kts

dependencies {
  implementation("io.opentelemetry.android:android-agent:[version]")
}
// Initialization
val rum = AndroidAgentBuilder()
  .setAppName("My app")
  .setAppVersion("0.0.0")
  .setEnvironmentName("debug")
  .asRumBuilder()
  .setSpanExporter(zipkinSpanExporter)
  .setLogRecordExporter(zipkinLogRecordExporter)
  .build()

I don't want the defaults but disk buffering only and some instrumentations

// Application's build.gradle.kts

dependencies {
  implementation("io.opentelemetry.android:core:[version]")
}
// Initialization
val rum = OpenTelemetryRum.builder()
  .setDiskBufferingConfiguration(DiskBufferingConfiguration.builder().setEnabled(true).build())
  .addInstrumentation(myInstrumentation)
  .setSpanExporter(spanExporter)
  .setLogRecordExporter(logRecordExporter)
  .build()

I got my Otel instance, I just want to install some instrumentations

// Application's build.gradle.kts

dependencies {
  implementation("io.opentelemetry.android:core:[version]")
}
// Initialization
val rum = OpenTelemetryRum.builder(app, myOtelSdkInstance)
  .addInstrumentation(myInstrumentation)
  .addInstrumentation(otherInstrumentation)
  .build()

@breedx-splk
Copy link
Contributor

Thanks for taking the time to put this together @LikeTheSalad. I think this is a reasonable start. A few things I noticed while reading through:

  • setting the app name should be opt-in, and should serve to override the default app name detector that we have in AndroidResource.
  • does it ever make sense to have localhost in the endpoint? Maybe you're just using that as an example, in which case maybe something like myvendor.example.com is clearer?
  • in the I want the defaults and I want to log the exported spans section, I think you can remove the endpoint setting, since you're overriding the exporter.
  • 🌶️ I know you want parity with metrics, but I think we should prevent configuring metrics. It's going to be a problem.
  • I think it's a little "extra" or unnecessary/redundant to have both setters and customizers for exporters. I think the customizer is sufficient.
  • Do you expect to leak out the underlying OpenTelemetryRumBuilder instance via asRumBuilder()? Are we ok with that?

@marandaneto
Copy link
Member

I think it's a little "extra" or unnecessary/redundant to have both setters and customizers for exporters. I think the customizer is sufficient.

+1 here but I've not used the customizer so I'm not sure of the difference between them tbh.
If it still makes sense, should the add customizer methods be part of the OTelRumBuilder instead? Feels like this can be used there as well.

asRumBuilder() wasn't obvious to me if I had not read the proposal, maybe the AgentBuilder does the composition of all OTelBuilder methods as well? So you can just:

// Initialization
val rum = AndroidAgentBuilder()
  .setAppName("My app")
  .setAppVersion("0.0.0")
  .setEnvironmentName("debug")
  .setSpanExporter(zipkinSpanExporter)
  .setLogRecordExporter(zipkinLogRecordExporter)
  .build()

Those things should be optional (and detected automatically if not overwriten), but maybe it's just the example:

  .setAppName("My app")
  .setAppVersion("0.0.0")
  .setEnvironmentName("debug")

@LikeTheSalad
Copy link
Contributor Author

Thanks for your feedback @breedx-splk ! Regarding your comments:

setting the app name should be opt-in, and should serve to override the default app name detector that we have in AndroidResource.

This is fair enough, I guess one way to go about it would be to override the app name provided in AndroidResource only when the AndroidAgentBuilder.setAppName() function is called and also not raise any exceptions if it's never called.

does it ever make sense to have localhost in the endpoint? Maybe you're just using that as an example, in which case maybe something like myvendor.example.com is clearer?

It's only for purposes of the example, though I'll edit it just to make it clearer.

in the I want the defaults and I want to log the exported spans section, I think you can remove the endpoint setting, since you're overriding the exporter.

I'm not sure I follow this one. In the example, we're using addSpanExporterCustomizer to customize the SpanExporter provided by the agent, which will probably be a GrpcHttp exporter set up using the endpoint setting. So the customizer is not fully replacing it but instead it's expanding it to also print logs as each span is exported.

🌶️ I know you want parity with metrics, but I think we should prevent configuring metrics. It's going to be a problem.

I understand your concerns around metrics which is why I didn't add any metrics-related config into AndroidAgentBuilder, there's only customizers for spans and log exporters because the agent will only set those 2 exporters into OpenTelemetryRumBuilder. OpenTelemetryRumBuilder on the other hand, doesn't know about any signal exporter impl, it just has a setter for each signal so that you can configure it from scratch, though the agent will only configure logs and spans.

I think it's a little "extra" or unnecessary/redundant to have both setters and customizers for exporters. I think the customizer is sufficient.

The setter and the customizer are in different places, the setter is in OpenTelemetryRumBuilder which is just a place to put all the "raw" stuff from scratch, whereas the customizers are in AndroidAgentBuilder because it creates the default exporters impls (for spans and logs) from scratch internally and then it sets them into OpenTelemetryRumBuilder using OpenTelemetryRumBuilder's setters, and before doing so, users can customize them to add some extra functionality to these default agent impls if needed.

Do you expect to leak out the underlying OpenTelemetryRumBuilder instance via asRumBuilder()? Are we ok with that?

I added asRumBuilder() to allow users to override parts of the OpenTelemetryRumBuilder instance that's internally created in AndroidAgentBuilder. So it's essentially a way for "power users" to get some defaults while also being able to tweak some parts if needed. I'm guessing that if we do a very good job when it comes to choosing the defaults that we set in the agent, then users won't need to call asRumBuilder() to override anything which makes me think that it shouldn't cause any issues. I see it as more of a handy way to deal with some specific use cases without having to fully configure OpenTelemetryRumBuilder from scratch.

Please let me know if you have further questions.

@LikeTheSalad
Copy link
Contributor Author

+1 here but I've not used the customizer so I'm not sure of the difference between them tbh.
If it still makes sense, should the add customizer methods be part of the OTelRumBuilder instead? Feels like this can be used there as well.

Tbh it's not fully clear to me the use of a customizer for exporters, given that they are just contracts and their impls tend to be immutable, for example, I can understand a customizer for a builder, such as the ones for tracer/logger builders, or even a customizer for a Resource, because you can merge it with another Resource of yours to provide both's set of attrs in a single one, though the exporter customizer is not clear to me, it most likely comes from the fact that they also have them here in that Java SDK extension, so I'm wondering what could have been the reasons to add them there.

The only use case I can find (unless I'm missing something) is to wrap it around another exporter that decorates it, such as the example I provided where a "span-logging exporter" is wrapping the real span exporter to log spans as they get exported. Though apart from that use case, the use for the customizer seems to be to just replace the provided one by default, which is essentially using it as a setter. The difference between both approaches would be that, when a "setter" is used, it means that nothing needs to be there by default, whereas when a "customizer" is used, we'd have to set some default first for it to be sent out to get "customized" later. For the customizer approach, I think it'd make sense that said "default impl" is an actual, working one, otherwise we'd be passing around "noop" impls for them to get "customized" which doesn't make much sense to me.

asRumBuilder() wasn't obvious to me if I had not read the proposal, maybe the AgentBuilder does the composition of all OTelBuilder methods as well?

I think that works too 👍 I just added customizers in the agent to try and keep some of the existing functionality, and also in case some user would want the default exporters but maybe would want to decorate them as in the example "I want the defaults and I want to log the exported spans".

Those things should be optional (and detected automatically if not overwriten), but maybe it's just the example:

I agree, if we have a reliable way to get those values automatically then those setters would only be used as a handy way to override them if needed. I'm also up for not providing those setters at all at first if we're confident about the values we can set there automatically, and then only adding them if someone ever asks for the option of overriding them.

Thanks for your feedback, @marandaneto

@marandaneto
Copy link
Member

marandaneto commented Sep 10, 2024

You can read the app's name and version from the package, so it's possible.
Eg:
https://developer.android.com/reference/android/content/pm/PackageInfo#versionCode
https://developer.android.com/reference/android/content/pm/PackageInfo#versionName
https://developer.android.com/reference/android/content/pm/PackageInfo#packageName
Apps name eg: context.applicationInfo.loadLabel(context.packageManager)

The EnvironmentName is a bit trickier because it depends on the generated class BuildConfig.DEBUG, BuildConfig.BUILD_TYPE, etc, so better to have it part of the builder

Note that a lot of things depend on the application's context, so when starting the agent, you'd need to pass the instance of the app's context.

@bidetofevil
Copy link
Contributor

I like the overall idea of the separation of Agent config vs the OTel SDK/Instrumentation config (via OpenTelemetryRum). Some additional feedback based on the existing comments:

  • asRumBuilder() as a method on the build is a bit confusing for me. If the idea is to return some defaults that can then be overwritten by power users, why not expose a factory method that returns an instance of OpenTelemetryRumBuilder that can then be tweaked and set back on the AndroidAgentBuilder? Or just have the default factory method return with the defaults populated? So to tweak any of it, you'd call setOpenTelemetryRumBuilder() or something explicitly

  • +1 to doing as much preconfiguration as possible to detect resource attributes instead of it being required to be set. But the one issue with using PackageManager at SDK startup is that it could be slow given that it's a system call, so blocking startup on that could be problematic in some situations. Being able to opt out of that by allowing programatic setting is probably a good solution to provide convenience for a small performance hit.

  • My only contribution to the customizer/exporter-setter discussion is that if we want to simply keep the customizer API, we need to document the hell out of it because it's not obvious that you can use it to be customize the existing default one AND replace it - or do both. Having some examples/recipes to do that would go a long way into clarifying that.

In addition, I'm weary about exposing method on AndroidAgentBuilder that effectively configures what is set on the OpenTelemetryRumBuilder. I feel like anything that effectively passes the config settings through should ideally just live in OpenTelemetryRumBuilder, so anything involving the exporter configuration/overriding.

It makes more sense to me to use the OpenTelemetryRumBuilder instance associated with that AndroidAgentBuilder to do that type of configuration. Having the outer object essentially pass that through could muddle the expectations of what the final builder generates given you can modify the same things on both.

Setting the endpoint via the Agent builder is a compromise so you don't always have to tweak OpenTelemetryRumBuilder, but I'm hoping cases like that are few and far between, and the fact that it will modify all the exporters set on the OpenTelemetryRumBuilder is clearly communicated.

For the advanced used case, it would be more like:

val rumBuilder = OpenTelemetryRum.builder()
  .setSpanExporter(zipkinSpanExporter)
  .setLogRecordExporter(zipkinLogRecordExporter)
  .addInstrumentation(...)

val agent = AndroidAgentBuilder()
  .setAppName("My app")
  .setAppVersion("0.0.0")
  .setEnvironmentName("debug")
  .setEndPointConfig(...)
  .setOpenTelemetryRumBuilder(rumBuilder)
  .build()

@LikeTheSalad
Copy link
Contributor Author

LikeTheSalad commented Sep 11, 2024

Thanks for your feedback, @bidetofevil. I think the approach of passing an instance of OpenTelemetryRumBuilder to the agent builder could also work. For your example, I'd only remove this line of code: .setEndPointConfig(...) because that endpoint config is more of a handy way for users to connect to their backends without having to create the exporters themselves (as the agent would create those instead using the info provided in the endpoint config) though in your example you're manually setting the exporters in the OpenTelemetryRumBuilder instance already so there's nothing else that the agent should do about exporters, if anything, the agent would just override yours which is probably not the expected behavior from your example.

After going through the feedback it seems like the option of "getting some defaults and then being able to override some things by directly touching an OpenTelemetryRumBuilder instance", either via asRumBuilder() or by passing it to the agent instead, seems to be a bit of a complicated topic, and while I'm not married to the asRumBuilder() idea because I think that @bidetofevil's option could work as well, I just took some time to think how often would this use case happen and the reality is that I'm not sure. So what I'm trying to say is that maybe we don't have to address it now, and maybe we can just have the agent to configure an OpenTelemetryRum instance with all of our recommended settings and, if some user doesn't want our defaults, then they would just have to configure their own OpenTelemetryRumBuilder from scratch, and only if we ever get some people complaining about not wanting to do all the OpenTelemetryRumBuilder configuration from scratch because maybe all they wanted to change was one exporter for example, then we can take a look at what mechanisms we could provide to enable that in the future.

@bidetofevil
Copy link
Contributor

Giving folks SOME way to configure is already pretty powerful. Making it easy, that's a nice to have, and something I'd argue we don't need, so long as they can programatically get one that contains our defaults.

@bidetofevil
Copy link
Contributor

Per today's discussion, I cobbled together the simplified config API that was talking about via the demo app:

main...bidetofevil:opentelemetry-android:fake-api

The idea is to expose a factory method that creates an OpenTelemetryRum instance where the input are optional value you want to apply to the builder before you build. We can expose additional methods to return instances of objects used as parameters so that folks can apply changes on top of our defaults. Those default-getting methods can have parameters too if they require inputs.

Let me know if that makes sense.

@bidetofevil
Copy link
Contributor

I redid the sample by modifying the agent and core code and using the new factory method in the demo: main...bidetofevil:opentelemetry-android:simple-setup

The general idea is that we expose our defaults as pieces that can be used selectively to do advanced config. Those that don't need that level of customization will just pass in the primitives in the parameter list they want to set and be on their way.

The only difference from Cesar's proposal is just a more Kotlin-y syntax.

@bidetofevil
Copy link
Contributor

Rewriting Cesar's examples to use the defaults or apply customizations on top of them:

I just want the defaults

val rum = createOpenTelemetryRum(
  appName = "My app",
  appVersion = "0.0.0",
  environmentName = "debug",
  endpointConfig = EndpointConfig("http://myvendor.example.com")
)

I want the defaults and I want to log the exported spans

val builder = Defaults.getDefaultRumBuilder().apply {
  addSpanExporterCustomizer { 
    exporter -> MyLoggerSpanExporterDelegator.wrap(exporter) 
  }
}

val rum = createOpenTelemetryRum(
  appName = "My app",
  appVersion = "0.0.0",
  environmentName = "debug",
  endpointConfig = EndpointConfig("http://myvendor.example.com"),
  builder = builder
)

I want the defaults plus some extra instrumentations

val builder = Defaults.getDefaultRumBuilder().apply {
  addSpanExporterCustomizer { 
    exporter -> MyLoggerSpanExporterDelegator.wrap(exporter) 
  }
  addInstrumentation(myInstrumentation)
}

val rum = createOpenTelemetryRum(
  appName = "My app",
  appVersion = "0.0.0",
  environmentName = "debug",
  endpointConfig = EndpointConfig("http://myvendor.example.com"),
  builder = builder
)

I want the defaults plus metrics

val builder = Defaults.getDefaultRumBuilder().apply {
  addSpanExporterCustomizer { 
    exporter -> MyLoggerSpanExporterDelegator.wrap(exporter) 
  }
  setMetricExporter(myMetricExporter)
}

val rum = createOpenTelemetryRum(
  appName = "My app",
  appVersion = "0.0.0",
  environmentName = "debug",
  endpointConfig = EndpointConfig("http://myvendor.example.com"),
  builder = builder
)

I want the defaults but I want to use Zipkin instead of Otlp

val builder = Defaults.getDefaultRumBuilder().apply {
  addSpanExporterCustomizer { 
    exporter -> MyLoggerSpanExporterDelegator.wrap(exporter) 
  }
  setSpanExporter(zipkinSpanExporter)
  setLogRecordExporter(zipkinLogRecordExporter)
}

val rum = createOpenTelemetryRum(
  appName = "My app",
  appVersion = "0.0.0",
  environmentName = "debug",
  endpointConfig = EndpointConfig("http://myvendor.example.com"),
  builder = builder
)

@LikeTheSalad
Copy link
Contributor Author

Thank you for taking the time, @bidetofevil !

Your example has helped me to get a much clearer idea of what you meant. So to make sure I got it all correct, I’ll try to point out what I understood from a comparative point of view with the proposal I provided earlier.

Both approaches serve as a "higher-level" config layer to OpentTelemetryRumBuilder from the agent module side, only that your approach attempts to do all the configuration and default rum agent initialization in a function that contains parameters with default values, as opposed to using the builder pattern. Also, the option I proposed is meant to handle its OpentTelemetryRumBuilder instance internally (and only exposing it, if needed, to add changes on top of what the agent builder has previously configured), whereas your approach expects to receive a preconfigured instance of OpentTelemetryRumBuilder to address use cases that require either adding extra functionality to the default agent behaviour, or overriding existing functionality (I have some comments below regarding the “overriding” functionality details).

If the above is correct, I think I’m fine to go with either approach because to me the most important part is to have a separation of concerns between what the core is capable of doing vs our recommended way of doing things, and the other most important part would be to make it easy to use, and your approach seems to cover both things. There are a couple of concerns that come to my mind though, which I’m not sure if are or will ever become a problem, some of them are related to comparisons between the function and the builder approaches, and I’d like to know your thoughts on them:

  • Based on the second example you shared, "simple-setup", it seems like users would need to be aware of the Defaults type in case they need to override some config options, as well as the createOpenTelemetryRum function which lives outside of it, where those configs will be used. I’m concerned about the different places/types/functions that our users would need to be aware of to handle all use cases.
  • The createOpenTelemetryRum function parameters are currently application, appName, appVersion, and rumBuilder, where the application one is the only one that must be provided by the caller. I think that works well based on what’s needed right now, however, I’m guessing more parameters will be added in the future (like the endpoint info for example) and that will mean that this function will become bigger in time with a lot of parameters for users to scan through, do you see that as a potential issue when it comes to the "ease of use" and/or the maintainability of the tool?
  • This might never be an issue but I’d like to mention it in case it wasn’t considered and/or in case I’m missing an existing solution for it. I think one advantage of using the builder pattern is that the usage might be more fluent in some cases, for example, if we ever need to add a config parameter for something that can be added multiple times (such as customizers for example) it seems handy to have an "adder" method that can be called multiple times, whereas, with the function params approach, users would have to first build a list and then pass it as param. Probably not a big deal though, especially with Kotlin’s helpers such as listOf, but still handy depending on how many lists might be needed.
  • Do we need to pass an instance of OpentTelemetryRumBuilder to createOpenTelemetryRum right now? I’m asking because it seems to be there to cover the "advanced use cases" that I was trying to address with asRumBuilder() which we’re not sure we’ll need, also, I think there’s a potential issue with doing so in terms of intuitiveness. The issue is based on the example that you’ve shared under "I want the defaults but I want to use Zipkin instead of Otlp". In that scenario, the user first creates their rum builder instance and sets their Zipkin exporters, but then it passes the builder over to the createOpenTelemetryRum function alongside an instance of EndpointConfig. In that scenario what should be expected? The agent to "add" its default exporters using the params from EndpointConfig so that both, the user-provided exporters and the agent ones coexist and export things in parallel? Or the agent to override the user-provided exporters? Or the agent to check if the rum builder has some exporters already set, and if so, avoid overriding them? - I guess this point can be applied to other configurable aspects of the rum builder, since the rum builder is created first. It would be a bit complicated to decide "what overrides what", because the user might want its own preconfigured builder to maintain its initial state and then the agent to "expand on it", but there might be cases where there’s only one object to be set in the builder which the agent will just override as it visits the builder after the user has passed it. I guess another option would be to tell users to create a builder instance, pass it to the agent, and then after calling the createOpenTelemetryRum function they can safely add their configs into the builder? But I don’t think that’s a good idea for many reasons. It probably would be better if createOpenTelemetryRum returns OpenTelemetryRumBuilder rather than OpenTelemetryRum, that way the agent does its thing first, and then users can override configs on the returned builder afterward if needed. That approach of returning OpenTelemetryRumBuilder might also help mitigate the need to have separate "Defaults" functions, as the returned OpenTelemetryRumBuilder will already be a fully configured instance of the builder with all the things we decide to add in the agent, so we could remove getDefaultRumBuilder as well as getDefaultConfig since the config can be another parameter of createOpenTelemetryRum, and also the diskbuffering config would be removed from the config object based on the diagram I shared above to ensure that all the "configs" from OtelRumConfig are serializable.

@bidetofevil
Copy link
Contributor

bidetofevil commented Sep 19, 2024

Conceptually, it's really very similar to what you had originally proposed, @LikeTheSalad. The two main differences that are see are:

  1. Whether this is implemented as a Java Builder pattern idiom vs a Kotlin factory method using named and default arguments; and
  2. You have an object your custom parameters first accumulates the result of the method calls on, e.g. you can call a setter/adder multiple times, and only after that is done do you return the OpenTelemetryRumBuilder instance for further customization if folks so desire.

In both, you have a simplified API that abstracts over a more complicated one, allowing certain attributes to be overwritten, while using others that are set by default, allowing the simple use case to not have to worry about the more complicated one. The stylistic difference as well as the accumulation/application order is really where they diverge.

So, lets tackle the first point first: style.

To answer you questions, no, you don't need to be aware of the defaults or OpenTelemetryRumBuidler to use this. The only required parameter in createOpenTelemetryRum() is application - everything is optional. So if you don't need to configure anything on there, the following works just fine:

val rum = createOpenTelemetryRum(
  application = application,
  appName = "myApp"
)

For the use case that don't require any advanced configuration (we are saying 90% of folks should be able to use the simplified API to generate their OpenTelemetryRum instance), the main difference is really Java builder vs Kotlin function with default parameters.

In terms of complexity as the class gets bigger, it's really the same, just in different places: if there are 10 things to set, there'd be 10 setters on the builder, vs 10 optional arguments you can specify that have defaults. In the case you don't need to use only a couple of them, it'll look similar:

val rum = AndroidAgentBuilder()
  .setFoo(foo)
  .setBar(bar).
  .build()

vs

val rum = createOpenTelemetryRum(
  foo = foo,
  bar = bar
)

And if you need to call all 10, is there a difference? We're still dealing with 10 variables that can be applied to build a thing. In Java, when you have to specify all the arguments in order, that can be really annoying and error prone, so folks use overloads to reduce the number of parameters. But in Kotlin, with default arguments, you no longer have to do that.

Also, with named arguments, you don't need to specify the parameters in order as long as you also specify the parameter name. So even if it's the 7th parameter you want to override, you don't need to treat it any differently than if it's the first one.

So really, this comes down to what you prefer. Since this is an Android project, I would personally go with the more Android-y approach on this front.

In terms of having an object we can accumulate setter calls on, the only then returning the builder to be customized, we can tweak the design a bit and get the same result: create a different object for the simplified API and allow it to return an OpenTelemetryRumBuilder or the Rum instance itself.

So the simple case becomes:

val rum = AndroidAgentBuilder(
  foo = foo,
  bar = bar
).buildRum()

You can even invoke it more like a Java builder:

val agentBuilder = AndroidAgentBuilder()
val rum = agentBuilder.apply {
  foo = foo
  bar = bar
}.buildRum()

And the more advanced use case will be:

val rum = AndroidAgentBuilder(
  foo = foo,
  bar = bar
).toRumBuilder()
  .addSpanExporterCustomizer(...)
  .build()

I've added this commit to illustrate that.

So really, I think this comes down to the stylistic difference and which one y'all prefer. If the factory method is not flexible enough, we can turn it into what is effectively a builder class with settable vars declared on the constructor that can be turned into a RumBuilder or Rum instance. If we want more fancy logic in there, we can even override the setters.

I hope I've gotten my point across that I would prefer a more Kotlin-y syntax and that we can achieve the stated objectives using it 😅 But ultimately, you and @breedx-splk are the maintainers, so y'all should decide the direction you want this to go. Now that I've said all this, I'm happy which ever way you decide to go (ok, I will be slightly happier one way, but I'm totally with it not being that lol)

@LikeTheSalad
Copy link
Contributor Author

LikeTheSalad commented Sep 23, 2024

I've put together a prototype that covers the agent API with:

  • A Kotlin factory method that returns OpenTelemetryRumBuilder, which to me it seems to be the simplest way of using the agent as there's only one method that users would need to be aware of. It also contains config params for the default instrumentations.
  • Following the diagram, it removes some API surface from OpenTelemetryRumBuilder.
  • Following the diagram, It moves the "unserializable" config params from OtelRumConfig into OpenTelemetryRumBuilder.
  • Following the diagram (kind of, as the interface initially named SessionManager is now changed to SessionProvider here) it sets the SessionProvider as the only session-related type in core, and moves all of our session-related types to the agent so that the existing session handling tools (the ones that create sessions based on a timeout) are provided by the agent, and "power-users" can provide their own session handling logic directly to core, if needed.
  • Following the diagram, it creates EndpointConfig and a default impl that provides HTTP endpoints.
  • Adds the OTLP exporters dependency to the agent module and makes the agent to automatically set otlp-http span and log exporters that use the provided endpoint config params.

Here we can see how this agent API is used in our demo app.

@LikeTheSalad
Copy link
Contributor Author

Hi everyone, I've created this PR with the prototype mentioned in my previous comment to make it easier to add comments on different parts of it, please have a look when you get a chance!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v1.0.0-required This is required to reach v1.0.0 (stability)
Projects
None yet
Development

No branches or pull requests

5 participants
@bidetofevil @marandaneto @LikeTheSalad @breedx-splk and others