Skip to content

Instrumentation api change - option 1 #1539

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.

Sign up for GitHub

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

Draft
LikeTheSalad wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Instrumentation api change - option 1 #1539

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
89c26cd
Base changes
LikeTheSalad Jan 26, 2026
e8f0384
Clean up InstallationContext
LikeTheSalad Jan 26, 2026
4d21ac7
Flushing from the rum api
LikeTheSalad Jan 26, 2026
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
7 changes: 7 additions & 0 deletions agent-api/src/main/java/io/opentelemetry/android/OpenTelemetryRum.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ package io.opentelemetry.android

import io.opentelemetry.api.OpenTelemetry
import io.opentelemetry.api.common.Attributes
import io.opentelemetry.sdk.common.CompletableResultCode

/**
* Entrypoint for the OpenTelemetry Real User Monitoring library for Android.
Expand All @@ -18,6 +19,10 @@ interface OpenTelemetryRum {
*/
val openTelemetry: OpenTelemetry

val clock: Clock

val sessionProvider: SessionProvider
Copy link
Copy Markdown
Member

@fractalwrench fractalwrench Jan 27, 2026

Choose a reason for hiding this comment

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

Happy with this but might be worth thinking about whether SessionProvider.noop() should still be exposed. Will end-users actually invoke that function?

Copy link
Copy Markdown
Contributor Author

@LikeTheSalad LikeTheSalad Jan 27, 2026

Choose a reason for hiding this comment

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

Good point. I guess it might be needed in case they wanted to create their own implementation of OpenTelemetryRum without sessions? It sounds like an edge case, though, so maybe it's still fine to remove it.


