Sulkta-OSS/element-x-ada
3
0
Fork 0
Code Issues 1 Pull requests 10 Projects Releases Packages Wiki Activity Actions

Simplify DefaultBugReporter and ensure response is closed #2905

Browse source
This commit is contained in:
Benoit Marty 2024-06-04 16:01:01 +02:00 • committed by Benoit Marty
parent 2dee312ba1
commit 39e95496cb
6 changed files with 191 additions and 226 deletions
Download patch file Download diff file Expand all files Collapse all files

6
features/rageshake/api/src/main/kotlin/io/element/android/features/rageshake/api/reporter/BugReporter.kt
View file

@ -25,7 +25,7 @@ interface BugReporter {
* @param withDevicesLogs true to include the device log * @param withDevicesLogs true to include the device log
* @param withCrashLogs true to include the crash logs * @param withCrashLogs true to include the crash logs
* @param withScreenshot true to include the screenshot * @param withScreenshot true to include the screenshot
* @param theBugDescription the bug description * @param problemDescription the bug description
* @param canContact true if the user opt in to be contacted directly * @param canContact true if the user opt in to be contacted directly
* @param listener the listener * @param listener the listener
*/ */
@ -33,9 +33,9 @@ interface BugReporter {
withDevicesLogs: Boolean, withDevicesLogs: Boolean,
withCrashLogs: Boolean, withCrashLogs: Boolean,
withScreenshot: Boolean, withScreenshot: Boolean,
theBugDescription: String, problemDescription: String,
canContact: Boolean = false, canContact: Boolean = false,
listener: BugReporterListener? listener: BugReporterListener
) )
/** /**

2
features/rageshake/impl/src/main/kotlin/io/element/android/features/rageshake/impl/bugreport/BugReportPresenter.kt
View file

@ -143,7 +143,7 @@ class BugReportPresenter @Inject constructor(
withDevicesLogs = formState.sendLogs, withDevicesLogs = formState.sendLogs,
withCrashLogs = hasCrashLogs && formState.sendLogs, withCrashLogs = hasCrashLogs && formState.sendLogs,
withScreenshot = formState.sendScreenshot, withScreenshot = formState.sendScreenshot,
theBugDescription = formState.description, problemDescription = formState.description,
canContact = formState.canContact, canContact = formState.canContact,
listener = listener listener = listener
) )

353
features/rageshake/impl/src/main/kotlin/io/element/android/features/rageshake/impl/reporter/DefaultBugReporter.kt
View file

@ -44,7 +44,6 @@ import io.element.android.libraries.sessionstorage.api.SessionStore
import kotlinx.coroutines.CancellationException import kotlinx.coroutines.CancellationException
import kotlinx.coroutines.flow.first import kotlinx.coroutines.flow.first
import kotlinx.coroutines.withContext import kotlinx.coroutines.withContext
import okhttp3.Call
import okhttp3.MediaType.Companion.toMediaTypeOrNull import okhttp3.MediaType.Companion.toMediaTypeOrNull
import okhttp3.OkHttpClient import okhttp3.OkHttpClient
import okhttp3.Request import okhttp3.Request
@ -89,11 +88,6 @@ class DefaultBugReporter @Inject constructor(
private const val LOG_DIRECTORY_NAME = "logs" private const val LOG_DIRECTORY_NAME = "logs"
} }
// the pending bug report call
private var bugReportCall: Call? = null
// boolean to cancel the bug report
private val isCancelled = false
private val logcatCommandDebug = arrayOf("logcat", "-d", "-v", "threadtime", "*:*") private val logcatCommandDebug = arrayOf("logcat", "-d", "-v", "threadtime", "*:*")
private var currentTracingFilter: String? = null private var currentTracingFilter: String? = null
@ -103,252 +97,197 @@ class DefaultBugReporter @Inject constructor(
withDevicesLogs: Boolean, withDevicesLogs: Boolean,
withCrashLogs: Boolean, withCrashLogs: Boolean,
withScreenshot: Boolean, withScreenshot: Boolean,
theBugDescription: String, problemDescription: String,
canContact: Boolean, canContact: Boolean,
listener: BugReporterListener? listener: BugReporterListener,
) { ) {
// enumerate files to delete // enumerate files to delete
val bugReportFiles: MutableList<File> = ArrayList() val bugReportFiles: MutableList<File> = ArrayList()
var response: Response? = null
try { try {
var serverError: String? = null var serverError: String? = null
withContext(coroutineDispatchers.io) { withContext(coroutineDispatchers.io) {
var bugDescription = theBugDescription
val crashCallStack = crashDataStore.crashInfo().first() val crashCallStack = crashDataStore.crashInfo().first()
val bugDescription = buildString {
if (crashCallStack.isNotEmpty() && withCrashLogs) { append(problemDescription)
bugDescription += "\n\n\n\n--------------------------------- crash call stack ---------------------------------\n" if (crashCallStack.isNotEmpty() && withCrashLogs) {
bugDescription += crashCallStack append("\n\n\n\n--------------------------------- crash call stack ---------------------------------\n")
append(crashCallStack)
}
} }
val gzippedFiles = mutableListOf<File>()
val gzippedFiles = ArrayList<File>()
if (withDevicesLogs) { if (withDevicesLogs) {
val files = getLogFiles().sortedByDescending { it.lastModified() } val files = getLogFiles().sortedByDescending { it.lastModified() }
files.mapNotNullTo(gzippedFiles) { f -> files.mapNotNullTo(gzippedFiles) { file ->
when { when {
isCancelled -> null file.extension == "gz" -> file
f.extension == "gz" -> f else -> compressFile(file)
else -> compressFile(f)
} }
} }
files.deleteAllExceptMostRecent() files.deleteAllExceptMostRecent()
} }
if (withCrashLogs || withDevicesLogs) {
if (!isCancelled && (withCrashLogs || withDevicesLogs)) {
saveLogCat() saveLogCat()
val gzippedLogcat = compressFile(logCatErrFile) val gzippedLogcat = compressFile(logCatErrFile)
if (null != gzippedLogcat) { if (gzippedLogcat != null) {
if (gzippedFiles.isEmpty()) { gzippedFiles.add(0, gzippedLogcat)
gzippedFiles.add(gzippedLogcat)
} else {
gzippedFiles.add(0, gzippedLogcat)
}
} }
} }
val sessionData = sessionStore.getLatestSession() val sessionData = sessionStore.getLatestSession()
val deviceId = sessionData?.deviceId ?: "undefined" val deviceId = sessionData?.deviceId ?: "undefined"
val userId = sessionData?.userId?.let { UserId(it) } val userId = sessionData?.userId?.let { UserId(it) }
// build the multi part request
if (!isCancelled) { val builder = BugReporterMultipartBody.Builder()
// build the multi part request .addFormDataPart("text", bugDescription)
val builder = BugReporterMultipartBody.Builder() .addFormDataPart("app", context.getString(R.string.bug_report_app_name))
.addFormDataPart("text", bugDescription) .addFormDataPart("user_agent", userAgentProvider.provide())
.addFormDataPart("app", context.getString(R.string.bug_report_app_name)) .addFormDataPart("user_id", userId?.toString() ?: "undefined")
.addFormDataPart("user_agent", userAgentProvider.provide()) .addFormDataPart("can_contact", canContact.toString())
.addFormDataPart("user_id", userId?.toString() ?: "undefined") .addFormDataPart("device_id", deviceId)
.addFormDataPart("can_contact", canContact.toString()) .addFormDataPart("device", Build.MODEL.trim())
.addFormDataPart("device_id", deviceId) .addFormDataPart("locale", Locale.getDefault().toString())
.apply { .addFormDataPart("sdk_sha", sdkMetadata.sdkGitSha)
userId?.let { .addFormDataPart("local_time", LocalDateTime.now().format(DateTimeFormatter.ISO_DATE_TIME))
matrixClientProvider.getOrNull(it)?.let { client -> .addFormDataPart("utc_time", LocalDateTime.ofInstant(Instant.now(), ZoneOffset.UTC).format(DateTimeFormatter.ISO_DATE_TIME))
val curveKey = client.encryptionService().deviceCurve25519() .addFormDataPart("app_id", buildMeta.applicationId)
val edKey = client.encryptionService().deviceEd25519() // Nightly versions have a custom version name suffix that we should remove for the bug report
if (curveKey != null && edKey != null) { .addFormDataPart("Version", buildMeta.versionName.replace("-nightly", ""))
addFormDataPart("device_keys", "curve25519:$curveKey, ed25519:$edKey") .addFormDataPart("label", buildMeta.versionName)
} .addFormDataPart("label", buildMeta.flavorDescription)
} .addFormDataPart("branch_name", buildMeta.gitBranchName)
} userId?.let {
} matrixClientProvider.getOrNull(it)?.let { client ->
.addFormDataPart("device", Build.MODEL.trim()) val curveKey = client.encryptionService().deviceCurve25519()
.addFormDataPart("locale", Locale.getDefault().toString()) val edKey = client.encryptionService().deviceEd25519()
.addFormDataPart("sdk_sha", sdkMetadata.sdkGitSha) if (curveKey != null && edKey != null) {
.addFormDataPart("local_time", LocalDateTime.now().format(DateTimeFormatter.ISO_DATE_TIME)) builder.addFormDataPart("device_keys", "curve25519:$curveKey, ed25519:$edKey")
.addFormDataPart("utc_time", LocalDateTime.ofInstant(Instant.now(), ZoneOffset.UTC).format(DateTimeFormatter.ISO_DATE_TIME))
.addFormDataPart("app_id", buildMeta.applicationId)
// Nightly versions have a custom version name suffix that we should remove for the bug report
.addFormDataPart("Version", buildMeta.versionName.replace("-nightly", ""))
currentTracingFilter?.let {
builder.addFormDataPart("tracing_filter", it)
}
// add the gzipped files, don't cancel the whole upload if only some file failed to upload
var totalUploadedSize = 0L
var uploadedSomeLogs = false
for (file in gzippedFiles) {
try {
val requestBody = file.asRequestBody(MimeTypes.OctetStream.toMediaTypeOrNull())
totalUploadedSize += requestBody.contentLength()
// If we are about to upload more than the max request size, stop here
if (totalUploadedSize > ApplicationConfig.MAX_LOG_UPLOAD_SIZE) {
Timber.e("Could not upload file ${file.name} because it would exceed the max request size")
break
}
builder.addFormDataPart("compressed-log", file.name, requestBody)
uploadedSomeLogs = true
} catch (e: CancellationException) {
throw e
} catch (e: Exception) {
Timber.e(e, "## sendBugReport() : fail to attach file ${file.name}")
} }
} }
}
bugReportFiles.addAll(gzippedFiles) if (crashCallStack.isNotEmpty() && withCrashLogs) {
builder.addFormDataPart("label", "crash")
if (gzippedFiles.isNotEmpty() && !uploadedSomeLogs) { }
serverError = "Couldn't upload any logs, please retry." currentTracingFilter?.let {
return@withContext builder.addFormDataPart("tracing_filter", it)
} }
// add the gzipped files, don't cancel the whole upload if only some file failed to upload
if (withScreenshot) { var totalUploadedSize = 0L
screenshotHolder.getFileUri() var uploadedSomeLogs = false
?.toUri() for (file in gzippedFiles) {
?.toFile()
?.let { screenshotFile ->
try {
builder.addFormDataPart(
"file",
screenshotFile.name,
screenshotFile.asRequestBody(MimeTypes.OctetStream.toMediaTypeOrNull())
)
} catch (e: Exception) {
Timber.e(e, "## sendBugReport() : fail to write screenshot")
}
}
}
// add some github labels
builder.addFormDataPart("label", buildMeta.versionName)
builder.addFormDataPart("label", buildMeta.flavorDescription)
builder.addFormDataPart("branch_name", buildMeta.gitBranchName)
if (crashCallStack.isNotEmpty() && withCrashLogs) {
builder.addFormDataPart("label", "crash")
}
val requestBody = builder.build()
// add a progress listener
requestBody.setWriteListener { totalWritten, contentLength ->
val percentage = if (-1L != contentLength) {
if (totalWritten > contentLength) {
100
} else {
(totalWritten * 100 / contentLength).toInt()
}
} else {
0
}
if (isCancelled && null != bugReportCall) {
bugReportCall!!.cancel()
}
Timber.v("## onWrite() : $percentage%")
try {
listener?.onProgress(percentage)
} catch (e: Exception) {
Timber.e(e, "## onProgress() : failed")
}
}
// build the request
val request = Request.Builder()
.url(bugReporterUrlProvider.provide())
.post(requestBody)
.build()
var responseCode = HttpURLConnection.HTTP_INTERNAL_ERROR
var response: Response? = null
var errorMessage: String? = null
// trigger the request
try { try {
bugReportCall = okHttpClient.get().newCall(request) val requestBody = file.asRequestBody(MimeTypes.OctetStream.toMediaTypeOrNull())
response = bugReportCall!!.execute() totalUploadedSize += requestBody.contentLength()
responseCode = response.code // If we are about to upload more than the max request size, stop here
if (totalUploadedSize > ApplicationConfig.MAX_LOG_UPLOAD_SIZE) {
Timber.e("Could not upload file ${file.name} because it would exceed the max request size")
break
}
builder.addFormDataPart("compressed-log", file.name, requestBody)
uploadedSomeLogs = true
} catch (e: CancellationException) { } catch (e: CancellationException) {
throw e throw e
} catch (e: Exception) { } catch (e: Exception) {
Timber.e(e, "response") Timber.e(e, "## sendBugReport() : fail to attach file ${file.name}")
errorMessage = e.localizedMessage
} }
}
// if the upload failed, try to retrieve the reason bugReportFiles.addAll(gzippedFiles)
if (responseCode != HttpURLConnection.HTTP_OK) { if (gzippedFiles.isNotEmpty() && !uploadedSomeLogs) {
if (null != errorMessage) { serverError = "Couldn't upload any logs, please retry."
serverError = "Failed with error $errorMessage" return@withContext
} else if (response?.body == null) { }
serverError = "Failed with error $responseCode" if (withScreenshot) {
screenshotHolder.getFileUri()
?.toUri()
?.toFile()
?.let { screenshotFile ->
try {
builder.addFormDataPart(
"file",
screenshotFile.name,
screenshotFile.asRequestBody(MimeTypes.OctetStream.toMediaTypeOrNull())
)
} catch (e: Exception) {
Timber.e(e, "## sendBugReport() : fail to write screenshot")
}
}
}
val requestBody = builder.build()
// add a progress listener
requestBody.setWriteListener { totalWritten, contentLength ->
val percentage = if (-1L != contentLength) {
if (totalWritten > contentLength) {
100
} else {
(totalWritten * 100 / contentLength).toInt()
}
} else {
0
}
Timber.v("## onWrite() : $percentage%")
listener.onProgress(percentage)
}
// build the request
val request = Request.Builder()
.url(bugReporterUrlProvider.provide())
.post(requestBody)
.build()
var errorMessage: String? = null
// trigger the request
try {
response = okHttpClient.get()
.newCall(request)
.execute()
} catch (e: CancellationException) {
throw e
} catch (e: Exception) {
Timber.e(e, "Error executing the request")
errorMessage = e.localizedMessage
}
val responseCode = response?.code
// if the upload failed, try to retrieve the reason
if (responseCode != HttpURLConnection.HTTP_OK) {
serverError = if (errorMessage != null) {
"Failed with error $errorMessage"
} else {
val responseBody = response?.body
if (responseBody == null) {
"Failed with error $responseCode"
} else { } else {
try { try {
val inputStream = response.body!!.byteStream() val inputStream = responseBody.byteStream()
serverError = inputStream.use { val serverErrorJson = inputStream.use {
buildString { it.readBytes().toString(Charsets.UTF_8)
var ch = it.read()
while (ch != -1) {
append(ch.toChar())
ch = it.read()
}
}
} }
// check if the error message try {
serverError?.let { val responseJSON = JSONObject(serverErrorJson)
try { responseJSON.getString("error")
val responseJSON = JSONObject(it) } catch (e: CancellationException) {
serverError = responseJSON.getString("error") throw e
} catch (e: CancellationException) { } catch (e: JSONException) {
throw e Timber.e(e, "Json conversion failed")
} catch (e: JSONException) { "Failed with error $responseCode"
Timber.e(e, "doInBackground ; Json conversion failed")
}
}
// should never happen
if (null == serverError) {
serverError = "Failed with error $responseCode"
} }
} catch (e: CancellationException) { } catch (e: CancellationException) {
throw e throw e
} catch (e: Exception) { } catch (e: Exception) {
Timber.e(e, "## sendBugReport() : failed to parse error") Timber.e(e, "## sendBugReport() : failed to parse error")
"Failed with error $responseCode"
} }
} }
} }
} }
} }
withContext(coroutineDispatchers.main) { if (serverError == null) {
bugReportCall = null listener.onUploadSucceed()
if (null != listener) { } else {
try { listener.onUploadFailed(serverError)
if (isCancelled) {
listener.onUploadCancelled()
} else if (null == serverError) {
listener.onUploadSucceed()
} else {
listener.onUploadFailed(serverError)
}
} catch (e: CancellationException) {
throw e
} catch (e: Exception) {
Timber.e(e, "## onPostExecute() : failed")
}
}
} }
} finally { } finally {
// delete the generated files when the bug report process has finished // delete the generated files when the bug report process has finished
for (file in bugReportFiles) { for (file in bugReportFiles) {
file.safeDelete() file.safeDelete()
} }
response?.close()
} }
} }
@ -424,17 +363,17 @@ class DefaultBugReporter @Inject constructor(
* @param streamWriter the stream writer * @param streamWriter the stream writer
*/ */
private fun getLogCatError(streamWriter: OutputStreamWriter) { private fun getLogCatError(streamWriter: OutputStreamWriter) {
val logcatProc: Process val logcatProcess: Process
try { try {
logcatProc = Runtime.getRuntime().exec(logcatCommandDebug) logcatProcess = Runtime.getRuntime().exec(logcatCommandDebug)
} catch (e1: IOException) { } catch (e1: IOException) {
return return
} }
try { try {
val separator = System.getProperty("line.separator") val separator = System.lineSeparator()
logcatProc.inputStream logcatProcess.inputStream
.reader() .reader()
.buffered(ApplicationConfig.MAX_LOG_UPLOAD_SIZE.toInt()) .buffered(ApplicationConfig.MAX_LOG_UPLOAD_SIZE.toInt())
.forEachLine { line -> .forEachLine { line ->

16
features/rageshake/impl/src/test/kotlin/io/element/android/features/rageshake/impl/bugreport/FakeBugReporter.kt
View file

@ -33,29 +33,29 @@ class FakeBugReporter(val mode: Mode = Mode.Success) : BugReporter {
withDevicesLogs: Boolean, withDevicesLogs: Boolean,
withCrashLogs: Boolean, withCrashLogs: Boolean,
withScreenshot: Boolean, withScreenshot: Boolean,
theBugDescription: String, problemDescription: String,
canContact: Boolean, canContact: Boolean,
listener: BugReporterListener?, listener: BugReporterListener,
) { ) {
delay(100) delay(100)
listener?.onProgress(0) listener.onProgress(0)
delay(100) delay(100)
listener?.onProgress(50) listener.onProgress(50)
delay(100) delay(100)
when (mode) { when (mode) {
Mode.Success -> Unit Mode.Success -> Unit
Mode.Failure -> { Mode.Failure -> {
listener?.onUploadFailed(A_FAILURE_REASON) listener.onUploadFailed(A_FAILURE_REASON)
return return
} }
Mode.Cancel -> { Mode.Cancel -> {
listener?.onUploadCancelled() listener.onUploadCancelled()
return return
} }
} }
listener?.onProgress(100) listener.onProgress(100)
delay(100) delay(100)
listener?.onUploadSucceed() listener.onUploadSucceed()
} }
override fun logDirectory(): File { override fun logDirectory(): File {

14
features/rageshake/impl/src/test/kotlin/io/element/android/features/rageshake/impl/reporter/DefaultBugReporterTest.kt
View file

@ -63,7 +63,7 @@ class DefaultBugReporterTest {
withDevicesLogs = true, withDevicesLogs = true,
withCrashLogs = true, withCrashLogs = true,
withScreenshot = true, withScreenshot = true,
theBugDescription = "a bug occurred", problemDescription = "a bug occurred",
canContact = true, canContact = true,
listener = object : BugReporterListener { listener = object : BugReporterListener {
override fun onUploadCancelled() { override fun onUploadCancelled() {
@ -130,7 +130,7 @@ class DefaultBugReporterTest {
withDevicesLogs = true, withDevicesLogs = true,
withCrashLogs = true, withCrashLogs = true,
withScreenshot = true, withScreenshot = true,
theBugDescription = "a bug occurred", problemDescription = "a bug occurred",
canContact = true, canContact = true,
listener = object : BugReporterListener { listener = object : BugReporterListener {
override fun onUploadCancelled() {} override fun onUploadCancelled() {}
@ -198,9 +198,9 @@ class DefaultBugReporterTest {
withDevicesLogs = true, withDevicesLogs = true,
withCrashLogs = true, withCrashLogs = true,
withScreenshot = true, withScreenshot = true,
theBugDescription = "a bug occurred", problemDescription = "a bug occurred",
canContact = true, canContact = true,
listener = null listener = FakeBugReporterListener(),
) )
val request = server.takeRequest() val request = server.takeRequest()
@ -240,9 +240,9 @@ class DefaultBugReporterTest {
withDevicesLogs = true, withDevicesLogs = true,
withCrashLogs = true, withCrashLogs = true,
withScreenshot = true, withScreenshot = true,
theBugDescription = "a bug occurred", problemDescription = "a bug occurred",
canContact = true, canContact = true,
listener = null listener = FakeBugReporterListener(),
) )
val request = server.takeRequest() val request = server.takeRequest()
@ -308,7 +308,7 @@ class DefaultBugReporterTest {
withDevicesLogs = true, withDevicesLogs = true,
withCrashLogs = true, withCrashLogs = true,
withScreenshot = true, withScreenshot = true,
theBugDescription = "a bug occurred", problemDescription = "a bug occurred",
canContact = true, canContact = true,
listener = object : BugReporterListener { listener = object : BugReporterListener {
override fun onUploadCancelled() { override fun onUploadCancelled() {

26
features/rageshake/impl/src/test/kotlin/io/element/android/features/rageshake/impl/reporter/FakeBugReporterListener.kt Normal file
View file

@ -0,0 +1,26 @@
/*
* Copyright (c) 2024 New Vector Ltd
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package io.element.android.features.rageshake.impl.reporter
import io.element.android.features.rageshake.api.reporter.BugReporterListener
class FakeBugReporterListener: BugReporterListener {
override fun onUploadCancelled() = Unit
override fun onUploadFailed(reason: String?) = Unit
override fun onProgress(progress: Int) = Unit
override fun onUploadSucceed() = Unit
}