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

Improve top error analytics #50

Merged
merged 3 commits into from
Oct 18, 2024
Merged

Improve top error analytics #50

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
48ed722
Add additional data to error analytics
Oct 15, 2024
582404e
Isolate out exoplayer ParserException
Oct 16, 2024
ca195fa
Add internet connectivity check to error analytics
Oct 18, 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
24 changes: 14 additions & 10 deletions Armadillo/src/main/java/com/scribd/armadillo/download/DownloadManagerExt.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@ internal data class TestableDownloadState(val id: Int,
val url: String,
val state: Int,
val downloadPercentage: Int,
val downloadedBytes: Long) {
val downloadedBytes: Long,
val failureReason: Int? = null) {
companion object {
const val QUEUED = ExoplayerDownload.STATE_QUEUED
const val COMPLETED = ExoplayerDownload.STATE_COMPLETED
Expand All @@ -24,11 +25,12 @@ internal data class TestableDownloadState(val id: Int,
}

constructor(download: ExoplayerDownload) : this(
download.request.data.decodeToInt(),
download.request.uri.toString(),
download.state,
download.percentDownloaded.toInt(),
download.bytesDownloaded)
download.request.data.decodeToInt(),
download.request.uri.toString(),
download.state,
download.percentDownloaded.toInt(),
download.bytesDownloaded,
download.failureReason)

/**
* This method converts [TestableDownloadState] (a testable wrapper fo exoplayer's [DownloadManager.TaskState])
Expand All @@ -47,13 +49,15 @@ internal data class TestableDownloadState(val id: Int,
}
DownloadState.STARTED(percent, downloadedBytes)
}

QUEUED -> return null
else -> DownloadState.FAILED
else -> DownloadState.FAILED(failureReason)
}

return DownloadProgressInfo(
id = id,
url = url,
downloadState = downloadState)
id = id,
url = url,
downloadState = downloadState,
exoPlayerDownloadState = state)
}
}
15 changes: 10 additions & 5 deletions Armadillo/src/main/java/com/scribd/armadillo/download/ExoplayerDownloadTracker.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,6 @@ import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.launch
import kotlinx.coroutines.withContext
import java.io.IOException
import java.lang.Exception
import java.util.HashMap
import javax.inject.Inject
import javax.inject.Named
import javax.inject.Singleton
Expand Down Expand Up @@ -111,7 +109,7 @@ internal class ExoplayerDownloadTracker @Inject constructor(

override fun updateProgress() {
downloadManager.currentDownloads.forEach { download ->
if(downloads.containsKey(download.request.uri.toString())){
if (downloads.containsKey(download.request.uri.toString())) {
downloads[download.request.uri.toString()] = download //older usage
} else {
downloads[download.request.id] = download
Expand All @@ -129,7 +127,7 @@ internal class ExoplayerDownloadTracker @Inject constructor(

override fun onDownloadChanged(downloadManager: DownloadManager, download: Download, finalException: Exception?) {
Log.v(TAG, "onDownloadChanged")
if(downloads.containsKey(download.request.uri.toString())){
if (downloads.containsKey(download.request.uri.toString())) {
downloads[download.request.uri.toString()] = download //older usage
} else {
downloads[download.request.id] = download
Expand All @@ -155,6 +153,7 @@ internal class ExoplayerDownloadTracker @Inject constructor(
@VisibleForTesting
fun dispatchActionsForProgress(downloadInfo: DownloadProgressInfo) {
val taskFailed = downloadInfo.isFailed()
val exoplayerDownloadState = downloadInfo.exoPlayerDownloadState
val isRemoveDownloadComplete = downloadInfo.downloadState is DownloadState.REMOVED
val isDownloadComplete = downloadInfo.isDownloaded()

Expand All @@ -173,7 +172,13 @@ internal class ExoplayerDownloadTracker @Inject constructor(
}

if (taskFailed) {
actions.add(ErrorAction(DownloadFailed()))
actions.add(ErrorAction(DownloadFailed(
mapOf(
"exo_player_download_state" to exoplayerDownloadState.toString(),
"is_download_complete" to isDownloadComplete.toString(),
"failure_reason" to (downloadInfo.downloadState as? DownloadState.FAILED)?.failureReason.toString()
)
)))
}

actions.forEach {
Expand Down
15 changes: 12 additions & 3 deletions Armadillo/src/main/java/com/scribd/armadillo/error/ArmadilloException.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,12 @@ class InvalidRequest(message: String)
/**
* Playback Errors
*/
data class HttpResponseCodeException(val responseCode: Int, val url: String?, override val cause: Exception)
: ArmadilloException(cause = cause, message = "HTTP Error $responseCode.", isNetworkRelatedError = true) {
data class HttpResponseCodeException(
val responseCode: Int,
val url: String?,
override val cause: Exception,
val extraData: Map<String, String> = emptyMap(),
) : ArmadilloException(cause = cause, message = "HTTP Error $responseCode.", isNetworkRelatedError = true) {
override val errorCode: Int = 200
}

Expand Down Expand Up @@ -96,6 +100,11 @@ class ConnectivityException(cause: Exception)
override val errorCode: Int = 206
}

class ParsingException(cause: Exception)
: ArmadilloException(cause = cause, message = "The content cannot be parsed and it is not playable: ${cause.message}") {
override val errorCode = 207
}

/**
* Download Errors
*/
Expand All @@ -104,7 +113,7 @@ class MissingInfoDownloadException(message: String)
override val errorCode = 301
}

