-
Notifications
You must be signed in to change notification settings - Fork 42
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
Comments
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:
|
+1 here but I've not used the customizer so I'm not sure of the difference between them tbh.
Those things should be optional (and detected automatically if not overwriten), but maybe it's just the example:
|
Thanks for your feedback @breedx-splk ! Regarding your comments:
This is fair enough, I guess one way to go about it would be to override the app name provided in
It's only for purposes of the example, though I'll edit it just to make it clearer.
I'm not sure I follow this one. In the example, we're using
I understand your concerns around metrics which is why I didn't add any metrics-related config into
The setter and the customizer are in different places, the setter is in
I added Please let me know if you have further questions. |
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 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.
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".
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 |
You can read the app's name and version from the package, so it's possible. The EnvironmentName is a bit trickier because it depends on the generated class 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. |
I like the overall idea of the separation of Agent config vs the OTel SDK/Instrumentation config (via
In addition, I'm weary about exposing method on It makes more sense to me to use the Setting the endpoint via the Agent builder is a compromise so you don't always have to tweak For the advanced used case, it would be more like:
|
Thanks for your feedback, @bidetofevil. I think the approach of passing an instance of 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 |
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. |
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 Let me know if that makes sense. |
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. |
Rewriting Cesar's examples to use the defaults or apply customizations on top of them: I just want the defaults
I want the defaults and I want to log the exported spans
I want the defaults plus some extra instrumentations
I want the defaults plus metrics
I want the defaults but I want to use Zipkin instead of Otlp
|
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 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:
|
Conceptually, it's really very similar to what you had originally proposed, @LikeTheSalad. The two main differences that are see are:
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
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 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:
vs
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 So the simple case becomes:
You can even invoke it more like a Java builder:
And the more advanced use case will be:
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) |
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. |
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! |
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.
The text was updated successfully, but these errors were encountered: