Skip to content
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

Initialization events #397

Merged
merged 9 commits into from
Jun 3, 2024
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -83,10 +83,11 @@ public final class OpenTelemetryRumBuilder {
private final OtelRumConfig config;
private final VisibleScreenTracker visibleScreenTracker = new VisibleScreenTracker();

private Function<? super SpanExporter, ? extends SpanExporter> spanExporterCustomizer = a -> a;
private final List<Consumer<InstrumentedApplication>> instrumentationInstallers =
new ArrayList<>();

private final List<Consumer<OpenTelemetrySdk>> otelSdkReadyListeners = new ArrayList<>();
private Function<? super SpanExporter, ? extends SpanExporter> spanExporterCustomizer = a -> a;
private Function<? super TextMapPropagator, ? extends TextMapPropagator> propagatorCustomizer =
(a) -> a;

Expand Down Expand Up @@ -325,6 +326,7 @@ OpenTelemetryRum build(ServiceManager serviceManager) {
Log.e(RumConstants.OTEL_RUM_LOG_TAG, "Could not initialize disk exporters.", e);
}
}
initializationEvents.spanExporterInitialized(spanExporter);

OpenTelemetrySdk sdk =
OpenTelemetrySdk.builder()
Expand Down Expand Up @@ -377,14 +379,30 @@ private void scheduleDiskTelemetryReader(
}
}

/**
* Adds a callback to be invoked after the OpenTelemetry SDK has been initialized. This can be
* used to defer some early lifecycle functionality until the working SDK is ready.
*
* @param callback - A callback that receives the OpenTelemetry SDK instance.
* @return this
*/
public OpenTelemetryRumBuilder addOtelSdkReadyListener(Consumer<OpenTelemetrySdk> callback) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consumer is API 24+ only, but I guess its desugared if isCoreLibraryDesugaringEnabled enabled right

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so, yeah...and it's used elsewhere.

otelSdkReadyListeners.add(callback);
return this;
}