class DownloadFailed
class DownloadFailed(val extraData: Map<String, String>)
: ArmadilloException(message = "The download has failed to finish.", isNetworkRelatedError = true) {
override val errorCode = 302
}
Expand Down
15 changes: 8 additions & 7 deletions Armadillo/src/main/java/com/scribd/armadillo/models/ArmadilloState.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,11 +98,12 @@ data class MediaControlState(
data class DownloadProgressInfo(
val id: Int,
val url: String,
val downloadState: DownloadState) {
val downloadState: DownloadState,
val exoPlayerDownloadState: Int? = null) {

fun isDownloaded(): Boolean = DownloadState.COMPLETED == downloadState

fun isFailed(): Boolean = DownloadState.FAILED == downloadState
fun isFailed(): Boolean = downloadState is DownloadState.FAILED

companion object {
const val PROGRESS_UNSET = C.PERCENTAGE_UNSET
Expand All @@ -129,25 +130,25 @@ sealed class DownloadState {
override fun toString() = "REMOVED"
}

object FAILED : DownloadState() {
data class FAILED(val failureReason: Int? = null) : DownloadState() {
override fun toString() = "FAILED"
}
}

data class InternalState(val isPlaybackEngineReady: Boolean = false)

sealed class DrmState (val drmType: DrmType?, val expireMillis: Milliseconds, val isSessionValid: Boolean) {
sealed class DrmState(val drmType: DrmType?, val expireMillis: Milliseconds, val isSessionValid: Boolean) {
/** This Content is not utilizing DRM protections, or is now first initializing **/
object NoDRM : DrmState(null, 0.milliseconds, true)

/** Attempt to open the license and decrypt */
class LicenseOpening(drmType: DrmType?, expireMillis: Milliseconds = 0.milliseconds): DrmState(drmType, expireMillis, true)
class LicenseOpening(drmType: DrmType?, expireMillis: Milliseconds = 0.milliseconds) : DrmState(drmType, expireMillis, true)

/** A DRM License has been obtained. */
class LicenseAcquired(drmType: DrmType, expireMillis: Milliseconds): DrmState(drmType, expireMillis, true)
class LicenseAcquired(drmType: DrmType, expireMillis: Milliseconds) : DrmState(drmType, expireMillis, true)

/** The player encountered an expiration event */
class LicenseExpired(drmType: DrmType?, expireMillis: Milliseconds): DrmState(drmType, expireMillis, false)
class LicenseExpired(drmType: DrmType?, expireMillis: Milliseconds) : DrmState(drmType, expireMillis, false)

/** A DRM license exists and content is able to be decrypted. */
class LicenseUsable(drmType: DrmType?, expireMillis: Milliseconds) : DrmState(drmType, expireMillis, true)
Expand Down
41 changes: 36 additions & 5 deletions Armadillo/src/main/java/com/scribd/armadillo/playback/ExoPlaybackExceptionExt.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,18 +3,22 @@ package com.scribd.armadillo.playback
import com.google.android.exoplayer2.ExoPlaybackException
import com.google.android.exoplayer2.ExoPlaybackException.TYPE_RENDERER
import com.google.android.exoplayer2.ExoPlaybackException.TYPE_SOURCE
import com.google.android.exoplayer2.ParserException
import com.google.android.exoplayer2.audio.AudioSink
import com.google.android.exoplayer2.drm.MediaDrmCallbackException
import com.google.android.exoplayer2.upstream.DataSpec
import com.google.android.exoplayer2.upstream.HttpDataSource
import com.scribd.armadillo.error.ArmadilloException
import com.scribd.armadillo.error.ArmadilloIOException
import com.scribd.armadillo.error.HttpResponseCodeException
import com.scribd.armadillo.error.ConnectivityException
import com.scribd.armadillo.error.HttpResponseCodeException
import com.scribd.armadillo.error.ParsingException
import com.scribd.armadillo.error.RendererConfigurationException
import com.scribd.armadillo.error.RendererInitializationException
import com.scribd.armadillo.error.RendererWriteException
import com.scribd.armadillo.error.UnexpectedException
import com.scribd.armadillo.error.UnknownRendererException
import java.io.IOException
import java.net.SocketTimeoutException
import java.net.UnknownHostException

Expand All @@ -23,16 +27,30 @@ internal fun ExoPlaybackException.toArmadilloException(): ArmadilloException {
return this.sourceException.let { source ->
when (source) {
is HttpDataSource.InvalidResponseCodeException ->
HttpResponseCodeException(source.responseCode, source.dataSpec.uri.toString(), source)
HttpResponseCodeException(source.responseCode, source.dataSpec.uri.toString(), source, source.dataSpec.toAnalyticsMap())

is HttpDataSource.HttpDataSourceException ->
HttpResponseCodeException(0, source.dataSpec.uri.toString(), source)
HttpResponseCodeException(source.reason, source.dataSpec.uri.toString(), source, source.dataSpec.toAnalyticsMap())

is MediaDrmCallbackException -> {
val httpCause = source.cause as? HttpDataSource.InvalidResponseCodeException
HttpResponseCodeException(httpCause?.responseCode ?: 0, httpCause?.dataSpec?.uri.toString(), source)
HttpResponseCodeException(httpCause?.responseCode
?: 0, httpCause?.dataSpec?.uri.toString(), source, source.dataSpec.toAnalyticsMap())
}

is UnknownHostException,
is SocketTimeoutException -> ConnectivityException(source)
else -> ArmadilloIOException(cause = this, actionThatFailedMessage = "Exoplayer error.")

else -> {
var cause: Throwable? = source
while (source.cause != null && cause !is ParserException) {
cause = source.cause
}
when (cause) {
is ParserException -> ParsingException(cause = this)
else -> ArmadilloIOException(cause = this, actionThatFailedMessage = "Exoplayer error.")
}
}
}
}
} else if (TYPE_RENDERER == type) {
Expand All @@ -47,4 +65,17 @@ internal fun ExoPlaybackException.toArmadilloException(): ArmadilloException {
} else {
UnexpectedException(cause = this, actionThatFailedMessage = "Exoplayer error")
}
}

private fun DataSpec.toAnalyticsMap(): Map<String, String> {
return mapOf(
"uri" to uri.toString(),
Copy link
Contributor

Choose a reason for hiding this comment

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

In general it is ok to have .toString() but we won't be able to differentiate between value being null vs empty string right? We can bubble that up as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This uri property is always non null -- also toString() returns "null" for null objects and "" for empty string

"uriPositionOffset" to uriPositionOffset.toString(),
"httpMethod" to httpMethod.toString(),
"position" to position.toString(),
"length" to length.toString(),
"key" to key.toString(),
"flags" to flags.toString(),
"customData" to customData.toString()
)
sidkaria marked this conversation as resolved.
Show resolved Hide resolved
}
25 changes: 13 additions & 12 deletions Armadillo/src/test/java/com/scribd/armadillo/download/DownloadManagerExtKtTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,14 +15,15 @@ class DownloadManagerExtKtTest {
}

private lateinit var downloadState: TestableDownloadState

@Before
fun setUp() {
downloadState = TestableDownloadState(
ID,
URL,
TestableDownloadState.COMPLETED,
DOWNLOAD_PERCENT,
DOWNLOADED_BYTES)
ID,
URL,
TestableDownloadState.COMPLETED,
DOWNLOAD_PERCENT,
DOWNLOADED_BYTES)
}

@Test
Expand All @@ -35,7 +36,7 @@ class DownloadManagerExtKtTest {
@Test
fun toDownloadInfo_downloadRemovalComplete_returnsRemoveProgress() {
val state = downloadState.copy(
state = TestableDownloadState.REMOVING)
state = TestableDownloadState.REMOVING)
val downloadInfo = state.toDownloadInfo()!!
assertThat(downloadInfo.id).isEqualTo(ID)
assertThat(downloadInfo.url).isEqualTo(URL)
Expand All @@ -45,7 +46,7 @@ class DownloadManagerExtKtTest {
@Test
fun toDownloadInfo_downloadComplete_returnsCompletedProgress() {
val state = downloadState.copy(
state = TestableDownloadState.COMPLETED)
state = TestableDownloadState.COMPLETED)
val downloadInfo = state.toDownloadInfo()!!
assertThat(downloadInfo.id).isEqualTo(ID)
assertThat(downloadInfo.url).isEqualTo(URL)
Expand All @@ -55,8 +56,8 @@ class DownloadManagerExtKtTest {
@Test
fun toDownloadInfo_downloadProgressJustBegan_returnsProgress() {
val state = downloadState.copy(
state = TestableDownloadState.IN_PROGRESS,
downloadPercentage = DownloadProgressInfo.PROGRESS_UNSET)
state = TestableDownloadState.IN_PROGRESS,
downloadPercentage = DownloadProgressInfo.PROGRESS_UNSET)
val downloadInfo = state.toDownloadInfo()!!
assertThat(downloadInfo.id).isEqualTo(ID)
assertThat(downloadInfo.url).isEqualTo(URL)
Expand All @@ -66,7 +67,7 @@ class DownloadManagerExtKtTest {
@Test
fun toDownloadInfo_downloadProgressWithProgress_returnsProgress() {
val state = downloadState.copy(
state = TestableDownloadState.IN_PROGRESS)
state = TestableDownloadState.IN_PROGRESS)
val downloadInfo = state.toDownloadInfo()!!
assertThat(downloadInfo.id).isEqualTo(ID)
assertThat(downloadInfo.url).isEqualTo(URL)
Expand All @@ -76,10 +77,10 @@ class DownloadManagerExtKtTest {
@Test
fun toDownloadInfo_unknownState_returnsFailed() {
val state = downloadState.copy(
state = 1000)
state = 1000)
val downloadInfo = state.toDownloadInfo()!!
assertThat(downloadInfo.id).isEqualTo(ID)
assertThat(downloadInfo.url).isEqualTo(URL)
assertThat(downloadInfo.downloadState).isEqualTo(DownloadState.FAILED)
assertThat(downloadInfo.downloadState).isEqualTo(DownloadState.FAILED())
}
}
2 changes: 1 addition & 1 deletion Armadillo/src/test/java/com/scribd/armadillo/download/ExoplayerDownloadTrackerTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ class ExoplayerDownloadTrackerTest {

@Test
fun dispatchActionsForProgress_taskFailed_dispatchesActions() {
downloadInfo = DownloadProgressInfo(ID, URL, DownloadState.FAILED)
downloadInfo = DownloadProgressInfo(ID, URL, DownloadState.FAILED())
exoplayerDownloadTracker.dispatchActionsForProgress(downloadInfo)
verify(stateModifier).dispatch(UpdateDownloadAction(downloadInfo))
verify(stateModifier).dispatch(isA<ErrorAction>())
Expand Down
Loading