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

Relax the ServiceManager singleton nature #671

Merged
merged 14 commits into from
Nov 18, 2024
Merged

Relax the ServiceManager singleton nature #671

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
ef96126
allow setting the ServiceManager and only create if needed at build()…
breedx-splk Oct 29, 2024
6510fbc
create InstallationContext
breedx-splk Oct 29, 2024
4028251
update AndroidInstrumentation.install() to take the InstallationContext.
breedx-splk Oct 29, 2024
53e2b10
decouple instrumentations from ServiceManager singleton
breedx-splk Oct 30, 2024
e4865e6
pull static usage up
breedx-splk Oct 30, 2024
6c1f307
remove static create method
breedx-splk Oct 30, 2024
530d16f
remove static create() in favor of constructor
breedx-splk Oct 30, 2024
927af23
pull operational object out of disk buffer config and promote to OTRB…
breedx-splk Oct 30, 2024
5571131
move opinionated default list of services out of ServiceManager inter…
breedx-splk Oct 30, 2024
d906f89
remove static call and just test the impl
breedx-splk Oct 30, 2024
2bcd1e8
cleanup
breedx-splk Oct 30, 2024
486840f
eliminate test dep on ServiceManager static usage
breedx-splk Oct 30, 2024
71b56dc
finally remove singleton nature from ServiceManager.
breedx-splk Oct 30, 2024
3e388ed
add period
breedx-splk Oct 30, 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
8 changes: 5 additions & 3 deletions core/src/main/java/io/opentelemetry/android/OpenTelemetryRum.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,14 +61,16 @@ static OpenTelemetryRumBuilder builder(Application application, OtelRumConfig co
* @param openTelemetrySdk The {@link OpenTelemetrySdk} that the user has already created.
* @param discoverInstrumentations TRUE to look for instrumentations in the classpath and
* applying them automatically.
* @param serviceManager The ServiceManager instance
*/
static SdkPreconfiguredRumBuilder builder(
Application application,
OpenTelemetrySdk openTelemetrySdk,
boolean discoverInstrumentations) {
ServiceManager.initialize(application);
boolean discoverInstrumentations,
ServiceManager serviceManager) {

return new SdkPreconfiguredRumBuilder(
application, openTelemetrySdk, discoverInstrumentations, ServiceManager.get());
application, openTelemetrySdk, discoverInstrumentations, serviceManager);
}

/** Returns a no-op implementation of {@link OpenTelemetryRum}. */
Expand Down
77 changes: 54 additions & 23 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 @@ -9,10 +9,13 @@

import android.app.Application;
import android.util.Log;
import androidx.annotation.NonNull;
import io.opentelemetry.android.common.RumConstants;
import io.opentelemetry.android.config.OtelRumConfig;
import io.opentelemetry.android.features.diskbuffering.DiskBufferingConfiguration;
import io.opentelemetry.android.features.diskbuffering.SignalFromDiskExporter;
import io.opentelemetry.android.features.diskbuffering.scheduler.DefaultExportScheduleHandler;
import io.opentelemetry.android.features.diskbuffering.scheduler.DefaultExportScheduler;
import io.opentelemetry.android.features.diskbuffering.scheduler.ExportScheduleHandler;
import io.opentelemetry.android.instrumentation.AndroidInstrumentation;
import io.opentelemetry.android.internal.features.networkattrs.NetworkAttributesSpanAppender;
Expand All @@ -23,6 +26,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.internal.services.ServiceManagerImpl;
import io.opentelemetry.android.internal.services.periodicwork.PeriodicWorkService;
import io.opentelemetry.android.session.SessionManager;
import io.opentelemetry.android.session.SessionProvider;
import io.opentelemetry.api.baggage.propagation.W3CBaggagePropagator;
Expand Down Expand Up @@ -57,6 +62,7 @@
import java.util.function.Consumer;
import java.util.function.Function;
import javax.annotation.Nullable;
import kotlin.jvm.functions.Function0;

