Skip to content

Commit

Permalink
Reduce the direct use of the static logger so tests functional tests …
Browse files Browse the repository at this point in the history
…work properly (#643)

## Goal

Removed most of the direct use of the static logger, and instead rely on an instance that gets passed in that will be created in the InitModule. I also removed a lot of useless dev logging.

The crux of he change involved using the instance that get passed down via the modules init chain, and in places where it's not convenient, rely on the static logger instead. Both are hooked into the same internal error service instance so internal exceptions are reported.

The static logger will be slimmed down then completely removed, but I couldn't get away from having at least one massive change. There was no easy way of teasing these out without risking mistakes, and most of the changes are at least trivial. 

There are other refactors that I wanted to do and they were done in subsequent PRs. 

I'll will point to the right places to look but most of the other changes are just removing dev logging, adding the logger to a constructor, and passing it in when creating instances.

## Testing

Tests were modified as needed. Relying on existing coverage to ensure functionality worked as before, except for adding a test that verifies the static and instance loggers are both hooked up to the same internal error service.
  • Loading branch information
bidetofevil committed Apr 3, 2024
2 parents 4f3bb3c + 6d2e294 commit 740b7f5
Show file tree
Hide file tree
Showing 210 changed files with 1,128 additions and 1,502 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,9 @@ public class NullParametersTest {
private static final String NULL_STRING = null;

@Rule
public IntegrationTestRule testRule = new IntegrationTestRule();
public IntegrationTestRule testRule = new IntegrationTestRule(
() -> IntegrationTestRule.Companion.newHarness(true)
);

@NonNull
private final Embrace embrace = testRule.getEmbrace();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ import io.embrace.android.embracesdk.injection.StorageModuleImpl
import io.embrace.android.embracesdk.injection.SystemServiceModule
import io.embrace.android.embracesdk.injection.SystemServiceModuleImpl
import io.embrace.android.embracesdk.internal.utils.Provider
import io.embrace.android.embracesdk.logging.InternalStaticEmbraceLogger
import io.embrace.android.embracesdk.worker.WorkerThreadModule
import io.embrace.android.embracesdk.worker.WorkerThreadModuleImpl
import org.junit.rules.ExternalResource
Expand Down Expand Up @@ -102,15 +101,15 @@ internal class IntegrationTestRule(
val embraceImpl = EmbraceImpl(
ModuleInitBootstrapper(
initModule = initModule,
openTelemetryModule = initModule.openTelemetryModule,
coreModuleSupplier = { _, _ -> fakeCoreModule },
workerThreadModuleSupplier = { workerThreadModule },
openTelemetryModule = initModule.openTelemetryModule,
coreModuleSupplier = { _, _, _ -> fakeCoreModule },
systemServiceModuleSupplier = { _, _ -> systemServiceModule },
androidServicesModuleSupplier = { _, _, _ -> androidServicesModule },
workerThreadModuleSupplier = { _ -> workerThreadModule },
storageModuleSupplier = { _, _, _ -> storageModule },
essentialServiceModuleSupplier = { _, _, _, _, _, _, _, _, _ -> essentialServiceModule },
dataCaptureServiceModuleSupplier = { _, _, _, _, _, _, _ -> dataCaptureServiceModule },
deliveryModuleSupplier = { _, _, _, _ -> fakeDeliveryModule }
deliveryModuleSupplier = { _, _, _, _, _ -> fakeDeliveryModule },
)
)
Embrace.setImpl(embraceImpl)
Expand All @@ -124,7 +123,6 @@ internal class IntegrationTestRule(
* Teardown the Embrace SDK, closing any resources as required
*/
override fun after() {
InternalStaticEmbraceLogger.logger.setToDefault()
Embrace.getImpl().stop()
}

Expand All @@ -138,7 +136,7 @@ internal class IntegrationTestRule(
val appFramework: Embrace.AppFramework = Embrace.AppFramework.NATIVE,
val initModule: FakeInitModule = FakeInitModule(clock = fakeClock),
val openTelemetryModule: OpenTelemetryModule = initModule.openTelemetryModule,
val fakeCoreModule: FakeCoreModule = FakeCoreModule(appFramework = appFramework),
val fakeCoreModule: FakeCoreModule = FakeCoreModule(appFramework = appFramework, logger = initModule.logger),
val workerThreadModule: WorkerThreadModule = WorkerThreadModuleImpl(initModule),
val fakeConfigService: FakeConfigService = FakeConfigService(
backgroundActivityCaptureEnabled = true,
Expand Down Expand Up @@ -207,7 +205,9 @@ internal class IntegrationTestRule(
companion object {
const val DEFAULT_SDK_START_TIME_MS = 169220160000L

fun newHarness(startImmediately: Boolean) = Harness(startImmediately = startImmediately)
fun newHarness(startImmediately: Boolean): Harness {
return Harness(startImmediately = startImmediately)
}

private val DEFAULT_SDK_LOCAL_CONFIG = SdkLocalConfig(
networking = NetworkLocalConfig(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ package io.embrace.android.embracesdk.session
import androidx.test.ext.junit.runners.AndroidJUnit4
import io.embrace.android.embracesdk.FakeDeliveryService
import io.embrace.android.embracesdk.IntegrationTestRule
import io.embrace.android.embracesdk.IntegrationTestRule.*
import io.embrace.android.embracesdk.IntegrationTestRule.Harness
import io.embrace.android.embracesdk.config.remote.RemoteConfig
import io.embrace.android.embracesdk.config.remote.SessionRemoteConfig
import io.embrace.android.embracesdk.fakes.FakeConfigService
Expand All @@ -16,6 +16,7 @@ import io.embrace.android.embracesdk.gating.SessionGatingKeys
import io.embrace.android.embracesdk.getSentSessionMessages
import io.embrace.android.embracesdk.hasEvent
import io.embrace.android.embracesdk.hasSpan
import io.embrace.android.embracesdk.logging.InternalEmbraceLogger
import io.embrace.android.embracesdk.payload.SessionMessage
import io.embrace.android.embracesdk.recordSession
import io.embrace.android.embracesdk.session.orchestrator.SessionSnapshotType
Expand Down Expand Up @@ -43,8 +44,9 @@ internal class OtelSessionGatingTest {
FakeConfigService(
sessionBehavior = fakeSessionBehavior {
RemoteConfig(sessionConfig = gatingConfig)
}
)
},
),
InternalEmbraceLogger()
)

@Rule
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import io.embrace.android.embracesdk.fakes.injection.FakeWorkerThreadModule
import io.embrace.android.embracesdk.getLastSavedSessionMessage
import io.embrace.android.embracesdk.getLastSentSessionMessage
import io.embrace.android.embracesdk.recordSession
import io.embrace.android.embracesdk.worker.WorkerName.*
import io.embrace.android.embracesdk.worker.WorkerName.PERIODIC_CACHE
import org.junit.Assert.assertEquals
import org.junit.Assert.assertNull
import org.junit.Rule
Expand All @@ -29,7 +29,7 @@ internal class PeriodicSessionCacheTest {
IntegrationTestRule.Harness(
fakeClock = clock,
initModule = fakeInitModule,
workerThreadModule = FakeWorkerThreadModule(fakeInitModule, PERIODIC_CACHE)
workerThreadModule = FakeWorkerThreadModule(fakeInitModule = fakeInitModule, name = PERIODIC_CACHE)
)
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,11 @@ import android.os.Build.VERSION_CODES.TIRAMISU
import androidx.test.ext.junit.runners.AndroidJUnit4
import io.embrace.android.embracesdk.Embrace.AppFramework
import io.embrace.android.embracesdk.IntegrationTestRule
import io.embrace.android.embracesdk.assertions.assertInternalErrorLogged
import io.embrace.android.embracesdk.internal.ApkToolsConfig
import io.embrace.android.embracesdk.internal.TraceparentGeneratorTest.Companion.validPattern
import io.embrace.android.embracesdk.internalErrorService
import io.embrace.android.embracesdk.logging.InternalStaticEmbraceLogger
import io.embrace.android.embracesdk.recordSession
import org.junit.Assert.assertEquals
import org.junit.Assert.assertFalse
Expand Down Expand Up @@ -136,4 +139,23 @@ internal class PublicApiTest {
}
}
}

@Test
fun `static logger and SDK logger instance log to the same internal error service`() {
with(testRule) {
embrace.start(harness.fakeCoreModule.context)
InternalStaticEmbraceLogger.logger.logError("not-used", RuntimeException("static"), true)
assertInternalErrorLogged(
checkNotNull(internalErrorService()).currentExceptionError,
checkNotNull(RuntimeException::class.qualifiedName),
"static"
)
harness.initModule.logger.logError("not-used", RuntimeException("non-static"), true)
assertInternalErrorLogged(
checkNotNull(internalErrorService()).currentExceptionError,
checkNotNull(RuntimeException::class.qualifiedName),
"non-static"
)
}
}
}
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
package io.embrace.android.embracesdk;

import static io.embrace.android.embracesdk.logging.InternalStaticEmbraceLogger.logger;

import android.annotation.SuppressLint;
import android.content.Context;
import android.webkit.ConsoleMessage;
Expand All @@ -15,8 +13,6 @@
import io.embrace.android.embracesdk.annotation.InternalApi;
import io.embrace.android.embracesdk.internal.EmbraceInternalInterface;
import io.embrace.android.embracesdk.internal.Systrace;
import io.embrace.android.embracesdk.logging.InternalEmbraceLogger;
import io.embrace.android.embracesdk.logging.InternalStaticEmbraceLogger;
import io.embrace.android.embracesdk.network.EmbraceNetworkRequest;
import io.embrace.android.embracesdk.payload.PushNotificationBreadcrumb;
import io.embrace.android.embracesdk.spans.EmbraceSpan;
Expand All @@ -42,9 +38,6 @@ public final class Embrace implements EmbraceAndroidApi {

private static EmbraceImpl impl = Systrace.traceSynchronous("embrace-impl-init", EmbraceImpl::new);

@NonNull
private final InternalEmbraceLogger internalEmbraceLogger = InternalStaticEmbraceLogger.logger;

static final String NULL_PARAMETER_ERROR_MESSAGE_TEMPLATE = " cannot be invoked because it contains null parameters";

/**
Expand Down Expand Up @@ -560,13 +553,13 @@ public void logPushNotification(@Nullable String title,
@NonNull Boolean hasData) {
if (verifyNonNullParameters("logPushNotification", messageDeliveredPriority, isNotification, hasData)) {
impl.logPushNotification(
title,
body,
topic,
id,
notificationPriority,
messageDeliveredPriority,
PushNotificationBreadcrumb.NotificationType.Builder.notificationTypeFor(hasData, isNotification)
title,
body,
topic,
id,
notificationPriority,
messageDeliveredPriority,
PushNotificationBreadcrumb.NotificationType.Builder.notificationTypeFor(hasData, isNotification)
);
}
}
Expand All @@ -576,8 +569,6 @@ public void trackWebViewPerformance(@NonNull String tag, @NonNull ConsoleMessage
if (verifyNonNullParameters("trackWebViewPerformance", tag, consoleMessage)) {
if (consoleMessage.message() != null) {
trackWebViewPerformance(tag, consoleMessage.message());
} else {
logger.logDebug("Empty WebView console message.");
}
}
}
Expand Down Expand Up @@ -652,9 +643,7 @@ private boolean verifyNonNullParameters(@NonNull String functionName, @NonNull O
if (param == null) {
final String errorMessage = functionName + NULL_PARAMETER_ERROR_MESSAGE_TEMPLATE;
if (isStarted()) {
internalEmbraceLogger.logError(errorMessage, new IllegalArgumentException(errorMessage), true);
} else {
internalEmbraceLogger.logSDKNotInitialized(errorMessage);
impl.getEmbraceInternalInterface().logInternalError(new IllegalArgumentException(errorMessage));
}
return false;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,8 @@ import android.net.Uri
import android.os.Handler
import android.os.Looper
import android.widget.Toast
import io.embrace.android.embracesdk.logging.InternalStaticEmbraceLogger.Companion.logger
import io.embrace.android.embracesdk.logging.InternalEmbraceLogger
import io.embrace.android.embracesdk.logging.InternalStaticEmbraceLogger
import io.embrace.android.embracesdk.samples.AutomaticVerificationChecker
import io.embrace.android.embracesdk.samples.VerificationActions
import io.embrace.android.embracesdk.samples.VerifyIntegrationException
Expand All @@ -33,7 +34,8 @@ import kotlin.system.exitProcess
*
*/
internal class EmbraceAutomaticVerification(
private val scheduledExecutorService: ScheduledExecutorService = Executors.newSingleThreadScheduledExecutor()
private val scheduledExecutorService: ScheduledExecutorService = Executors.newSingleThreadScheduledExecutor(),
private val logger: InternalEmbraceLogger
) : ActivityLifecycleListener, ProcessStateListener {
private val handler = Handler(Looper.getMainLooper())

Expand All @@ -43,9 +45,9 @@ internal class EmbraceAutomaticVerification(

internal lateinit var processStateService: ProcessStateService

var automaticVerificationChecker = AutomaticVerificationChecker()
var automaticVerificationChecker = AutomaticVerificationChecker(logger)

var verificationActions = VerificationActions(Embrace.getInstance(), automaticVerificationChecker)
var verificationActions = VerificationActions(Embrace.getInstance(), logger, automaticVerificationChecker)

/**
* This flag track if the verification result popup was displayed or not,
Expand All @@ -59,7 +61,7 @@ internal class EmbraceAutomaticVerification(
private const val EMBRACE_CONTACT_EMAIL = "[email protected]"
private const val VERIFY_INTEGRATION_DELAY = 200L
private const val ON_FOREGROUND_TIMEOUT = 5000L
internal val instance = EmbraceAutomaticVerification()
internal val instance = EmbraceAutomaticVerification(logger = InternalStaticEmbraceLogger.logger)
}

fun verifyIntegration() {
Expand Down Expand Up @@ -154,10 +156,6 @@ internal class EmbraceAutomaticVerification(
try {
appInitializerClass = Class.forName("androidx.startup.AppInitializer")
} catch (cnfe: ClassNotFoundException) {
logger.logDeveloper(
"EmbraceAutomaticVerification",
"AppInitializer not found. Assuming that appCompat < 1.4.1"
)
return false
}

Expand All @@ -172,10 +170,6 @@ internal class EmbraceAutomaticVerification(
val result = isEagerlyInitialized.invoke(appInitializer, lifecycleInitializerClass) as Boolean
return result.not()
} ?: run {
logger.logDeveloper(
"EmbraceAutomaticVerification",
"Null application object, can not verify lifecycle annotations"
)
return false
}
} catch (e: Exception) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,6 @@
import io.embrace.android.embracesdk.logging.InternalEmbraceLogger;
import io.embrace.android.embracesdk.logging.InternalErrorLogger;
import io.embrace.android.embracesdk.logging.InternalErrorService;
import io.embrace.android.embracesdk.logging.InternalStaticEmbraceLogger;
import io.embrace.android.embracesdk.ndk.NativeModule;
import io.embrace.android.embracesdk.ndk.NdkService;
import io.embrace.android.embracesdk.network.EmbraceNetworkRequest;
Expand Down Expand Up @@ -98,15 +97,15 @@ final class EmbraceImpl {
@NonNull
private final AtomicBoolean started = new AtomicBoolean(false);

@NonNull
private final InternalEmbraceLogger internalEmbraceLogger = InternalStaticEmbraceLogger.logger;

@NonNull
private final ModuleInitBootstrapper moduleInitBootstrapper;

@NonNull
private final Clock sdkClock;

@NonNull
private final InternalEmbraceLogger internalEmbraceLogger;

/**
* Custom app ID that overrides the one specified at build time
*/
Expand Down Expand Up @@ -210,6 +209,7 @@ final class EmbraceImpl {
EmbraceImpl(@NonNull ModuleInitBootstrapper bs) {
moduleInitBootstrapper = bs;
sdkClock = moduleInitBootstrapper.getInitModule().getClock();
internalEmbraceLogger = moduleInitBootstrapper.getInitModule().getLogger();
tracer = moduleInitBootstrapper.getOpenTelemetryModule().getEmbraceTracer();
uninitializedSdkInternalInterface =
LazyKt.lazy(
Expand Down Expand Up @@ -264,7 +264,7 @@ private void startImpl(@NonNull Context context,
}

final long startTimeMs = sdkClock.now();
internalEmbraceLogger.logDeveloper("Embrace", "Starting SDK for framework " + framework.name());
internalEmbraceLogger.logInfo("Starting SDK for framework " + framework.name());
moduleInitBootstrapper.init(context, enableIntegrationTesting, framework, startTimeMs, customAppId);
Systrace.startSynchronous("post-services-setup");
telemetryService = moduleInitBootstrapper.getInitModule().getTelemetryService();
Expand Down Expand Up @@ -378,7 +378,6 @@ private void startImpl(@NonNull Context context,
// we went to the foreground, but if an activity had already gone to the foreground, we may have missed
// sending this, so to ensure the startup message is sent, we force it to be sent here.
if (!essentialServiceModule.getProcessStateService().isInBackground()) {
internalEmbraceLogger.logDeveloper("Embrace", "Sending startup moment");
dataContainerModule.getEventService().sendStartupMoment();
}

Expand Down Expand Up @@ -659,7 +658,7 @@ void startMoment(@NonNull String name,
@Nullable String identifier,
@Nullable Map<String, Object> properties) {
if (checkSdkStartedAndLogPublicApiUsage("start_moment")) {
eventService.startEvent(name, identifier, normalizeProperties(properties));
eventService.startEvent(name, identifier, normalizeProperties(properties, internalEmbraceLogger));
onActivityReported();
}
}
Expand All @@ -675,7 +674,7 @@ void startMoment(@NonNull String name,
*/
void endMoment(@NonNull String name, @Nullable String identifier, @Nullable Map<String, Object> properties) {
if (checkSdkStartedAndLogPublicApiUsage("end_moment")) {
eventService.endEvent(name, identifier, normalizeProperties(properties));
eventService.endEvent(name, identifier, normalizeProperties(properties, internalEmbraceLogger));
onActivityReported();
}
}
Expand Down Expand Up @@ -868,7 +867,7 @@ void logMessage(
message,
type,
logExceptionType,
normalizeProperties(properties),
normalizeProperties(properties, internalEmbraceLogger),
stackTraceElements,
customStackTrace,
appFramework,
Expand Down Expand Up @@ -1220,11 +1219,11 @@ private void sampleCurrentThreadDuringAnrs() {
}

@Nullable
private Map<String, Object> normalizeProperties(@Nullable Map<String, ?> properties) {
private Map<String, Object> normalizeProperties(@Nullable Map<String, ?> properties, @Nullable InternalEmbraceLogger logger) {
Map<String, Object> normalizedProperties = new HashMap<>();
if (properties != null) {
try {
normalizedProperties = PropertyUtils.sanitizeProperties(properties);
normalizedProperties = PropertyUtils.sanitizeProperties(properties, logger);
} catch (Exception e) {
internalEmbraceLogger.logError("Exception occurred while normalizing the properties.", e);
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package io.embrace.android.embracesdk

import io.embrace.android.embracesdk.annotation.InternalApi
import io.embrace.android.embracesdk.logging.InternalStaticEmbraceLogger
import io.embrace.android.embracesdk.samples.EmbraceCrashSamples

/**
Expand All @@ -14,7 +15,7 @@ import io.embrace.android.embracesdk.samples.EmbraceCrashSamples
public object EmbraceSamples {

private val embraceCrashSamples = EmbraceCrashSamples
private val embraceAutomaticVerification = EmbraceAutomaticVerification()
private val embraceAutomaticVerification = EmbraceAutomaticVerification(logger = InternalStaticEmbraceLogger.logger)

/**
* Starts an automatic verification of the following Embrace features:
Expand Down
Loading

0 comments on commit 740b7f5

Please sign in to comment.