Initialization hooks #196
Initialization hooks #196
Conversation
| @@ -263,9 +265,23 @@ public OpenTelemetryRum build() { | |||
| SdkPreconfiguredRumBuilder delegate = | |||
| new SdkPreconfiguredRumBuilder(application, sdk, sessionId); | |||
| instrumentationInstallers.forEach(delegate::addInstrumentation); | |||
|
|
|||
| notifyInitializationEnd(); | |||
There was a problem hiding this comment.
Can one of the methods above throw an exception? if not all good, otherwise maybe the notifyInitializationEnd call should be under a finally block, or even having a error hook.
There was a problem hiding this comment.
That's a good point, @breedx-splk has a better understanding of all the processes that happen during the RUM initialization, so I think we should wait for his input on this. However, atm what I can say is that I don't think the RUM init should throw any exceptions or should silently fail either, maybe some components might fail to initialize (such as the disk buffering for example) but so far that's not a reason to stop the overall RUM initialization. Though it's definitely a good conversation to have on what (and how) should we notify our users in case something fails.
| private boolean networkChangeMonitoringEnabled = true; | ||
| private final List<InitializationListener> initializationListeners = new ArrayList<>(); |
There was a problem hiding this comment.
Should this list be thread-safe or not expected/allowed to be called from multiple threads?
There was a problem hiding this comment.
I can't think of a valid use case where it would be needed to access it from multiple threads atm. The writing should only happen before initializing RUM, and the reading should only be done inside our RUM init code which is currently happening in a single thread. Though there's a valid case to be made on how far we should take "defensive programming" practices. I wonder if OTel has some rules on it.
There was a problem hiding this comment.
Yeah, most likely what could happen is someone iterating over the list and trying to mutate the list (removing by a specific type or something), it'd throw an exception, it'd not if its thread-safe (no iterable needed).
I don't see the need if not this use case as well since the single thread only during the init.
| } | ||
| } | ||
|
|
||
| public List<InitializationListener> getInitializationListeners() { |
There was a problem hiding this comment.
A comment that is an unmodifiable list would be nice, maybe people will try to mutate directly from the list instead of calling add, etc.
Maybe adding a remove method as well? not sure if really needed.
There was a problem hiding this comment.
I like the idea of adding a comment to explain it. I think we could add a remove method in the future if needed, but so far I don't have a use case in mind for it.
instrumentation/src/main/java/io/opentelemetry/android/config/OtelRumConfig.java
Outdated
Show resolved
Hide resolved
...ion/src/main/java/io/opentelemetry/android/instrumentation/startup/InitializationListener.kt
Outdated
Show resolved
Hide resolved
…entation/startup/InitializationListener.kt Co-authored-by: Manoel Aranda Neto <[email protected]>
| /** | ||
| * Called when the RUM initialization starts. | ||
| */ | ||
| fun onStart() |
There was a problem hiding this comment.
We have found it helpful in upstream to have default no-op bodies for some of these, so that implementations only need to override the parts they actually care about. What do you think?
There was a problem hiding this comment.
Sounds good! I tend to forget that interfaces' default functions exist 😅
| * Called when the RUM initialization ends. | ||
| * @param openTelemetry - The initialized OpenTelemetry instance. | ||
| */ | ||
| fun onEnd(openTelemetry: OpenTelemetry) |
There was a problem hiding this comment.
What if this was onEnd(rum: OpenTelemetryRum) instead? The otel instance (like what you have here) as well as the session id are both obtainable from the OpenTelemetryRum instance.
There was a problem hiding this comment.
Makes sense, though now that I think about the whole thing after the proposal I wrote yesterday, it seems to me that these hooks might overlap with instrumentation installers. So maybe it's not needed to add another, similar mechanism if we decide to make that one public?
|
@LikeTheSalad What do you want to do with this one? |
It seems like it might no longer be needed after the changes we discussed here. Since in order to add instrumentations from outside I'm guessing we should expose some sort of API that would allow registered instrumentations to get notified when the OpenTelemetry instance is ready to be used, such as the existing InstrumentedApplication one (which is essentially what this PR was trying to do). What do you think? |
|
Ok I think I'm ok either sitting on this or closing it until we see how init events might fall out in the new mode. Agreed that there's probably going to be an api needed. :) Just wanted to check and see what your opinion on it was. 👍🏻 |
|
Closing as this is no longer needed. |
Related to #195