Skip to content

Commit

Permalink
refactor: remove strict mode config
Browse files Browse the repository at this point in the history
  • Loading branch information
fractalwrench committed May 9, 2024
1 parent 461946e commit f803561
Show file tree
Hide file tree
Showing 17 changed files with 38 additions and 139 deletions.
6 changes: 2 additions & 4 deletions embrace-android-sdk/config/detekt/baseline.xml
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,9 @@
<SmellBaseline>
<ManuallySuppressedIssues></ManuallySuppressedIssues>
<CurrentIssues>
<ID>ArgumentListWrapping:LegacyExceptionInfoTest.kt$LegacyExceptionInfoTest$( "io.embrace.android.embracesdk.LegacyExceptionInfoTest.testOfThrowable(LegacyExceptionInfoTest.kt:45)", info.lines.first())</ID>
<ID>ArgumentListWrapping:LegacyExceptionInfoTest.kt$LegacyExceptionInfoTest$("io.embrace.android.embracesdk.LegacyExceptionInfoTest.testOfThrowable(LegacyExceptionInfoTest.kt:45)", info.lines.first())</ID>
<ID>DataClassContainsFunctions:LegacyExceptionError.kt$LegacyExceptionError$fun addException(ex: Throwable?, appState: String?, clock: Clock)</ID>
<ID>DataClassContainsFunctions:LegacyExceptionError.kt$LegacyExceptionError$private fun getExceptionInfo(ex: Throwable?): List&lt;LegacyExceptionInfo&gt;</ID>
<ID>DataClassShouldBeImmutable:LegacyExceptionError.kt$LegacyExceptionError$@Json(name = "c") var occurrences = 0</ID>
<ID>DataClassShouldBeImmutable:LegacyExceptionError.kt$LegacyExceptionError$@Json(name = "rep") var exceptionErrors = mutableListOf&lt;LegacyExceptionErrorInfo&gt;()</ID>
<ID>DataClassShouldBeImmutable:LegacyExceptionError.kt$LegacyExceptionError$@Json(name = "c") var occurrences: Int = 0</ID>
<ID>DataClassShouldBeImmutable:LegacyExceptionError.kt$LegacyExceptionError$@Json(name = "rep") var exceptionErrors: MutableList&lt;LegacyExceptionErrorInfo&gt; = mutableListOf()</ID>
</CurrentIssues>
</SmellBaseline>
Original file line number Diff line number Diff line change
Expand Up @@ -22,22 +22,9 @@ internal class SessionBehavior(
) {

companion object {

/**
* By default, prevents to capture internal error logs as part of session payload
*/
const val ERROR_LOG_STRICT_MODE_DEFAULT = false

const val SESSION_PROPERTY_LIMIT = 10
}

/**
* Whether the limit on the number of internal exceptions in the payload should be increased
* for strict mode.
*/
fun isSessionErrorLogStrictModeEnabled(): Boolean =
local?.sessionEnableErrorLogStrictMode ?: ERROR_LOG_STRICT_MODE_DEFAULT

/**
* The whitelist of events (crashes, errors) that should send a full session payload even
* if the gating feature is enabled.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,5 @@ internal class SessionLocalConfig(
* gating feature is enabled.
*/
@Json(name = "send_full_for")
val fullSessionEvents: Set<String>? = null,

/**
* Local/Internal logs with ERROR severity are going to be captured as part of our session payload tp monitor potential issues
*/
@Json(name = "error_log_strict_mode")
val sessionEnableErrorLogStrictMode: Boolean? = null
val fullSessionEvents: Set<String>? = null
)
Original file line number Diff line number Diff line change
Expand Up @@ -18,16 +18,14 @@ internal class SdkObservabilityModuleImpl(
essentialServiceModule: EssentialServiceModule
) : SdkObservabilityModule {

private val logStrictMode by lazy {
val configService = essentialServiceModule.configService
configService.sessionBehavior.isSessionErrorLogStrictModeEnabled()
}

override val internalErrorService: InternalErrorService by singleton {
EmbraceInternalErrorService(essentialServiceModule.processStateService, initModule.clock, logStrictMode)
EmbraceInternalErrorService(
essentialServiceModule.processStateService,
initModule.clock
)
}

override val reportingLoggerAction: ReportingLoggerAction by singleton {
ReportingLoggerAction(internalErrorService, logStrictMode)
ReportingLoggerAction(internalErrorService)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,7 @@ import java.net.UnknownServiceException
*/
internal class EmbraceInternalErrorService(
private val processStateService: ProcessStateService,
private val clock: Clock,
private val logStrictMode: Boolean
private val clock: Clock
) : InternalErrorService {
private var configService: ConfigService? = null
private var err: LegacyExceptionError? = null
Expand Down Expand Up @@ -95,7 +94,7 @@ internal class EmbraceInternalErrorService(
return
}
if (err == null) {
err = LegacyExceptionError(logStrictMode)
err = LegacyExceptionError()
}

// if the config service has not been set yet, capture the exception
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,7 @@ package io.embrace.android.embracesdk.logging
import android.util.Log

internal class ReportingLoggerAction(
private val internalErrorService: InternalErrorService,
private val logStrictMode: Boolean = false
private val internalErrorService: InternalErrorService
) : InternalEmbraceLogger.LoggerAction {

override fun log(
Expand All @@ -13,24 +12,16 @@ internal class ReportingLoggerAction(
throwable: Throwable?,
logStacktrace: Boolean
) {
val finalThrowable = when {
logStrictMode && severity == InternalEmbraceLogger.Severity.ERROR && throwable == null -> LogStrictModeException(
msg
)
else -> throwable
}

if (finalThrowable != null) {
if (throwable != null) {
try {
internalErrorService.handleInternalError(finalThrowable)
internalErrorService.handleInternalError(throwable)
} catch (e: Throwable) {
// Yo dawg, I heard you like to handle internal errors...
Log.w(EMBRACE_TAG, msg, e)
}
}
}

class LogStrictModeException(msg: String) : Exception(msg)
class InternalError(msg: String) : Exception(msg)
class NotAnException(msg: String) : Exception(msg)
}
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,14 @@ import io.embrace.android.embracesdk.internal.payload.InternalError
* Describes an Exception Error with a count of occurrences and a list of exceptions (causes).
*/
@JsonClass(generateAdapter = true)
internal data class LegacyExceptionError(@Transient private val logStrictMode: Boolean = false) {
internal data class LegacyExceptionError(

@Json(name = "c")
var occurrences = 0
var occurrences: Int = 0,

@Json(name = "rep")
var exceptionErrors = mutableListOf<LegacyExceptionErrorInfo>()
var exceptionErrors: MutableList<LegacyExceptionErrorInfo> = mutableListOf()
) {

/**
* Add a new exception error info if exceptionError's size is below 20.
Expand All @@ -26,11 +27,7 @@ internal data class LegacyExceptionError(@Transient private val logStrictMode: B
*/
fun addException(ex: Throwable?, appState: String?, clock: Clock) {
occurrences++
var exceptionsLimits = DEFAULT_EXCEPTION_ERROR_LIMIT
if (logStrictMode) {
exceptionsLimits = DEFAULT_EXCEPTION_ERROR_LIMIT_STRICT_MODE
}
if (exceptionErrors.size < exceptionsLimits) {
if (exceptionErrors.size < DEFAULT_EXCEPTION_ERROR_LIMIT) {
exceptionErrors.add(
LegacyExceptionErrorInfo(
clock.now(),
Expand Down Expand Up @@ -61,4 +58,3 @@ internal data class LegacyExceptionError(@Transient private val logStrictMode: B
* The occurrences list limit.
*/
private const val DEFAULT_EXCEPTION_ERROR_LIMIT = 10
private const val DEFAULT_EXCEPTION_ERROR_LIMIT_STRICT_MODE = 50
Original file line number Diff line number Diff line change
Expand Up @@ -159,18 +159,6 @@ internal class LocalConfigTest {
@Test
fun testSessionOnlyConfig() {
var localConfig = LocalConfigParser.buildConfig(
"GrCPU",
false,
"{\"session\": {\"error_log_strict_mode\": true}}",
serializer,
logger
)
assertTrue(
checkNotNull(localConfig.sdkConfig.sessionConfig?.sessionEnableErrorLogStrictMode)
)

// receive a session component to restrict session messages
localConfig = LocalConfigParser.buildConfig(
"GrCPU",
false,
"{\"session\": {\"components\": [\"breadcrumbs_taps\"]}}",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ internal class SessionTest {
startupThreshold = 5000,
sdkStartupDuration = 109,
unhandledExceptions = 1,
exceptionError = LegacyExceptionError(false),
exceptionError = LegacyExceptionError(),
orientations = listOf(Orientation(1, 16092342200)),
properties = mapOf("fake-key" to "fake-value"),
symbols = mapOf("fake-native-key" to "fake-native-value"),
Expand Down Expand Up @@ -92,7 +92,7 @@ internal class SessionTest {
assertEquals(5000L, startupThreshold)
assertEquals(109L, sdkStartupDuration)
assertEquals(1, unhandledExceptions)
assertEquals(LegacyExceptionError(false), exceptionError)
assertEquals(LegacyExceptionError(), exceptionError)
assertEquals(listOf(Orientation(1, 16092342200)), orientations)
assertEquals(mapOf("fake-key" to "fake-value"), properties)
assertEquals(mapOf("fake-native-key" to "fake-native-value"), symbols)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,7 @@ internal class SessionBehaviorTest {

private val local = SessionLocalConfig(
sessionComponents = setOf("breadcrumbs"),
fullSessionEvents = setOf("crash"),
sessionEnableErrorLogStrictMode = true
fullSessionEvents = setOf("crash")
)

private val remote = RemoteConfig(
Expand All @@ -30,7 +29,6 @@ internal class SessionBehaviorTest {
@Test
fun testDefaults() {
with(fakeSessionBehavior()) {
assertFalse(isSessionErrorLogStrictModeEnabled())
assertEquals(emptySet<String>(), getFullSessionEvents())
assertNull(getSessionComponents())
assertFalse(isGatingFeatureEnabled())
Expand All @@ -42,7 +40,6 @@ internal class SessionBehaviorTest {
@Test
fun testLocalOnly() {
with(fakeSessionBehavior(localCfg = { local })) {
assertTrue(isSessionErrorLogStrictModeEnabled())
assertEquals(setOf("breadcrumbs"), getSessionComponents())
assertEquals(setOf("crash"), getFullSessionEvents())
assertTrue(isGatingFeatureEnabled())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import io.embrace.android.embracesdk.deserializeEmptyJsonString
import io.embrace.android.embracesdk.deserializeJsonFromResource
import org.junit.Assert.assertEquals
import org.junit.Assert.assertNull
import org.junit.Assert.assertTrue
import org.junit.Test

internal class SessionLocalConfigTest {
Expand All @@ -18,7 +17,6 @@ internal class SessionLocalConfigTest {
@Test
fun testDeserialization() {
val obj = deserializeJsonFromResource<SessionLocalConfig>("session_config.json")
assertTrue(checkNotNull(obj.sessionEnableErrorLogStrictMode))
assertEquals(setOf("breadcrumbs"), obj.sessionComponents)
assertEquals(setOf("crash"), obj.fullSessionEvents)
}
Expand All @@ -30,7 +28,6 @@ internal class SessionLocalConfigTest {
}

private fun verifyDefaults(cfg: SessionLocalConfig) {
assertNull(cfg.sessionEnableErrorLogStrictMode)
assertNull(cfg.sessionComponents)
assertNull(cfg.fullSessionEvents)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,7 @@ import io.embrace.android.embracesdk.logging.ReportingLoggerAction
internal class FakeSdkObservabilityModule(
override val internalErrorService: InternalErrorService = EmbraceInternalErrorService(
FakeProcessStateService(),
FakeClock(),
true
FakeClock()
)
) : SdkObservabilityModule {

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ internal class EmbraceInternalErrorServiceTest {
@Before
fun setUp() {
activityService = FakeProcessStateService()
service = EmbraceInternalErrorService(activityService, clock, false)
service = EmbraceInternalErrorService(activityService, clock)
cfg = RemoteConfig()
cfgService =
FakeConfigService(dataCaptureEventBehavior = fakeDataCaptureEventBehavior { cfg })
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,9 @@ internal class ReportingLoggerActionTest {
private lateinit var reportingLoggerAction: ReportingLoggerAction
private lateinit var loggerAction: FakeLoggerAction

private fun setupService(strictModeEnabled: Boolean = false) {
private fun setupService() {
internalErrorService = mockk(relaxUnitFun = true)
reportingLoggerAction = ReportingLoggerAction(internalErrorService, strictModeEnabled)
reportingLoggerAction = ReportingLoggerAction(internalErrorService)
}

@Before
Expand All @@ -39,57 +39,28 @@ internal class ReportingLoggerActionTest {
setupService()
reportingLoggerAction.log(errorMsg, InternalEmbraceLogger.Severity.DEBUG, exception, true)

verify { internalErrorService.handleInternalError(exception) }
verify(exactly = 1) { internalErrorService.handleInternalError(exception) }
}

@Test
fun `if an exception is thrown reporting error, swallow it`() {
setupService()
every { internalErrorService.handleInternalError(exception) } throws RuntimeException()
reportingLoggerAction.log(errorMsg, InternalEmbraceLogger.Severity.DEBUG, exception, true)
verify { internalErrorService.handleInternalError(exception) }
verify(exactly = 1) { internalErrorService.handleInternalError(exception) }
}

@Test
fun `if logStrictMode is enabled and a throwable is available with ERROR severity`() {
setupService(true)
reportingLoggerAction.log(errorMsg, InternalEmbraceLogger.Severity.ERROR, exception, true)
verify { internalErrorService.handleInternalError(exception) }
}

@Test
fun `if logStrictMode is enabled and a throwable is not available with ERROR severity then handle exception`() {
setupService(true)
reportingLoggerAction.log(errorMsg, InternalEmbraceLogger.Severity.ERROR, null, true)
verify(exactly = 1) {
internalErrorService.handleInternalError(
any() as ReportingLoggerAction.LogStrictModeException
)
}
}

@Test
fun `if logStrictMode is enabled and a throwable is not available with INFO severity then dont handle exception`() {
setupService(true)
fun `if a throwable is not available with INFO severity then dont handle exception`() {
setupService()
reportingLoggerAction.log(errorMsg, InternalEmbraceLogger.Severity.INFO, null, true)
verify(exactly = 0) { internalErrorService.handleInternalError(any() as Exception) }
}

@Test
fun `if logStrictMode is disabled and a throwable is available with ERROR severity`() {
setupService(false)
fun `if a throwable is available with ERROR severity`() {
setupService()
reportingLoggerAction.log(errorMsg, InternalEmbraceLogger.Severity.ERROR, exception, true)
verify { internalErrorService.handleInternalError(exception) }
}

@Test
fun `if logStrictMode is enabled and an exception is thrown, swallow it`() {
setupService(true)
every {
internalErrorService.handleInternalError(any() as ReportingLoggerAction.LogStrictModeException)
} throws RuntimeException()

reportingLoggerAction.log(errorMsg, InternalEmbraceLogger.Severity.DEBUG, exception, true)
verify { internalErrorService.handleInternalError(any() as ReportingLoggerAction.LogStrictModeException) }
verify(exactly = 1) { internalErrorService.handleInternalError(exception) }
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ internal class BackgroundActivityTest {
infoLogsAttemptedToSend = 1,
warnLogsAttemptedToSend = 2,
errorLogsAttemptedToSend = 3,
exceptionError = LegacyExceptionError(false),
exceptionError = LegacyExceptionError(),
crashReportId = "fake-crash-id",
endType = Session.LifeEventType.BKGND_STATE,
startType = Session.LifeEventType.BKGND_STATE,
Expand Down Expand Up @@ -65,7 +65,7 @@ internal class BackgroundActivityTest {
assertEquals(Session.LifeEventType.BKGND_STATE, endType)
assertEquals(Session.LifeEventType.BKGND_STATE, startType)
assertEquals(1, unhandledExceptions)
assertEquals(LegacyExceptionError(false), exceptionError)
assertEquals(LegacyExceptionError(), exceptionError)
assertEquals(mapOf("fake-key" to "fake-value"), properties)
}
}
Expand Down

0 comments on commit f803561

Please sign in to comment.