/** Leverage the configuration to wire up various instrumentation components. */
private void applyConfiguration() {
if (config.shouldGenerateSdkInitializationEvents()) {
if (initializationEvents == InitializationEvents.NO_OP) {
initializationEvents = new SdkInitializationEvents();
SdkInitializationEvents sdkInitEvents = new SdkInitializationEvents();
addOtelSdkReadyListener(sdkInitEvents::finish);
initializationEvents = sdkInitEvents;
}
Map<String, String> configMap = new HashMap<>();
// TODO: Convert config to map
// breedx-splk: Left incomplete for now, because I think Cesar is making changes around
// this
initializationEvents.recordConfiguration(configMap);
}
initializationEvents.sdkInitializationStarted();
Expand Down Expand Up @@ -491,7 +509,6 @@ private SdkTracerProvider buildTracerProvider(
.setResource(resource)
.addSpanProcessor(new SessionIdSpanAppender(sessionId));

initializationEvents.spanExporterInitialized(spanExporter);
BatchSpanProcessor batchSpanProcessor = BatchSpanProcessor.builder(spanExporter).build();
tracerProviderBuilder.addSpanProcessor(batchSpanProcessor);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,5 +34,18 @@ public class RumConstants {

public static final String APP_START_SPAN_NAME = "AppStart";

public static final class Events {
public static final String INIT_EVENT_STARTED = "rum.sdk.init.started";
public static final String INIT_EVENT_CONFIG = "rum.sdk.init.config";
public static final String INIT_EVENT_NET_PROVIDER = "rum.sdk.init.net.provider";
public static final String INIT_EVENT_NET_MONITOR = "rum.sdk.init.net.monitor";
public static final String INIT_EVENT_ANR_MONITOR = "rum.sdk.init.anr_monitor";
public static final String INIT_EVENT_JANK_MONITOR = "rum.sdk.init.jank_monitor";
public static final String INIT_EVENT_CRASH_REPORTER = "rum.sdk.init.crash.reporter";
public static final String INIT_EVENT_SPAN_EXPORTER = "rum.sdk.init.span.exporter";

private Events() {}
}

private RumConstants() {}
}
3 changes: 3 additions & 0 deletions gradle/libs.versions.toml
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
[versions]
opentelemetry = "1.38.0"
opentelemetry-alpha = "1.38.0-alpha"
opentelemetry-instrumentation = "2.4.0"
opentelemetry-instrumentation-alpha = "2.4.0-alpha"
opentelemetry-semconv = "1.25.0-alpha"
Expand All @@ -25,6 +26,8 @@ opentelemetry-instrumentation-okhttp = { module = "io.opentelemetry.instrumentat
opentelemetry-semconv = { module = "io.opentelemetry.semconv:opentelemetry-semconv", version.ref = "opentelemetry-semconv" }
opentelemetry-semconv-incubating = { module = "io.opentelemetry.semconv:opentelemetry-semconv-incubating", version.ref = "opentelemetry-semconv" }
opentelemetry-api = { module = "io.opentelemetry:opentelemetry-api" }
opentelemetry-api-incubator = { module = "io.opentelemetry:opentelemetry-api-incubator" }
opentelemetry-sdk-extension-incubator = { module = "io.opentelemetry:opentelemetry-sdk-extension-incubator", version.ref = "opentelemetry-alpha" }
opentelemetry-sdk = { module = "io.opentelemetry:opentelemetry-sdk" }
opentelemetry-exporter-logging = { module = "io.opentelemetry:opentelemetry-exporter-logging" }
opentelemetry-diskBuffering = { module = "io.opentelemetry.contrib:opentelemetry-disk-buffering", version.ref = "opentelemetry-contrib" }
Expand Down
2 changes: 2 additions & 0 deletions instrumentation/startup/build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -21,5 +21,7 @@ dependencies {
implementation(libs.androidx.core)
implementation(libs.opentelemetry.semconv)
implementation(libs.opentelemetry.sdk)
implementation(libs.opentelemetry.api.incubator)
implementation(libs.opentelemetry.sdk.extension.incubator)
implementation(libs.opentelemetry.instrumentation.api)
}

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.android.instrumentation.startup

import io.opentelemetry.sdk.trace.export.SpanExporter

interface InitializationEvents {
fun sdkInitializationStarted()

fun recordConfiguration(config: Map<String, String>)

fun currentNetworkProviderInitialized()

fun networkMonitorInitialized()

fun anrMonitorInitialized()

fun slowRenderingDetectorInitialized()

fun crashReportingInitialized()

fun spanExporterInitialized(spanExporter: SpanExporter)

companion object {
@JvmField
val NO_OP: InitializationEvents =
object : InitializationEvents {
override fun sdkInitializationStarted() {}

override fun recordConfiguration(config: Map<String, String>) {}

override fun currentNetworkProviderInitialized() {}

override fun networkMonitorInitialized() {}

override fun anrMonitorInitialized() {}

override fun slowRenderingDetectorInitialized() {}

override fun crashReportingInitialized() {}

override fun spanExporterInitialized(spanExporter: SpanExporter) {}
}
}
}

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,116 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.android.instrumentation.startup

import io.opentelemetry.android.common.RumConstants
import io.opentelemetry.api.common.AttributeKey
import io.opentelemetry.api.common.Attributes
import io.opentelemetry.api.incubator.logs.AnyValue
import io.opentelemetry.sdk.OpenTelemetrySdk
import io.opentelemetry.sdk.logs.internal.SdkEventLoggerProvider
import io.opentelemetry.sdk.trace.export.SpanExporter
import java.time.Instant
import java.util.function.Consumer
import java.util.function.Supplier
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


class SdkInitializationEvents(private val clock: Supplier<Instant> = Supplier { Instant.now() }) : InitializationEvents {
private val events: MutableList<Event> = ArrayList()
breedx-splk marked this conversation as resolved.
Show resolved Hide resolved

override fun sdkInitializationStarted() {
addEvent(RumConstants.Events.INIT_EVENT_STARTED)
}

override fun recordConfiguration(config: Map<String, String>) {
val map: MutableMap<String, AnyValue<*>> = HashMap()
breedx-splk marked this conversation as resolved.
Show resolved Hide resolved
config.entries.forEach(
Consumer { e: Map.Entry<String, String> ->
map[e.key] = AnyValue.of(e.value)
},
)
val body = AnyValue.of(map)
addEvent(RumConstants.Events.INIT_EVENT_CONFIG, body)
}

override fun currentNetworkProviderInitialized() {
addEvent(RumConstants.Events.INIT_EVENT_NET_PROVIDER)
}

override fun networkMonitorInitialized() {
addEvent(RumConstants.Events.INIT_EVENT_NET_MONITOR)
}

override fun anrMonitorInitialized() {
addEvent(RumConstants.Events.INIT_EVENT_ANR_MONITOR)
}

override fun slowRenderingDetectorInitialized() {
addEvent(RumConstants.Events.INIT_EVENT_JANK_MONITOR)
}

override fun crashReportingInitialized() {
addEvent(RumConstants.Events.INIT_EVENT_CRASH_REPORTER)
}

override fun spanExporterInitialized(spanExporter: SpanExporter) {
val attributes =
Attributes.of(AttributeKey.stringKey("span.exporter"), spanExporter.toString())
addEvent(RumConstants.Events.INIT_EVENT_SPAN_EXPORTER, attributes)
}

fun finish(sdk: OpenTelemetrySdk) {
val loggerProvider = sdk.sdkLoggerProvider
val eventLogger =
SdkEventLoggerProvider.create(loggerProvider).get("otel.initialization.events")
events.forEach { event: Event ->
val eventBuilder =
eventLogger.builder(event.name)
.setTimestamp(event.timestamp)
.setAttributes(event.attributes)
if (event.body != null) {
// TODO: Config is technically correct because config is the only startup event
// with a body, but this is ultimately clunky/fragile.
eventBuilder.put("config", event.body)
}
eventBuilder.emit()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}

private fun addEvent(
name: String,
body: AnyValue<*>,
) {
addEvent(name, null, body)
}

private fun addEvent(
name: String,
attr: Attributes,
) {
addEvent(name, attr, null)
}

private fun addEvent(
name: String,
attr: Attributes? = null,
body: AnyValue<*>? = null,
) {
events.add(Event(clock.get(), name, attr, body))
}
breedx-splk marked this conversation as resolved.
Show resolved Hide resolved

private class Event(
val timestamp: Instant,
val name: String,
val attributes: Attributes?,
val body: AnyValue<*>? = null,
) {
private constructor(timestamp: Instant, name: String, body: AnyValue<*>) : this(
breedx-splk marked this conversation as resolved.
Show resolved Hide resolved
timestamp,
name,
null,
body,
)
}
}
Loading