/**
* Get the client session ID associated with this instance of the RUM instrumentation library.
* Note: this value will change throughout the lifetime of an application instance, so it is
Expand All @@ -40,6 +45,8 @@ interface OpenTelemetryRum {
attributes: Attributes = Attributes.empty(),
)

fun flushLogRecords(): CompletableResultCode
Copy link
Copy Markdown
Member

@fractalwrench fractalwrench Jan 27, 2026

Choose a reason for hiding this comment

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

IMO either shutdown() should achieve this functionality or we can expose forceFlush(). It feels like flushLogRecords() is probably exposed to make one particular feature work, but IMO it'd be preferable just to have one API that flushes all telemetry, if we even want to expose that at all..

Copy link
Copy Markdown
Contributor Author

@LikeTheSalad LikeTheSalad Jan 27, 2026

Choose a reason for hiding this comment

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

It feels like flushLogRecords() is probably exposed to make one particular feature work

That's true, it'd only be useful for crash reporting, at least for now.

shutdown() should achieve this functionality

I'm open to that option, though I would have some questions. For example, is it ok to shut down RUM from an instrumentation? Or maybe it won't be needed to do so because shutdown() should be called on a crash automatically anyway (via our shutdownHook)? - If we decide on the latter, can we ensure that the app will have enough time to get to the SDK shutdown function before the OS kills the app?

or we can expose forceFlush()

I'm open to going with a "one method to flush all signals" approach. I'd still be a bit wary about the time it'd need to take to finish executing it all before the OS kills the app on a crash, though. But it sounds better than relying on the shutdown function, because forceFlush would be called directly from the instrumentation.

Copy link
Copy Markdown
Member

@fractalwrench fractalwrench Jan 27, 2026

Choose a reason for hiding this comment

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

After thinking about this some more on the other PR, I would lean towards just not exposing this as a public API for now (and perhaps forever). It could be an implementation detail of the SDK: #1540 (review)


/**
* Initiates orderly shutdown of this OpenTelemetryRum instance. After this method completes,
* the instance should be considered invalid and no longer used.
Expand Down
8 changes: 2 additions & 6 deletions ...rumentation/src/main/java/io/opentelemetry/android/instrumentation/InstallationContext.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,15 +7,11 @@ package io.opentelemetry.android.instrumentation

import android.app.Application
import android.content.Context
import io.opentelemetry.android.session.SessionProvider
import io.opentelemetry.api.OpenTelemetry
import io.opentelemetry.sdk.common.Clock
import io.opentelemetry.android.OpenTelemetryRum

class InstallationContext(
Copy link
Copy Markdown
Member

@fractalwrench fractalwrench Jan 27, 2026

Choose a reason for hiding this comment

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

We could perhaps consider whether InstallationContext is required anymore given that Context and OpenTelemetryRum could plausibly be passed into the install/uninstall functions instead.

If we do keep it, perhaps it's worth renaming to something like InstallationParams given that Context has a special meaning on Android already, and that OTel already has a separate concept of context.

Copy link
Copy Markdown
Contributor Author

@LikeTheSalad LikeTheSalad Jan 27, 2026

Choose a reason for hiding this comment

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

I don't have a strong opinion on this, so I'm ok with either removing it or renaming it 👍

val context: Context,
Copy link
Copy Markdown
Member

@fractalwrench fractalwrench Jan 27, 2026

Choose a reason for hiding this comment

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

I definitely like the fact that there are only 2 params left over here now.

val openTelemetry: OpenTelemetry,
val sessionProvider: SessionProvider,
val clock: Clock,
val openTelemetry: OpenTelemetryRum,
Copy link
Copy Markdown
Contributor

@breedx-splk breedx-splk Jan 27, 2026

Choose a reason for hiding this comment

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

nitpick: I don't love this name now -- because it suggests something from the upstream api and not a rum class. I might call it rum or openTelemetryRum.

Copy link
Copy Markdown
Contributor Author

@LikeTheSalad LikeTheSalad Jan 27, 2026

Choose a reason for hiding this comment

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

Makes sense. I'm up for renaming it to rum.

) {
val application: Application? = context as? Application
}
15 changes: 9 additions & 6 deletions ...ation/crash/src/main/java/io/opentelemetry/android/instrumentation/crash/CrashReporter.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,11 @@

package io.opentelemetry.android.instrumentation.crash

import io.opentelemetry.android.OpenTelemetryRum
import io.opentelemetry.android.common.internal.utils.threadIdCompat
import io.opentelemetry.android.instrumentation.common.EventAttributesExtractor
import io.opentelemetry.api.common.Attributes
import io.opentelemetry.context.Context
import io.opentelemetry.sdk.OpenTelemetrySdk
import io.opentelemetry.semconv.ExceptionAttributes.EXCEPTION_MESSAGE
import io.opentelemetry.semconv.ExceptionAttributes.EXCEPTION_STACKTRACE
import io.opentelemetry.semconv.ExceptionAttributes.EXCEPTION_TYPE
Expand All @@ -24,7 +24,7 @@ internal class CrashReporter(
additionalExtractors.toList()

/** Installs the crash reporting instrumentation. */
fun install(openTelemetry: OpenTelemetrySdk) {
fun install(openTelemetry: OpenTelemetryRum) {
val handler =
CrashReportingExceptionHandler(
crashProcessor = { crashDetails: CrashDetails ->
Expand All @@ -38,10 +38,13 @@ internal class CrashReporter(
}

private fun processCrash(
openTelemetry: OpenTelemetrySdk,
openTelemetry: OpenTelemetryRum,
crashDetails: CrashDetails,
) {
val logger = openTelemetry.sdkLoggerProvider.loggerBuilder("io.opentelemetry.crash").build()
val logger =
openTelemetry.openTelemetry.logsBridge
.loggerBuilder("io.opentelemetry.crash")
.build()
val throwable = crashDetails.cause
val thread = crashDetails.thread
val attributesBuilder =
Expand All @@ -66,8 +69,8 @@ internal class CrashReporter(
.emit()
}

private fun waitForCrashFlush(openTelemetry: OpenTelemetrySdk) {
val flushResult = openTelemetry.sdkLoggerProvider.forceFlush()
private fun waitForCrashFlush(openTelemetry: OpenTelemetryRum) {
val flushResult = openTelemetry.flushLogRecords()
flushResult.join(10, TimeUnit.SECONDS)
}
}
3 changes: 1 addition & 2 deletions .../main/java/io/opentelemetry/android/instrumentation/crash/CrashReporterInstrumentation.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import com.google.auto.service.AutoService
import io.opentelemetry.android.instrumentation.AndroidInstrumentation
import io.opentelemetry.android.instrumentation.InstallationContext
import io.opentelemetry.android.instrumentation.common.EventAttributesExtractor
import io.opentelemetry.sdk.OpenTelemetrySdk

/** Entrypoint for installing the crash reporting instrumentation. */
@AutoService(AndroidInstrumentation::class)
Expand All @@ -27,7 +26,7 @@ class CrashReporterInstrumentation : AndroidInstrumentation {
val crashReporter = CrashReporter(additionalExtractors)

// TODO avoid using OpenTelemetrySdk methods, only use the ones from OpenTelemetry api.
Copy link
Copy Markdown
Contributor

@breedx-splk breedx-splk Jan 27, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

todo can go away if we do this

crashReporter.install(ctx.openTelemetry as OpenTelemetrySdk)
crashReporter.install(ctx.openTelemetry)
}

override val name: String = "crash"
Expand Down
Loading