/**
* A builder of {@link OpenTelemetryRum}. It enabled configuring the OpenTelemetry SDK and disabling
Expand Down Expand Up @@ -86,6 +92,9 @@ public final class OpenTelemetryRumBuilder {

private Resource resource;

@Nullable private ServiceManager serviceManager;
@Nullable private ExportScheduleHandler exportScheduleHandler;

private static TextMapPropagator buildDefaultPropagator() {
return TextMapPropagator.composite(
W3CTraceContextPropagator.getInstance(), W3CBaggagePropagator.getInstance());
Expand Down Expand Up @@ -265,13 +274,8 @@ public OpenTelemetryRumBuilder addLogRecordExporterCustomizer(
* @return A new {@link OpenTelemetryRum} instance.
*/
public OpenTelemetryRum build() {
ServiceManager.initialize(application);
return build(ServiceManager.get());
}

OpenTelemetryRum build(ServiceManager serviceManager) {
InitializationEvents initializationEvents = InitializationEvents.get();
applyConfiguration(serviceManager, initializationEvents);
applyConfiguration(initializationEvents);

DiskBufferingConfiguration diskBufferingConfiguration =
config.getDiskBufferingConfiguration();
Expand All @@ -280,8 +284,7 @@ OpenTelemetryRum build(ServiceManager serviceManager) {
SignalFromDiskExporter signalFromDiskExporter = null;
if (diskBufferingConfiguration.isEnabled()) {
try {
StorageConfiguration storageConfiguration =
createStorageConfiguration(serviceManager);
StorageConfiguration storageConfiguration = createStorageConfiguration();
final SpanExporter originalSpanExporter = spanExporter;
spanExporter =
SpanToDiskExporter.create(originalSpanExporter, storageConfiguration);
Expand Down Expand Up @@ -315,7 +318,7 @@ OpenTelemetryRum build(ServiceManager serviceManager) {

otelSdkReadyListeners.forEach(listener -> listener.accept(sdk));

scheduleDiskTelemetryReader(signalFromDiskExporter, diskBufferingConfiguration);
scheduleDiskTelemetryReader(signalFromDiskExporter);

SdkPreconfiguredRumBuilder delegate =
new SdkPreconfiguredRumBuilder(
Expand All @@ -324,15 +327,37 @@ OpenTelemetryRum build(ServiceManager serviceManager) {
timeoutHandler,
sessionManager,
config.shouldDiscoverInstrumentations(),
serviceManager);
getServiceManager());
instrumentations.forEach(delegate::addInstrumentation);
return delegate.build();
}

private StorageConfiguration createStorageConfiguration(ServiceManager serviceManager)
throws IOException {
Preferences preferences = serviceManager.getPreferences();
CacheStorage storage = serviceManager.getCacheStorage();
@NonNull
private ServiceManager getServiceManager() {
if (serviceManager == null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

In theory this should be called only from the main thread, but probably doesn't hurt to synchronize the init/set?

serviceManager = ServiceManagerImpl.Companion.create(application);
}
return serviceManager;
}

public OpenTelemetryRumBuilder setServiceManager(ServiceManager serviceManager) {
this.serviceManager = serviceManager;
return this;
}

/**
* Sets a scheduler that will take care of periodically read data stored in disk and export it.
* If not specified, the default schedule exporter will be used.
*/
public OpenTelemetryRumBuilder setExportScheduleHandler(
ExportScheduleHandler exportScheduleHandler) {
this.exportScheduleHandler = exportScheduleHandler;
return this;
}

private StorageConfiguration createStorageConfiguration() throws IOException {
Preferences preferences = getServiceManager().getPreferences();
CacheStorage storage = getServiceManager().getCacheStorage();
DiskBufferingConfiguration config = this.config.getDiskBufferingConfiguration();
DiskManager diskManager = new DiskManager(storage, preferences, config);
return StorageConfiguration.builder()
Expand All @@ -347,11 +372,18 @@ private StorageConfiguration createStorageConfiguration(ServiceManager serviceMa
.build();
}

private void scheduleDiskTelemetryReader(
@Nullable SignalFromDiskExporter signalExporter,
DiskBufferingConfiguration diskBufferingConfiguration) {
ExportScheduleHandler exportScheduleHandler =
diskBufferingConfiguration.getExportScheduleHandler();
private void scheduleDiskTelemetryReader(@Nullable SignalFromDiskExporter signalExporter) {

if (exportScheduleHandler == null) {
ServiceManager serviceManager = getServiceManager();
// TODO: Is it safe to get the work service yet here? If so, we can
// avoid all this lazy supplier stuff....
Function0<PeriodicWorkService> getWorkService = serviceManager::getPeriodicWorkService;
exportScheduleHandler =
new DefaultExportScheduleHandler(
new DefaultExportScheduler(getWorkService), getWorkService);
}

if (signalExporter == null) {
// Disabling here allows to cancel previously scheduled exports using tools that
// can run even after the app has been terminated (such as WorkManager).
Expand All @@ -378,8 +410,7 @@ public OpenTelemetryRumBuilder addOtelSdkReadyListener(Consumer<OpenTelemetrySdk
}

/** Leverage the configuration to wire up various instrumentation components. */
private void applyConfiguration(
ServiceManager serviceManager, InitializationEvents initializationEvents) {
private void applyConfiguration(InitializationEvents initializationEvents) {
if (config.shouldGenerateSdkInitializationEvents()) {
initializationEvents.recordConfiguration(config);
}
Expand All @@ -402,7 +433,7 @@ private void applyConfiguration(
(tracerProviderBuilder, app) -> {
SpanProcessor networkAttributesSpanAppender =
NetworkAttributesSpanAppender.create(
serviceManager.getCurrentNetworkProvider());
getServiceManager().getCurrentNetworkProvider());
return tracerProviderBuilder.addSpanProcessor(
networkAttributesSpanAppender);
});
Expand All @@ -415,7 +446,7 @@ private void applyConfiguration(
(tracerProviderBuilder, app) -> {
SpanProcessor screenAttributesAppender =
new ScreenAttributesSpanProcessor(
serviceManager.getVisibleScreenService());
getServiceManager().getVisibleScreenService());
return tracerProviderBuilder.addSpanProcessor(screenAttributesAppender);
});
}
Expand Down
4 changes: 3 additions & 1 deletion 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 @@ -8,6 +8,7 @@ package io.opentelemetry.android
import android.app.Application
import io.opentelemetry.android.instrumentation.AndroidInstrumentation
import io.opentelemetry.android.instrumentation.AndroidInstrumentationLoader
import io.opentelemetry.android.instrumentation.InstallationContext
import io.opentelemetry.android.internal.services.ServiceManager
import io.opentelemetry.android.session.SessionManager
import io.opentelemetry.sdk.OpenTelemetrySdk
Expand Down Expand Up @@ -53,8 +54,9 @@ class SdkPreconfiguredRumBuilder
val openTelemetryRum = OpenTelemetryRumImpl(sdk, sessionManager)

// Install instrumentations
val ctx = InstallationContext(application, openTelemetryRum.openTelemetry, serviceManager)
for (instrumentation in getInstrumentations()) {
instrumentation.install(application, openTelemetryRum.openTelemetry)
instrumentation.install(ctx)
}

return openTelemetryRum
Expand Down
19 changes: 0 additions & 19 deletions ...main/java/io/opentelemetry/android/features/diskbuffering/DiskBufferingConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,6 @@

package io.opentelemetry.android.features.diskbuffering;

import io.opentelemetry.android.features.diskbuffering.scheduler.DefaultExportScheduleHandler;
import io.opentelemetry.android.features.diskbuffering.scheduler.ExportScheduleHandler;
import java.util.concurrent.TimeUnit;
import java.util.logging.Level;
import java.util.logging.Logger;
Expand All @@ -18,15 +16,13 @@ public final class DiskBufferingConfiguration {

private final boolean enabled;
private final int maxCacheSize;
private final ExportScheduleHandler exportScheduleHandler;
private final long maxFileAgeForWriteMillis;
private final long minFileAgeForReadMillis;
private final long maxFileAgeForReadMillis;

private DiskBufferingConfiguration(Builder builder) {
enabled = builder.enabled;
maxCacheSize = builder.maxCacheSize;
exportScheduleHandler = builder.exportScheduleHandler;
maxFileAgeForWriteMillis = builder.maxFileAgeForWriteMillis;
minFileAgeForReadMillis = builder.minFileAgeForReadMillis;
maxFileAgeForReadMillis = builder.maxFileAgeForReadMillis;
Expand All @@ -48,10 +44,6 @@ public int getMaxCacheFileSize() {
return MAX_FILE_SIZE;
}

public ExportScheduleHandler getExportScheduleHandler() {
return exportScheduleHandler;
}

public long getMaxFileAgeForWriteMillis() {
return maxFileAgeForWriteMillis;
}
Expand All @@ -71,8 +63,6 @@ public static final class Builder {
private long minFileAgeForReadMillis = TimeUnit.SECONDS.toMillis(33);
private long maxFileAgeForReadMillis = TimeUnit.HOURS.toMillis(18);

private ExportScheduleHandler exportScheduleHandler = DefaultExportScheduleHandler.create();

/** Enables or disables disk buffering. */
public Builder setEnabled(boolean enabled) {
this.enabled = enabled;
Expand Down Expand Up @@ -113,15 +103,6 @@ public Builder setMaxCacheSize(int maxCacheSize) {
return this;
}

/**
* Sets a scheduler that will take care of periodically read data stored in disk and export
* it.
*/
public Builder setExportScheduleHandler(ExportScheduleHandler exportScheduleHandler) {
this.exportScheduleHandler = exportScheduleHandler;
return this;
}

public DiskBufferingConfiguration build() {
// See note in StorageConfiguration.getMinFileAgeForReadMillis()
if (minFileAgeForReadMillis <= maxFileAgeForWriteMillis) {
Expand Down
12 changes: 0 additions & 12 deletions ...io/opentelemetry/android/features/diskbuffering/scheduler/DefaultExportScheduleHandler.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@

package io.opentelemetry.android.features.diskbuffering.scheduler

import io.opentelemetry.android.internal.services.ServiceManager
import io.opentelemetry.android.internal.services.periodicwork.PeriodicWorkService
import java.util.concurrent.atomic.AtomicBoolean

Expand All @@ -22,15 +21,4 @@ class DefaultExportScheduleHandler(
periodicWorkService.enqueue(exportScheduler)
}
}

companion object {
@JvmStatic
fun create(): DefaultExportScheduleHandler {
return DefaultExportScheduleHandler(
DefaultExportScheduler.create(),
) {
ServiceManager.get().getPeriodicWorkService()
}
}
}
}
7 changes: 0 additions & 7 deletions .../java/io/opentelemetry/android/features/diskbuffering/scheduler/DefaultExportScheduler.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ package io.opentelemetry.android.features.diskbuffering.scheduler
import android.util.Log
import io.opentelemetry.android.common.RumConstants.OTEL_RUM_LOG_TAG
import io.opentelemetry.android.features.diskbuffering.SignalFromDiskExporter
import io.opentelemetry.android.internal.services.ServiceManager
import io.opentelemetry.android.internal.services.periodicwork.PeriodicRunnable
import io.opentelemetry.android.internal.services.periodicwork.PeriodicWorkService
import java.io.IOException
Expand All @@ -18,12 +17,6 @@ class DefaultExportScheduler(periodicWorkServiceProvider: () -> PeriodicWorkServ
PeriodicRunnable(periodicWorkServiceProvider) {
companion object {
private val DELAY_BEFORE_NEXT_EXPORT_IN_MILLIS = TimeUnit.SECONDS.toMillis(10)

fun create(): DefaultExportScheduler {
return DefaultExportScheduler {
ServiceManager.get().getPeriodicWorkService()
}
}
}

override fun onRun() {
Expand Down
10 changes: 2 additions & 8 deletions core/src/main/java/io/opentelemetry/android/instrumentation/AndroidInstrumentation.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,7 @@

package io.opentelemetry.android.instrumentation

import android.app.Application
import io.opentelemetry.android.OpenTelemetryRum
import io.opentelemetry.api.OpenTelemetry

/**
* This interface defines a tool that automatically generates telemetry for a specific use-case,
Expand All @@ -30,11 +28,7 @@ interface AndroidInstrumentation {
* only be called from [OpenTelemetryRum]'s builder once the [OpenTelemetryRum] instance is initialized and ready
* to use for generating telemetry.
*
* @param application The Android application being instrumented.
* @param openTelemetry The [OpenTelemetry] instance to use for creating signals.
* @param ctx The InstallationContext under which the instrumentation is being installed.
*/
fun install(
application: Application,
openTelemetry: OpenTelemetry,
)
fun install(ctx: InstallationContext)
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 this idea. I still need to take a deeper look at the whole PR so I can provide a better review overall, though this part reminded me of this other PR which makes it a bit confusing to me because there it seemed like we wanted to avoid passing core-specific types to instrumentations? I'm ok with doing so btw, I just want to make sure what's the overall consensus of the kind of use cases we want to support for instrumentations (whether we need them to be fully independent or not).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And when you say "core-specific types" in this case you mean both the InstallationContext itself but also the ServiceManager contained within?

In any case, the api needs to have something to pass to install(), and instrumentations might want to leverage the consistent way we've started setting up to get at services. I'm open to other ways that don't involve global/static/singletons. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

And when you say "core-specific types" in this case you mean both the InstallationContext itself but also the ServiceManager contained within?

Yes, that's what I was thinking.

In any case, the api needs to have something to pass to install(), and instrumentations might want to leverage the consistent way we've started setting up to get at services. I'm open to other ways that don't involve global/static/singletons. :)

Makes sense, we still need to pass something to instrumentations, and I like this idea of passing a single object of ours because it means that we could add more tools in the future if needed without having to change the signature of install().

I think the solution to this will become clearer once we fully understand the problem, if it's a matter of having "zero dependency" on anything that's outside of the upstream java repo, then things might get quite complicated, however, if it's just a matter of package size concerns and having a lot of code from a dependency that might not be fully used, such as the Android core, then it's probably a matter of extracting the instrumentation-related files out of core, as @surbhiia mentioned here, where we can also add a new type like InstallationContext. If I understood correctly, I think the problem is the latter, so maybe we can get away with just moving this type into the separate "instrumentation API" module. Now, what to do about the service manager is another issue though, I hope we can discuss the details properly in a SIG meeting.

Copy link
Contributor

Choose a reason for hiding this comment

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

While I agree that there are probably several layers of functionality we can split this out into that will allow the interfaces, singletons, and config objects be more decoupled, it may still be useful to maintain this uber object for init that references all the layers for convenience - the alternative is to have N different objects and have them passed in separately. At that point, it's more of an aesthetic choice, and I don't particular care too much :-)

}
16 changes: 16 additions & 0 deletions core/src/main/java/io/opentelemetry/android/instrumentation/InstallationContext.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.android.instrumentation

import android.app.Application
import io.opentelemetry.android.internal.services.ServiceManager
import io.opentelemetry.api.OpenTelemetry

data class InstallationContext(
val application: Application,
val openTelemetry: OpenTelemetry,
val serviceManager: ServiceManager,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we make the application and serviceManager nullable as they're not needed in all instrumentations?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Possibly, but I think we mostly want to use the same context when installing all instrumentations at startup -- in which case some of the instrumentations will need some of these, so they won't be null.

I think it's simpler if instrumentations just ignore (don't use) the parts they don't need.

Copy link
Contributor

@surbhiia surbhiia Oct 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

That makes sense! We can perhaps provide a getter for a no-op ServiceManager instance to support the use cases when a user is utilizing one/some of the many instrumentations like the okhttp3 instrumentation that does not require a ServiceManager?

Or we can skip that too assuming such users can bypass the AndroidInstrumentation interface and directly call the underlying code under install method but providing a no-op could be beneficial for users who want to leverage the
AndroidInstrumentation interface too for wrapping in their own discover instrumentations feature. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of dealing with nulls, why not make them interfaces that we can provide Fakes to during a test?

If it's all Kotlin, then nulls are fine if it's handled downstream in only a few key places, but because there's still so much Java, I'd go with Fake or no-op implementations so we never ever get NPEs, but also will be able to bypass this during tests. Another advantage of that is there's no need to mock any of these during tests - just instantiate a the fake and you're good to go.

Copy link
Contributor

Choose a reason for hiding this comment

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

Based on yesterday's SIG meeting, we should try to find a way to allow instrumentations to reuse services/utilities without having to expose them as public apis. There's more details about it all on this issue.

)
Loading