Initialization events #397
Conversation
| * @param callback - A callback that receives the OpenTelemetry SDK instance. | ||
| * @return this | ||
| */ | ||
| public OpenTelemetryRumBuilder addOtelSdkReadyListener(Consumer<OpenTelemetrySdk> callback) { |
There was a problem hiding this comment.
Consumer is API 24+ only, but I guess its desugared if isCoreLibraryDesugaringEnabled enabled right
There was a problem hiding this comment.
I think so, yeah...and it's used elsewhere.
| import io.opentelemetry.sdk.trace.export.SpanExporter | ||
| import java.time.Instant | ||
| import java.util.function.Consumer | ||
| import java.util.function.Supplier |
There was a problem hiding this comment.
...up/src/main/java/io/opentelemetry/android/instrumentation/startup/SdkInitializationEvents.kt
Outdated
Show resolved
Hide resolved
| * @param callback - A callback that receives the OpenTelemetry SDK instance. | ||
| * @return this | ||
| */ | ||
| public OpenTelemetryRumBuilder addOtelSdkReadyListener(Consumer<OpenTelemetrySdk> callback) { |
There was a problem hiding this comment.
This reminds me of what I tried to do in this other PR, which so far seemed like it was superseded by the Instrumentation API, as it provides a way for instrumentations to get notified when the SDK is ready to be used. What use case is this method trying to solve?
There was a problem hiding this comment.
There's similarity for sure. It's a straightforward means for a component (in this case, the init events) to get notified immediately after the otel sdk has been initialized. In this case, the init events have to be buffered until they can be emitted. Without this register/notify mechanism, there's no way to know when it's "safe" to emit the events.
I made it a bit generic, because I thought that there could be other uses for this. However, there's the YAGNI school of thought, which might suggest that I remove this and just explicitly call a flush or emitNow method on the InitEvents instead. I don't feel strongly either way.
| // with a body, but this is ultimately clunky/fragile. | ||
| eventBuilder.put("config", event.body) | ||
| } | ||
| eventBuilder.emit() |
There was a problem hiding this comment.
I think I misunderstood the goal of the InitializationEvents at first as I thought of it as a set of callbacks for ourselves and our users to keep track of the state of the RUM initialization in code, though I wasn't expecting this feature to actually generate telemetry, which brings a couple of questions. Do we need to generate telemetry for this use-case (or is it enough to provide the callbacks and then our users can do anything they'd wish there, for example sending their own events if needed)? Can users opt out of it? Do we need to define the events in the semantic conventions first? Since this feature is automatically generating telemetry, shouldn't it be an AndroidInstrumentation implementation?
There was a problem hiding this comment.
Do we need to generate telemetry for this use-case (or is it enough to provide the callbacks and then our users can do anything they'd wish there, for example sending their own events if needed)?
Generating events that track milestones in the agent startup and configuration is the main goal with this. These events have been extremely helpful when looking at startup times and understanding how long the agent itself takes to reach certain states in the initialization. I don't see any benefit in exposing these hooks to user/application code. What would a user do at any of these times? They can't send their own events, because the sdk has not been initialized.
Can users opt out of it?
Yes, there is config.disableSdkInitializationEvents().
Do we need to define the events in the semantic conventions first?
I don't think it has to happen first, but yes, it would be correct to have defined semantic conventions for these.
Since this feature is automatically generating telemetry, shouldn't it be an AndroidInstrumentation implementation
Almost certainly yes, but that didn't exist when I wrote it! :) Moving so fast over here....heh. We can pick that up as a follow-on item I hope.
There was a problem hiding this comment.
They can't send their own events, because the sdk has not been initialized.
We technically can't either :) though there's always a way... In any case, I get the idea now, it's certainly helpful to get this info for debugging the Android agent 👍
I'm generally hesitant about producing telemetry out of the box because it assumes that we know what every user of this lib will want to emit, which sounds a bit opinionated, so it's nice that we provide a way to opt-out.
I don't think it has to happen first, but yes, it would be correct to have defined semantic conventions for these.
This use case seems to be pretty specific to this agent, so I'm fine with moving forward without adding these events to the semconv first, however, I'd like to have some sort of litmus test to know when it's fine to send telemetry without conventions and when it's not, because we're going to need to be prepared when someone asks about it (unless it's a common practice around OTel impls to send telemetry that's not yet defined in the conventions, in which case then we probably shouldn't stress too much about it).
Almost certainly yes, but that didn't exist when I wrote it! :) Moving so fast over here....heh. We can pick that up as a follow-on item I hope.
Sounds good! It seems like you need to have this feature available before the instrumentation API work is done, so I think it's fine to move forward as is for now. I'm planning to adapt all the existing automatically generated telemetry to the new API anyway, so I think I can pick this one up too.
...up/src/main/java/io/opentelemetry/android/instrumentation/startup/SdkInitializationEvents.kt
Outdated
Show resolved
Hide resolved
...up/src/main/java/io/opentelemetry/android/instrumentation/startup/SdkInitializationEvents.kt
Outdated
Show resolved
Hide resolved
...up/src/main/java/io/opentelemetry/android/instrumentation/startup/SdkInitializationEvents.kt
Outdated
Show resolved
Hide resolved
…/instrumentation/startup/SdkInitializationEvents.kt Co-authored-by: Manoel Aranda Neto <[email protected]>
…/instrumentation/startup/SdkInitializationEvents.kt Co-authored-by: Manoel Aranda Neto <[email protected]>
Resolves #168 by wiring up initialization events.
The init events happen before the otel sdk is created, so we have to buffer them until after the sdk is created. Once that happens, the cached events are emitted as otel events.