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

[Do not merge] Agent api prototype #624

Draft
wants to merge 16 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all 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
3 changes: 2 additions & 1 deletion android-agent/build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,8 @@ android {

dependencies {
api(project(":core"))
implementation(libs.opentelemetry.instrumentation.api)
implementation(libs.opentelemetry.sdk)
implementation(libs.opentelemetry.exporter.otlp)

// Default instrumentations:
api(project(":instrumentation:activity"))
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,167 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.android.agent

import android.app.Application
import io.opentelemetry.android.OpenTelemetryRum
import io.opentelemetry.android.OpenTelemetryRumBuilder
import io.opentelemetry.android.agent.endpoint.EndpointConfig
import io.opentelemetry.android.agent.session.SessionIdTimeoutHandler
import io.opentelemetry.android.agent.session.SessionManager
import io.opentelemetry.android.config.OtelRumConfig
import io.opentelemetry.android.features.diskbuffering.DiskBufferingConfiguration
import io.opentelemetry.android.instrumentation.AndroidInstrumentationLoader
import io.opentelemetry.android.instrumentation.activity.ActivityLifecycleInstrumentation
import io.opentelemetry.android.instrumentation.anr.AnrInstrumentation
import io.opentelemetry.android.instrumentation.common.ScreenNameExtractor
import io.opentelemetry.android.instrumentation.crash.CrashDetails
import io.opentelemetry.android.instrumentation.crash.CrashReporterInstrumentation
import io.opentelemetry.android.instrumentation.fragment.FragmentLifecycleInstrumentation
import io.opentelemetry.android.instrumentation.network.NetworkChangeInstrumentation
import io.opentelemetry.android.instrumentation.slowrendering.SlowRenderingInstrumentation
import io.opentelemetry.android.internal.services.ServiceManager
import io.opentelemetry.android.internal.services.network.data.CurrentNetwork
import io.opentelemetry.api.trace.Tracer
import io.opentelemetry.exporter.otlp.http.logs.OtlpHttpLogRecordExporter
import io.opentelemetry.exporter.otlp.http.trace.OtlpHttpSpanExporter
import io.opentelemetry.instrumentation.api.instrumenter.AttributesExtractor
import io.opentelemetry.sdk.common.Clock
import java.time.Duration

object AndroidAgent {
Copy link
Contributor

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?

private val activityLifecycleInstrumentation by lazy {
AndroidInstrumentationLoader.getInstrumentation(
ActivityLifecycleInstrumentation::class.java,
)!!
Copy link
Contributor

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

}
private val fragmentLifecycleInstrumentation by lazy {
AndroidInstrumentationLoader.getInstrumentation(
FragmentLifecycleInstrumentation::class.java,
)!!
}
private val anrInstrumentation by lazy {
AndroidInstrumentationLoader.getInstrumentation(
AnrInstrumentation::class.java,
)!!
}
private val crashReporterInstrumentation by lazy {
AndroidInstrumentationLoader.getInstrumentation(
CrashReporterInstrumentation::class.java,
)!!
}
private val networkChangeInstrumentation by lazy {
AndroidInstrumentationLoader.getInstrumentation(NetworkChangeInstrumentation::class.java)!!
}
private val slowRenderingInstrumentation by lazy {
AndroidInstrumentationLoader.getInstrumentation(SlowRenderingInstrumentation::class.java)!!
}

fun createRumBuilder(
application: Application,
otelRumConfig: OtelRumConfig = OtelRumConfig(),
Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Contributor

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.

endpointConfig: EndpointConfig = EndpointConfig.getDefault("http://localhost"),
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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

sessionTimeout: Duration = SessionIdTimeoutHandler.DEFAULT_SESSION_TIMEOUT,
activityTracerCustomizer: ((Tracer) -> Tracer)? = null,
activityNameExtractor: ScreenNameExtractor? = null,
fragmentTracerCustomizer: ((Tracer) -> Tracer)? = null,
fragmentNameExtractor: ScreenNameExtractor? = null,
anrAttributesExtractor: AttributesExtractor<Array<StackTraceElement>, Void>? = null,
crashAttributesExtractor: AttributesExtractor<CrashDetails, Void>? = null,
networkChangeAttributesExtractor: AttributesExtractor<CurrentNetwork, Void>? = null,
slowRenderingDetectionPollInterval: Duration? = null,
): OpenTelemetryRumBuilder {
val rumBuilder = OpenTelemetryRum.builder(application, otelRumConfig)

configureSessionProvider(rumBuilder, sessionTimeout)
configureExporters(rumBuilder, endpointConfig)
configureDiskBuffering(rumBuilder)

applyInstrumentationConfigs(
activityTracerCustomizer,
activityNameExtractor,
fragmentTracerCustomizer,
fragmentNameExtractor,
anrAttributesExtractor,
crashAttributesExtractor,
networkChangeAttributesExtractor,
slowRenderingDetectionPollInterval,
)

return rumBuilder
}

private fun configureSessionProvider(
rumBuilder: OpenTelemetryRumBuilder,
sessionTimeout: Duration,
) {
val clock = Clock.getDefault()
val sessionIdTimeoutHandler = SessionIdTimeoutHandler(clock, sessionTimeout)
rumBuilder.setSessionProvider(SessionManager.create(clock, sessionIdTimeoutHandler))
rumBuilder.addOtelSdkReadyListener {
ServiceManager.get().getAppLifecycleService().registerListener(sessionIdTimeoutHandler)
}
}

private fun configureExporters(
rumBuilder: OpenTelemetryRumBuilder,
endpointConfig: EndpointConfig,
) {
// Creating span exporter builder
val spanExporterBuilder =
OtlpHttpSpanExporter.builder().setEndpoint(endpointConfig.getSpanExporterUrl())
// Creating log exporter builder
val logRecordExporterBuilder =
OtlpHttpLogRecordExporter.builder()
.setEndpoint(endpointConfig.getLogRecordExporterUrl())

// Adding headers
endpointConfig.getHeaders()
.forEach { (key, value) ->
spanExporterBuilder.addHeader(key, value)
logRecordExporterBuilder.addHeader(key, value)
}

// Adding exporters to the rum builder
rumBuilder.setSpanExporter(spanExporterBuilder.build())
rumBuilder.setLogRecordExporter(logRecordExporterBuilder.build())
}

private fun configureDiskBuffering(rumBuilder: OpenTelemetryRumBuilder) {
rumBuilder.setDiskBufferingConfiguration(
DiskBufferingConfiguration.builder()
.setEnabled(true)
.setMaxCacheSize(10_000_000).build(),
)
}

private fun applyInstrumentationConfigs(
activityTracerCustomizer: ((Tracer) -> Tracer)?,
activityNameExtractor: ScreenNameExtractor?,
fragmentTracerCustomizer: ((Tracer) -> Tracer)?,
fragmentNameExtractor: ScreenNameExtractor?,
anrAttributesExtractor: AttributesExtractor<Array<StackTraceElement>, Void>?,
crashAttributesExtractor: AttributesExtractor<CrashDetails, Void>?,
networkChangeAttributesExtractor: AttributesExtractor<CurrentNetwork, Void>?,
slowRenderingDetectionPollInterval: Duration?,
) {
activityTracerCustomizer?.let { activityLifecycleInstrumentation.setTracerCustomizer(it) }
activityNameExtractor?.let { activityLifecycleInstrumentation.setScreenNameExtractor(it) }
fragmentTracerCustomizer?.let { fragmentLifecycleInstrumentation.setTracerCustomizer(it) }
fragmentNameExtractor?.let { fragmentLifecycleInstrumentation.setScreenNameExtractor(it) }
anrAttributesExtractor?.let { anrInstrumentation.addAttributesExtractor(it) }
crashAttributesExtractor?.let { crashReporterInstrumentation.addAttributesExtractor(it) }
networkChangeAttributesExtractor?.let {
networkChangeInstrumentation.addAttributesExtractor(
it,
)
}
slowRenderingDetectionPollInterval?.let {
slowRenderingInstrumentation.setSlowRenderingDetectionPollInterval(
it,
)
}
}
}

This file was deleted.

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

package io.opentelemetry.android.agent.endpoint

internal data class DefaultHttpEndpointConfig(
private val baseUrl: String,
private val headers: Map<String, String>,
) : EndpointConfig {
override fun getSpanExporterUrl(): String {
return "$baseUrl/v1/traces"
}

override fun getLogRecordExporterUrl(): String {
return "$baseUrl/v1/logs"
}

override fun getHeaders(): Map<String, String> {
return headers
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.android.agent.endpoint

interface EndpointConfig {
fun getSpanExporterUrl(): String

fun getLogRecordExporterUrl(): String

fun getHeaders(): Map<String, String>

companion object {
@JvmStatic
fun getDefault(
baseUrl: String,
headers: Map<String, String> = emptyMap(),
): EndpointConfig {
return DefaultHttpEndpointConfig(baseUrl.trimEnd('/'), headers)
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.android.session
package io.opentelemetry.android.agent.session

interface Session {
fun getId(): String
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.android.session
package io.opentelemetry.android.agent.session

import io.opentelemetry.api.trace.TraceId
import java.util.Random
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.android.agent.session

import io.opentelemetry.android.internal.services.applifecycle.ApplicationStateListener
import io.opentelemetry.sdk.common.Clock
import java.time.Duration

/**
* This class encapsulates the following criteria about the sessionId timeout:
* - If the app is in the foreground sessionId should never time out.
* - If the app is in the background and no activity (spans) happens for >15 minutes, sessionId
* should time out.
* - If the app is in the background and some activity (spans) happens in <15 minute intervals,
* sessionId should not time out.
*
* Consequently, when the app spent >15 minutes without any activity (spans) in the background,
* after moving to the foreground the first span should trigger the sessionId timeout.
*/
internal class SessionIdTimeoutHandler(
private val clock: Clock = Clock.getDefault(),
private val sessionTimeout: Duration = DEFAULT_SESSION_TIMEOUT,
) : ApplicationStateListener {
@Volatile
private var timeoutStartNanos: Long = 0

@Volatile
private var state = State.FOREGROUND

override fun onApplicationForegrounded() {
state = State.TRANSITIONING_TO_FOREGROUND
}

override fun onApplicationBackgrounded() {
state = State.BACKGROUND
}

fun hasTimedOut(): Boolean {
// don't apply sessionId timeout to apps in the foreground
if (state == State.FOREGROUND) {
return false
}
val elapsedTime = clock.nanoTime() - timeoutStartNanos
return elapsedTime >= sessionTimeout.toNanos()
}

fun bump() {
timeoutStartNanos = clock.nanoTime()

// move from the temporary transition state to foreground after the first span
if (state == State.TRANSITIONING_TO_FOREGROUND) {
state = State.FOREGROUND
}
}

private enum class State {
FOREGROUND,
BACKGROUND,

/** A temporary state representing the first event after the app has been brought back. */
TRANSITIONING_TO_FOREGROUND,
}

companion object {
val DEFAULT_SESSION_TIMEOUT: Duration = Duration.ofMinutes(15)
}
}
Loading