open-telemetry / opentelemetry-android

OpenTelemetry Tooling for Android
Apache License 2.0
147 stars 33 forks source link

1.0.0 RUM initialization API proposals #556

Open LikeTheSalad opened 1 month ago

LikeTheSalad commented 1 month ago

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 commented 1 month ago

Proposal

Screenshot 2024-08-26 at 15 52 50

Description

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 commented 4 weeks ago

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:

marandaneto commented 4 weeks ago

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 commented 4 weeks ago

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 commented 4 weeks ago

+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 commented 4 weeks ago

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 commented 4 weeks ago

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:

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 commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

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

https://github.com/open-telemetry/opentelemetry-android/compare/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 commented 2 weeks ago

I redid the sample by modifying the agent and core code and using the new factory method in the demo: https://github.com/open-telemetry/opentelemetry-android/compare/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 commented 2 weeks ago

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 commented 2 weeks ago

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:

bidetofevil commented 2 weeks ago

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 commented 2 weeks ago

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

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

LikeTheSalad commented 10 hours ago

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!