-
Notifications
You must be signed in to change notification settings - Fork 41
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
[Do not merge] Agent api prototype #624
base: main
Are you sure you want to change the base?
[Do not merge] Agent api prototype #624
Conversation
fun createRumBuilder( | ||
application: Application, | ||
otelRumConfig: OtelRumConfig = OtelRumConfig(), | ||
endpointConfig: EndpointConfig = EndpointConfig.getDefault("http://localhost"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea of having a simple and direct configuration approach, but we should still make it easy for users to configure separate endpoints for logs and spans. We can't assume that they're the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, yeah it makes sense. Right now the EndpointConfig interface provides a separate getter for each, although it's not the most user-friendly approach having to implement it just to provide different URLs. Do you have any preferences of what could be a nice way to address this? The one thing I was thinking right now was to remove the EndpointConfig
param and instead add string params for each url, and maybe a "defaultUrl" one that gets overridden if there's a value provided for a specific signal url.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Separate for each and fallback to the default sounds good. There aren't so many signals that it would be onerous to specify the URL twice on init if they have a custom one
|
||
fun createRumBuilder( | ||
application: Application, | ||
otelRumConfig: OtelRumConfig = OtelRumConfig(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as a user, when I see that I can pass in a "rum config" object, I instantly wonder what's in there. ...but then I see this other stuff alongside it and wonder even more why some of this configurable stuff is in the config but some of it is passed directly to the builder. Seems inconsistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's true, although I'd argue that we're already in a similar situation by having an OtelRumConfig
object passed to the OpenTelemetryRumBuilder
which itself has some config options too! 😅
At some point, I was thinking about OtelRumConfig
as a place for flags so that we could switch things on/off and also provide a serializable part of the config which might be useful for debug logs, and then the rest of config options in the builder as a place for more complex objects... Although maybe we would also like to show something else in the debug logs apart from just flags, in which case I'm not sure how can I justify having both tbh. What's your view on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this more of a concern about what config goes into what object? It feels OK to have both if we can clearly separate which configs go into which object. But yeah, if OtelRumConfig is public, I would assume that folks can pass in an instance configured however they want and assume that it'll work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some suggestions but I think this is an incremental improvement that we can build on.
Between this PR and this, a larger discussion can probably be had about separating the project into layers so that they can be individually configured and used. There's still a bit too much mixing of concerns between the Agent, the platform services it provides, the SDK instance it wraps, and the instrumentation that depend directly on one or more of these layers. Until the separation is more formal, it'll be hard to get away from some of these challenges of partial configuration and reuse.
private val activityLifecycleInstrumentation by lazy { | ||
AndroidInstrumentationLoader.getInstrumentation( | ||
ActivityLifecycleInstrumentation::class.java, | ||
)!! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
checkNotNull()
is preferred if you have to verify something is non-null
An alternative is to allow these to be null and handle it downstream if we can envision a case where the agent can work without some of these things
|
||
fun createRumBuilder( | ||
application: Application, | ||
otelRumConfig: OtelRumConfig = OtelRumConfig(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this more of a concern about what config goes into what object? It feels OK to have both if we can clearly separate which configs go into which object. But yeah, if OtelRumConfig is public, I would assume that folks can pass in an instance configured however they want and assume that it'll work.
fun createRumBuilder( | ||
application: Application, | ||
otelRumConfig: OtelRumConfig = OtelRumConfig(), | ||
endpointConfig: EndpointConfig = EndpointConfig.getDefault("http://localhost"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Separate for each and fallback to the default sounds good. There aren't so many signals that it would be onerous to specify the URL twice on init if they have a custom one
import io.opentelemetry.sdk.common.Clock | ||
import java.time.Duration | ||
|
||
object AndroidAgent { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is effectively a static singleton. Are we OK with this (especially for testing), or do we want to wrap all the state up in its own object so we can replace it at runtime?
Prototype that should cover what's being discussed here.
Here we can see how this agent API is used in our demo app.