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.

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
Show file tree
Hide file tree
Changes from all commits
Commits
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 Armadillo/src/main/AndroidManifest.xml
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
xmlns:android="http://schemas.android.com/apk/res/android"
xmlns:tools="http://schemas.android.com/tools">

<uses-permission android:name="android.permission.ACCESS_NETWORK_STATE" />
<uses-permission android:name="android.permission.INTERNET"/>
<uses-permission android:name="android.permission.RECEIVE_BOOT_COMPLETED"/>
<uses-permission android:name="android.permission.FOREGROUND_SERVICE"/>
Expand Down
17 changes: 17 additions & 0 deletions Armadillo/src/main/java/com/scribd/armadillo/Util.kt
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package com.scribd.armadillo

import android.content.Context
import android.net.ConnectivityManager
import android.net.NetworkCapabilities
import android.os.Build
import androidx.annotation.ChecksSdkIntAtLeast
import com.scribd.armadillo.models.Chapter
Expand Down Expand Up @@ -47,5 +49,20 @@ fun sanitizeChapters(chapters: List<Chapter>): List<Chapter> {
}
}

fun isInternetAvailable(context: Context): Boolean {
val connectivityManager =
context.getSystemService(Context.CONNECTIVITY_SERVICE) as ConnectivityManager
val networkCapabilities = connectivityManager.activeNetwork ?: return false
val actNw =
connectivityManager.getNetworkCapabilities(networkCapabilities) ?: return false
return when {
actNw.hasTransport(NetworkCapabilities.TRANSPORT_WIFI) -> true
actNw.hasTransport(NetworkCapabilities.TRANSPORT_CELLULAR) -> true
actNw.hasTransport(NetworkCapabilities.TRANSPORT_ETHERNET) -> true
actNw.hasTransport(NetworkCapabilities.TRANSPORT_VPN) -> true
else -> false
}
}

@ChecksSdkIntAtLeast(api = Build.VERSION_CODES.S)
fun hasSnowCone() = Build.VERSION.SDK_INT >= Build.VERSION_CODES.S
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import com.scribd.armadillo.playback.MediaMetadataCompatBuilderImpl
import com.scribd.armadillo.playback.PlaybackEngineFactoryHolder
import com.scribd.armadillo.playback.PlaybackStateBuilderImpl
import com.scribd.armadillo.playback.PlaybackStateCompatBuilder
import com.scribd.armadillo.playback.PlayerEventListener
import com.scribd.armadillo.playback.mediasource.DrmMediaSourceHelper
import com.scribd.armadillo.playback.mediasource.DrmMediaSourceHelperImpl
import com.scribd.armadillo.playback.mediasource.HeadersMediaSourceFactoryFactory
Expand Down Expand Up @@ -75,4 +76,8 @@ internal class PlaybackModule {
@Provides
@Singleton
fun drmSessionManagerProvider(stateStore: StateStore.Modifier): DrmSessionManagerProvider = ArmadilloDrmSessionManagerProvider(stateStore)

@Provides
@Singleton
fun playerEventListener(context: Context): PlayerEventListener = PlayerEventListener(context)
}
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)
}
}
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
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
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
Original file line number Diff line number Diff line change
@@ -1,38 +1,58 @@
package com.scribd.armadillo.playback

import android.content.Context
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 com.scribd.armadillo.isInternetAvailable
import java.net.SocketTimeoutException
import java.net.UnknownHostException

internal fun ExoPlaybackException.toArmadilloException(): ArmadilloException {
internal fun ExoPlaybackException.toArmadilloException(context: Context): ArmadilloException {
return if (TYPE_SOURCE == type) {
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
(context))

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

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(context))
}

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 +67,18 @@ internal fun ExoPlaybackException.toArmadilloException(): ArmadilloException {
} else {
UnexpectedException(cause = this, actionThatFailedMessage = "Exoplayer error")
}
}

private fun DataSpec.toAnalyticsMap(context: Context): 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(),
"isInternetConnected" to isInternetAvailable(context).toString(),
)
sidkaria marked this conversation as resolved.
Show resolved Hide resolved
}
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,8 @@ internal class ExoplayerPlaybackEngine(private var audioPlayable: AudioPlayable)
@VisibleForTesting
internal lateinit var exoPlayer: ExoPlayer

private val playerEventListener = PlayerEventListener()
@Inject
internal lateinit var playerEventListener: PlayerEventListener

override val currentChapterIndex: Int
get() = audioPlayable.chapters.indexOf(currentChapter)
Expand Down Expand Up @@ -155,7 +156,7 @@ internal class ExoplayerPlaybackEngine(private var audioPlayable: AudioPlayable)
stateModifier.dispatch(PlaybackEngineReady(true))
stateModifier.dispatch(PlayerStateAction(PlaybackState.PAUSED))
} catch (ex: Exception) {
val armadilloException = if(ex is ArmadilloException) ex else PlaybackStartFailureException(cause = ex)
val armadilloException = if (ex is ArmadilloException) ex else PlaybackStartFailureException(cause = ex)
stateModifier.dispatch(ErrorAction(armadilloException))
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package com.scribd.armadillo.playback

import android.content.Context
import android.util.Log
import com.google.android.exoplayer2.ExoPlaybackException
import com.google.android.exoplayer2.ExoPlayer
Expand All @@ -22,7 +23,7 @@ import javax.inject.Inject
*
* It communicates changes by sending [Action]s with [StateStore.Modifier].
*/
internal class PlayerEventListener : Player.Listener {
internal class PlayerEventListener @Inject constructor(private val context: Context) : Player.Listener {
init {
Injector.mainComponent.inject(this)
}
Expand All @@ -35,7 +36,7 @@ internal class PlayerEventListener : Player.Listener {
internal lateinit var stateModifier: StateStore.Modifier

override fun onPlayerError(error: PlaybackException) {
val exception = (error as ExoPlaybackException).toArmadilloException()
val exception = (error as ExoPlaybackException).toArmadilloException(context)
stateModifier.dispatch(ErrorAction(exception))
Log.e(TAG, "onPlayerError: $error")
}
Expand Down
Loading
Loading