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.

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

Jump to bottom

Refactor session handling #564

Merged
merged 20 commits into from
Aug 30, 2024
Merged

Refactor session handling #564

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
f098fe2
first pass at new session api
breedx-splk Aug 27, 2024
56f0b19
session work in progress
breedx-splk Aug 28, 2024
81d2f42
Rename .java to .kt
breedx-splk Aug 28, 2024
a428718
fix up some tests
breedx-splk Aug 28, 2024
ef9c20e
make sampler use interface
breedx-splk Aug 28, 2024
8d13d28
move SessionIdTest.java to SessionManagerTest.kt and fix up
breedx-splk Aug 28, 2024
2d9f56c
remove unused SessionId.java
breedx-splk Aug 28, 2024
276b155
remove unused SessionChangeObserver
breedx-splk Aug 28, 2024
b20a910
spotless
breedx-splk Aug 28, 2024
30c5a50
make constants
breedx-splk Aug 28, 2024
7234663
don't send events for the NONE session
breedx-splk Aug 28, 2024
869840b
default to NONE session and cause session start after adding observer
breedx-splk Aug 28, 2024
fe4d8e6
Rename .java to .kt
breedx-splk Aug 28, 2024
8436771
migrate observer to kotlin
breedx-splk Aug 28, 2024
7aa16ba
spotless
breedx-splk Aug 28, 2024
fe5d9e4
pass config session timeout and don't force SessionManager to be a bu…
breedx-splk Aug 28, 2024
ae68dd1
fix typo
breedx-splk Aug 28, 2024
8d54ec7
fix test
breedx-splk Aug 28, 2024
9ef46b0
fix tests
breedx-splk Aug 28, 2024
17b0792
remove SessionIdEventSender (to make the PR more purely refactoring)
breedx-splk Aug 29, 2024
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
1 change: 1 addition & 0 deletions core/build.gradle.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ dependencies {
api(platform(libs.opentelemetry.platform))
api(libs.opentelemetry.api)
implementation(libs.opentelemetry.sdk)
implementation(libs.opentelemetry.api.incubator)
implementation(libs.opentelemetry.exporter.logging)
implementation(libs.opentelemetry.instrumentation.api)
implementation(libs.opentelemetry.semconv.incubating)
Expand Down
22 changes: 12 additions & 10 deletions core/src/main/java/io/opentelemetry/android/OpenTelemetryRumBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@
import io.opentelemetry.android.internal.services.CacheStorage;
import io.opentelemetry.android.internal.services.Preferences;
import io.opentelemetry.android.internal.services.ServiceManager;
import io.opentelemetry.android.session.SessionManager;
import io.opentelemetry.android.session.SessionProvider;
import io.opentelemetry.api.baggage.propagation.W3CBaggagePropagator;
import io.opentelemetry.api.trace.propagation.W3CTraceContextPropagator;
import io.opentelemetry.context.propagation.ContextPropagators;
Expand Down Expand Up @@ -65,7 +67,6 @@
*/
public final class OpenTelemetryRumBuilder {

private final SessionId sessionId;
private final Application application;
private final List<BiFunction<SdkTracerProviderBuilder, Application, SdkTracerProviderBuilder>>
tracerProviderCustomizers = new ArrayList<>();
Expand All @@ -76,6 +77,7 @@ public final class OpenTelemetryRumBuilder {
private final OtelRumConfig config;
private final List<AndroidInstrumentation> instrumentations = new ArrayList<>();
private final List<Consumer<OpenTelemetrySdk>> otelSdkReadyListeners = new ArrayList<>();
private final SessionIdTimeoutHandler timeoutHandler;
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 this is fine for the purpose of replacing the existing behavior's implementation with the new API. However, since this is the core part of the lib, I think it should receive the lowest level part of the API, which is SessionProvider. Passing a SessionIdTimeoutHandler in core implies that the only way all apps can get their session handled is by time, even if that doesn't match their specific requirements. Probably a good place to pass SessionIdTimeoutHandler would be in the higher-level type AndroidAgentBuilder that it's proposed here. That way the "time-based" approach that we recommend would be the easiest to set up via the agent, though it would still be possible to override it when needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

In fact, probably only SessionProvider should live in core and we should move all of its subtypes/helpers over to android-agent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can talk more about this, because I think that we should be opinionated about sessions having a default max time to live...but the user should ultimately be able to configure and/or override this. As you pointed out, it's also consistent with the existing/current behavior.

Can we work on this aspect in a follow-up PR effort?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good. We can follow up on it later 👍

private Function<? super SpanExporter, ? extends SpanExporter> spanExporterCustomizer = a -> a;
private Function<? super LogRecordExporter, ? extends LogRecordExporter>
logRecordExporterCustomizer = a -> a;
Expand All @@ -97,7 +99,7 @@ public static OpenTelemetryRumBuilder create(Application application, OtelRumCon
OpenTelemetryRumBuilder(
Application application, OtelRumConfig config, SessionIdTimeoutHandler timeoutHandler) {
this.application = application;
this.sessionId = new SessionId(timeoutHandler);
this.timeoutHandler = timeoutHandler;
this.resource = AndroidResource.createDefault(application);
this.config = config;
}
Expand Down Expand Up @@ -253,10 +255,6 @@ public OpenTelemetryRumBuilder addLogRecordExporterCustomizer(
return this;
}

public SessionId getSessionId() {
return sessionId;
}

/**
* Creates a new instance of {@link OpenTelemetryRum} with the settings of this {@link
* OpenTelemetryRumBuilder}.
Expand Down Expand Up @@ -303,10 +301,13 @@ OpenTelemetryRum build(ServiceManager serviceManager) {
}
initializationEvents.spanExporterInitialized(spanExporter);

SessionManager sessionManager =
SessionManager.create(timeoutHandler, config.getSessionTimeout().toNanos());

OpenTelemetrySdk sdk =
OpenTelemetrySdk.builder()
.setTracerProvider(
buildTracerProvider(sessionId, application, spanExporter))
buildTracerProvider(sessionManager, application, spanExporter))
.setMeterProvider(buildMeterProvider(application))
.setLoggerProvider(buildLoggerProvider(application, logsExporter))
.setPropagators(buildFinalPropagators())
Expand All @@ -320,7 +321,8 @@ OpenTelemetryRum build(ServiceManager serviceManager) {
new SdkPreconfiguredRumBuilder(
application,
sdk,
sessionId,
timeoutHandler,
sessionManager,
config.shouldDiscoverInstrumentations(),
serviceManager);
instrumentations.forEach(delegate::addInstrumentation);
Expand Down Expand Up @@ -417,11 +419,11 @@ private void applyConfiguration(
}

private SdkTracerProvider buildTracerProvider(
SessionId sessionId, Application application, SpanExporter spanExporter) {
SessionProvider sessionProvider, Application application, SpanExporter spanExporter) {
SdkTracerProviderBuilder tracerProviderBuilder =
SdkTracerProvider.builder()
.setResource(resource)
.addSpanProcessor(new SessionIdSpanAppender(sessionId));
.addSpanProcessor(new SessionIdSpanAppender(sessionProvider));

BatchSpanProcessor batchSpanProcessor = BatchSpanProcessor.builder(spanExporter).build();
tracerProviderBuilder.addSpanProcessor(batchSpanProcessor);
Expand Down
9 changes: 5 additions & 4 deletions core/src/main/java/io/opentelemetry/android/OpenTelemetryRumImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,17 +5,18 @@

package io.opentelemetry.android;

import io.opentelemetry.android.session.SessionManager;
import io.opentelemetry.api.OpenTelemetry;
import io.opentelemetry.sdk.OpenTelemetrySdk;

final class OpenTelemetryRumImpl implements OpenTelemetryRum {

private final OpenTelemetrySdk openTelemetrySdk;
private final SessionId sessionId;
private final SessionManager sessionManager;

OpenTelemetryRumImpl(OpenTelemetrySdk openTelemetrySdk, SessionId sessionId) {
OpenTelemetryRumImpl(OpenTelemetrySdk openTelemetrySdk, SessionManager sessionManager) {
this.openTelemetrySdk = openTelemetrySdk;
this.sessionId = sessionId;
this.sessionManager = sessionManager;
}

@Override
Expand All @@ -25,6 +26,6 @@ public OpenTelemetry getOpenTelemetry() {

@Override
public String getRumSessionId() {
return sessionId.getSessionId();
return sessionManager.getSessionId();
}
}
14 changes: 5 additions & 9 deletions core/src/main/java/io/opentelemetry/android/SdkPreconfiguredRumBuilder.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,17 +9,16 @@ import android.app.Application
import io.opentelemetry.android.instrumentation.AndroidInstrumentation
import io.opentelemetry.android.instrumentation.AndroidInstrumentationLoader
import io.opentelemetry.android.internal.services.ServiceManager
import io.opentelemetry.android.session.SessionManager
import io.opentelemetry.sdk.OpenTelemetrySdk

class SdkPreconfiguredRumBuilder
@JvmOverloads
internal constructor(
private val application: Application,
private val sdk: OpenTelemetrySdk,
private val sessionId: SessionId =
SessionId(
SessionIdTimeoutHandler(),
),
private val timeoutHandler: SessionIdTimeoutHandler = SessionIdTimeoutHandler(),
private val sessionManager: SessionManager = SessionManager(timeoutHandler = timeoutHandler),
private val discoverInstrumentations: Boolean,
private val serviceManager: ServiceManager,
) {
Expand Down Expand Up @@ -49,12 +48,9 @@ class SdkPreconfiguredRumBuilder
serviceManager.start()
// the app state listeners need to be run in the first ActivityLifecycleCallbacks since they
// might turn off/on additional telemetry depending on whether the app is active or not
appLifecycleService.registerListener(sessionId.timeoutHandler)
appLifecycleService.registerListener(timeoutHandler)

val tracer = sdk.getTracer(OpenTelemetryRum::class.java.simpleName)
sessionId.setSessionIdChangeListener(SessionIdChangeTracer(tracer))

val openTelemetryRum = OpenTelemetryRumImpl(sdk, sessionId)
val openTelemetryRum = OpenTelemetryRumImpl(sdk, sessionManager)

// Install instrumentations
for (instrumentation in getInstrumentations()) {
Expand Down
92 changes: 0 additions & 92 deletions core/src/main/java/io/opentelemetry/android/SessionId.java
View file
Open in desktop

This file was deleted.

12 changes: 0 additions & 12 deletions core/src/main/java/io/opentelemetry/android/SessionIdChangeListener.java
View file
Open in desktop

This file was deleted.

26 changes: 0 additions & 26 deletions core/src/main/java/io/opentelemetry/android/SessionIdChangeTracer.java
View file
Open in desktop

This file was deleted.

14 changes: 10 additions & 4 deletions core/src/main/java/io/opentelemetry/android/SessionIdRatioBasedSampler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

package io.opentelemetry.android;

import io.opentelemetry.android.session.SessionProvider;
import io.opentelemetry.api.common.Attributes;
import io.opentelemetry.api.trace.SpanKind;
import io.opentelemetry.context.Context;
Expand All @@ -22,10 +23,10 @@
*/
public class SessionIdRatioBasedSampler implements Sampler {
private final Sampler ratioBasedSampler;
private final SessionId sessionId;
private final SessionProvider sessionProvider;

public SessionIdRatioBasedSampler(double ratio, SessionId sessionId) {
this.sessionId = sessionId;
public SessionIdRatioBasedSampler(double ratio, SessionProvider sessionProvider) {
this.sessionProvider = sessionProvider;
// SessionId uses the same format as TraceId, so we can reuse trace ID ratio sampler.
this.ratioBasedSampler = Sampler.traceIdRatioBased(ratio);
}
Expand All @@ -40,7 +41,12 @@ public SamplingResult shouldSample(
List<LinkData> parentLinks) {
// Replace traceId with sessionId
return ratioBasedSampler.shouldSample(
parentContext, sessionId.getSessionId(), name, spanKind, attributes, parentLinks);
parentContext,
sessionProvider.getSessionId(),
name,
spanKind,
attributes,
parentLinks);
}

@Override
Expand Down
12 changes: 7 additions & 5 deletions core/src/main/java/io/opentelemetry/android/SessionIdSpanAppender.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,23 +5,25 @@

package io.opentelemetry.android;

import io.opentelemetry.android.common.RumConstants;
import static io.opentelemetry.semconv.incubating.SessionIncubatingAttributes.SESSION_ID;

import io.opentelemetry.android.session.SessionProvider;
import io.opentelemetry.context.Context;
import io.opentelemetry.sdk.trace.ReadWriteSpan;
import io.opentelemetry.sdk.trace.ReadableSpan;
import io.opentelemetry.sdk.trace.SpanProcessor;

final class SessionIdSpanAppender implements SpanProcessor {

private final SessionId sessionId;
private final SessionProvider sessionProvider;

public SessionIdSpanAppender(SessionId sessionId) {
this.sessionId = sessionId;
public SessionIdSpanAppender(SessionProvider sessionProvider) {
this.sessionProvider = sessionProvider;
}

@Override
public void onStart(Context parentContext, ReadWriteSpan span) {
span.setAttribute(RumConstants.SESSION_ID_KEY, sessionId.getSessionId());
span.setAttribute(SESSION_ID, sessionProvider.getSessionId());
}

@Override
Expand Down
7 changes: 4 additions & 3 deletions core/src/main/java/io/opentelemetry/android/SessionIdTimeoutHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@
* <p>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.
*/
final class SessionIdTimeoutHandler implements ApplicationStateListener {
// TODO: Migrate to kotlin and make internal?
public final class SessionIdTimeoutHandler implements ApplicationStateListener {

static final Duration DEFAULT_SESSION_TIMEOUT = Duration.ofMinutes(15);
private final Duration sessionTimeout;
Expand Down Expand Up @@ -56,7 +57,7 @@ public void onApplicationBackgrounded() {
state = State.BACKGROUND;
}

boolean hasTimedOut() {
public boolean hasTimedOut() {
// don't apply sessionId timeout to apps in the foreground
if (state == State.FOREGROUND) {
return false;
Expand All @@ -65,7 +66,7 @@ boolean hasTimedOut() {
return elapsedTime >= sessionTimeout.toNanos();
}

void bump() {
public void bump() {
timeoutStartNanos = clock.nanoTime();

// move from the temporary transition state to foreground after the first span
Expand Down
Loading