Pr review comments frank (#3)

* Replace deprecated jcenter by mavencentral * Add threading annotations and fix some lint warnings * Improve readability of Prefs writes * Wrap some exceptions for more helpful logging * Change visibility of most classes to internal and add logging control * Add Seconds to minimumSubmitInterval name for clarity * Use coroutines for threading * Don't throw non-fatal Exceptions, log them instead * Don't ignore submitinterval for debug builds * Close connection in httpPost, where it is created * Remove loglevel instruction from code snippet to other part of README.md * Clarify Q42StatsConfig property names
Q42 · Sep 9, 2021 · 213a589 · 213a589
1 parent bb675d9
commit 213a589
Show file tree

Hide file tree

Showing 16 changed files with 106 additions and 84 deletions.
diff --git a/.idea/dictionaries/ninovanhooff.xml b/.idea/dictionaries/ninovanhooff.xml
diff --git a/.idea/jarRepositories.xml b/.idea/jarRepositories.xml
diff --git a/README.md b/README.md
@@ -3,7 +3,7 @@
 [![](https://jitci.com/gh/Q42/Q42Stats.Android/svg)](https://jitci.com/gh/Q42/Q42Stats.Android)
 
 
-Collect stats for Q42 internal usage, shared accross multiple Android projects.
+Collect stats for Q42 internal usage, shared across multiple Android projects.
 
 An iOS version is also available: https://github.com/Q42/Q42Stats
 
@@ -32,8 +32,8 @@ class SampleApplication : Application() {
         super.onCreate()
         Q42Stats(
             Q42StatsConfig(
-                fireBaseProject = "theProject",
-                firebaseCollection = "theCollection",
+                firebaseProjectId = "theProject",
+                firestoreCollectionId = "theCollection",
                 // wait at least 7.5 days between data collections. the extra .5 is for time-of-day randomization
                 minimumSubmitInterval = (60 * 60 * 24 * 7.5).toLong()
             )
@@ -45,6 +45,12 @@ This can be safely called from the main thread since all work (both collecting s
 
 It is safe to call this function multiple times, as it will exit immediately if it is already running or when a data collection interval has not passed yet.
 
+By default, Q42Stats only logs errors. For debugging purposes, set the log level before using Q42Stats:
+
+```
+Q42Stats.logLevel = Q42StatsLogLevel.Debug
+```
+
 ## Data collected
 
 Not all fields are supported on all versions of Android. If unsupported, the corresponding value may be false, "unknown" or the key may be completely omitted.

diff --git a/app/src/main/java/com/q42/q42stats/sample/SampleApplication.kt b/app/src/main/java/com/q42/q42stats/sample/SampleApplication.kt
@@ -10,10 +10,10 @@ class SampleApplication : Application() {
         super.onCreate()
         Q42Stats(
             Q42StatsConfig(
-                fireBaseProject = "theProject",
-                firebaseCollection = "theCollection",
+                firebaseProjectId = "theProject",
+                firebaseCollectionId = "theCollection",
                 // wait at least 7.5 days between data collections. the extra .5 is for time-of-day randomization
-                minimumSubmitInterval = (60 * 60 * 24 * 7.5).toLong()
+                minimumSubmitIntervalSeconds = (60 * 60 * 24 * 7.5).toLong()
             )
         ).runAsync(this.applicationContext)
     }

diff --git a/build.gradle b/build.gradle
@@ -3,7 +3,7 @@ buildscript {
     ext.kotlin_version = "1.4.32"
     repositories {
         google()
-        jcenter()
+        mavenCentral()
     }
     dependencies {
         classpath "com.android.tools.build:gradle:4.1.3"
@@ -17,7 +17,7 @@ buildscript {
 allprojects {
     repositories {
         google()
-        jcenter()
+        mavenCentral()
         maven { url "https://jitpack.io" }
     }
 }

diff --git a/library/build.gradle b/library/build.gradle
@@ -48,6 +48,8 @@ dependencies {
 
     implementation "org.jetbrains.kotlin:kotlin-stdlib:$kotlin_version"
     implementation 'org.jetbrains.kotlinx:kotlinx-coroutines-android:1.4.2'
+    implementation "androidx.annotation:annotation:1.2.0"
+
     testImplementation 'junit:junit:4.13.2'
     //json is included in Android, but not in Kotlin. This import makes it available for unit tests
     testImplementation 'org.json:json:20201115'

diff --git a/library/src/main/java/com/q42/q42stats/library/FireStoreUtils.kt b/library/src/main/java/com/q42/q42stats/library/FireStoreUtils.kt
@@ -15,7 +15,7 @@ import java.io.Serializable
  *     }
  * ```
  */
-fun Map<String, Serializable>.toFireStoreFormat(): JSONObject {
+internal fun Map<String, Serializable>.toFireStoreFormat(): JSONObject {
     val fireStoreMap = mapOf(
         "fields" to this.mapValues {
             mapOf("stringValue" to it.value.toString())

diff --git a/library/src/main/java/com/q42/q42stats/library/HttpService.kt b/library/src/main/java/com/q42/q42stats/library/HttpService.kt
@@ -1,48 +1,53 @@
 package com.q42.q42stats.library
 
-import org.json.JSONException
+import androidx.annotation.WorkerThread
 import org.json.JSONObject
 import java.io.BufferedWriter
-import java.io.IOException
 import java.io.OutputStreamWriter
 import java.net.HttpURLConnection
 import java.net.URL
 import javax.net.ssl.HttpsURLConnection
 
-object HttpService {
-    /** Synchronously send the stats. Make sure to run this on a worker thread */
+@WorkerThread
+internal object HttpService {
+
     fun sendStatsSync(config: Q42StatsConfig, data: JSONObject) {
         sendStatsSync(
-            "https://firestore.googleapis.com/v1/projects/${config.fireBaseProject}/" +
-                    "databases/(default)/documents/${config.firebaseCollection}?mask.fieldPaths=_",
+            "https://firestore.googleapis.com/v1/projects/${config.firebaseProjectId}/" +
+                    "databases/(default)/documents/${config.firebaseCollectionId}?mask.fieldPaths=_",
             data
         )
     }
 
-    /** Synchronously send the stats. Make sure to run this on a worker thread */
     private fun sendStatsSync(url: String, data: JSONObject) {
         httpPost(url, data)
     }
 
-    @Throws(IOException::class, JSONException::class)
     private fun httpPost(url: String, jsonObject: JSONObject) {
         val conn = URL(url).openConnection() as HttpsURLConnection
-        conn.requestMethod = "POST"
-        conn.setRequestProperty("Content-Type", "application/json; charset=utf-8")
-        setPostRequestContent(conn, jsonObject)
-        conn.connect()
-        Q42StatsLogger.d(TAG, "Response: ${conn.responseCode} ${conn.responseMessage}")
+        try {
+            conn.requestMethod = "POST"
+            conn.setRequestProperty("Content-Type", "application/json; charset=utf-8")
+            sendPostRequestContent(conn, jsonObject)
+        } catch (e: Throwable) {
+            Q42StatsLogger.e(TAG, "Could not send stats to server", e)
+        } finally {
+            conn.disconnect()
+        }
     }
 
-    @Throws(IOException::class)
-    private fun setPostRequestContent(conn: HttpURLConnection, jsonObject: JSONObject) {
-
-        val os = conn.outputStream
-        val writer = BufferedWriter(OutputStreamWriter(os, "UTF-8"))
-        writer.write(jsonObject.toString())
-        Q42StatsLogger.d(TAG, "Sending JSON: $jsonObject")
-        writer.flush()
-        writer.close()
-        os.close()
+    private fun sendPostRequestContent(conn: HttpURLConnection, jsonObject: JSONObject) {
+        try {
+            conn.outputStream.use { os ->
+                BufferedWriter(OutputStreamWriter(os, "UTF-8")).use { writer ->
+                    writer.write(jsonObject.toString())
+                    Q42StatsLogger.d(TAG, "Sending JSON: $jsonObject")
+                    writer.flush()
+                }
+            }
+            Q42StatsLogger.d(TAG, "Response: ${conn.responseCode} ${conn.responseMessage}")
+        } catch (e: Throwable) {
+            Q42StatsLogger.e(TAG, "Could not add data to POST request", e)
+        }
     }
 }
diff --git a/library/src/main/java/com/q42/q42stats/library/Q42Stats.kt b/library/src/main/java/com/q42/q42stats/library/Q42Stats.kt
@@ -1,61 +1,56 @@
 package com.q42.q42stats.library
 
 import android.content.Context
+import androidx.annotation.AnyThread
+import androidx.annotation.WorkerThread
 import com.q42.q42stats.library.collector.AccessibilityCollector
 import com.q42.q42stats.library.collector.PreferencesCollector
 import com.q42.q42stats.library.collector.SystemCollector
-import kotlinx.coroutines.Dispatchers
-import kotlinx.coroutines.GlobalScope
-import kotlinx.coroutines.launch
+import kotlinx.coroutines.*
 import java.io.Serializable
-import java.util.concurrent.atomic.AtomicBoolean
 
 const val TAG = "Q42Stats"
 
-private val MUTEX = Unit
-
 class Q42Stats(private val config: Q42StatsConfig) {
-    private val isRunning = AtomicBoolean(false)
 
     /* Collects stats and sends it to the server. This method is safe to be called from anywhere
-    in your app and will do nothing if it has already run before */
-    fun runAsync(context: Context) {
+    in your app and will do nothing if it is running or has already run before */
+    @AnyThread
+    fun runAsync(context: Context, coroutineScope: CoroutineScope = MainScope()) {
         Q42StatsLogger.d(TAG, "Q42Stats: Checking Preconditions")
-        if (isRunning.get()) {
-            Q42StatsLogger.i(
-                TAG,
-                "Q42Stats is already running. Exit."
-            )
-            return
+        // check preconditions on the main thread to prevent concurrency issues
+        coroutineScope.launch(Dispatchers.Main) {
+            if (job?.isActive != true) { // job is null or not active
+                // Do the actual work on a worker thread
+                job = coroutineScope.launch(Dispatchers.IO) { runSync(context) }
+            } else {
+                Q42StatsLogger.i(TAG, "Q42Stats is already running. Exit.")
+            }
         }
-        GlobalScope.launch(Dispatchers.IO) { synchronized(MUTEX) { runSync(context) } }
     }
 
-    /** This should be run on a worker thread */
+    @WorkerThread
     private fun runSync(context: Context) {
         try {
-            isRunning.set(true)
             val prefs = Q42StatsPrefs(context)
-            if (prefs.withinSubmitInterval(config.minimumSubmitInterval * 1000L) && !BuildConfig.DEBUG) {
+            if (prefs.withinSubmitInterval(config.minimumSubmitIntervalSeconds * 1000L)) {
                 Q42StatsLogger.i(
                     TAG,
-                    "Q42Stats were already sent in the last ${config.minimumSubmitInterval} seconds."
+                    "Q42Stats were already sent in the last ${config.minimumSubmitIntervalSeconds} seconds."
                 )
                 return
             }
             Q42StatsLogger.i(TAG, "Q42Stats: Start")
             val collected = collect(context, prefs).toFireStoreFormat()
             HttpService.sendStatsSync(config, collected)
             prefs.updateSubmitTimestamp()
-
         } catch (e: Throwable) {
             Q42StatsLogger.e(TAG, "Q42Stats encountered an error", e)
             if (BuildConfig.DEBUG) {
                 throw e
             }
         } finally {
             Q42StatsLogger.i(TAG, "Q42Stats: Exit")
-            isRunning.set(false)
         }
     }
 
@@ -71,4 +66,16 @@ class Q42Stats(private val config: Q42StatsConfig) {
         collected += SystemCollector.collect()
         return collected
     }
+
+    companion object {
+        @Suppress("unused")
+        var logLevel: Q42StatsLogLevel
+            get() = Q42StatsLogger.logLevel
+            set(value) {
+                Q42StatsLogger.logLevel = value
+            }
+
+        /** A static job ensures that only a single instance of Q42Stats can be running */
+        private var job: Job? = null
+    }
 }
diff --git a/library/src/main/java/com/q42/q42stats/library/Q42StatsConfig.kt b/library/src/main/java/com/q42/q42stats/library/Q42StatsConfig.kt
@@ -1,9 +1,9 @@
 package com.q42.q42stats.library
 
 data class Q42StatsConfig(
-    val fireBaseProject: String,
-    val firebaseCollection: String,
+    val firebaseProjectId: String,
+    val firebaseCollectionId: String,
     /** Data collection is skipped when less than this many seconds have passed
      * since the previous run */
-    val minimumSubmitInterval: Long
+    val minimumSubmitIntervalSeconds: Long
 )
diff --git a/library/src/main/java/com/q42/q42stats/library/Q42StatsLogger.kt b/library/src/main/java/com/q42/q42stats/library/Q42StatsLogger.kt
@@ -2,42 +2,42 @@ package com.q42.q42stats.library
 
 import android.util.Log
 
-object Q42StatsLogger {
+internal object Q42StatsLogger {
     /** logs with lower importance will be ignored */
-    private val logLevel = if (BuildConfig.DEBUG) LogLevel.Verbose else LogLevel.Info
+    var logLevel = if (BuildConfig.DEBUG) Q42StatsLogLevel.Verbose else Q42StatsLogLevel.Error
 
     fun v(tag: String, message: String) {
-        if (logLevel <= LogLevel.Verbose) {
+        if (logLevel <= Q42StatsLogLevel.Verbose) {
             Log.v(tag, message)
         }
     }
 
     fun d(tag: String, message: String) {
-        if (logLevel <= LogLevel.Debug) {
+        if (logLevel <= Q42StatsLogLevel.Debug) {
             Log.d(tag, message)
         }
     }
 
     fun i(tag: String, message: String) {
-        if (logLevel <= LogLevel.Info) {
+        if (logLevel <= Q42StatsLogLevel.Info) {
             Log.i(tag, message)
         }
     }
 
     fun w(tag: String, message: String) {
-        if (logLevel <= LogLevel.Warn) {
+        if (logLevel <= Q42StatsLogLevel.Warn) {
             Log.w(tag, message)
         }
     }
 
     fun e(tag: String, message: String, e: Throwable? = null) {
-        if (logLevel <= LogLevel.Error) {
-            Log.e(tag, message, e)
+        if (logLevel <= Q42StatsLogLevel.Error) {
+            Log.e(tag, "$message: ${e?.message}", e)
         }
     }
 }
 
-enum class LogLevel {
+enum class Q42StatsLogLevel {
     //Log levels in order of importance
     Verbose,
     Debug,

diff --git a/library/src/main/java/com/q42/q42stats/library/Q42StatsPrefs.kt b/library/src/main/java/com/q42/q42stats/library/Q42StatsPrefs.kt
@@ -8,7 +8,7 @@ private const val SHARED_PREFS_NAME = "Q42StatsPrefs"
 private const val LAST_SUBMIT_TIMESTAMP_KEY = "lastSubmitTimestamp"
 private const val INSTALLATION_ID_KEY = "installationId"
 
-class Q42StatsPrefs(context: Context) {
+internal class Q42StatsPrefs(context: Context) {
     private val prefs: SharedPreferences =
         context.getSharedPreferences(SHARED_PREFS_NAME, Context.MODE_PRIVATE)
 
@@ -21,20 +21,16 @@ class Q42StatsPrefs(context: Context) {
         System.currentTimeMillis() <
                 prefs.getLong(LAST_SUBMIT_TIMESTAMP_KEY, 0) + interval
 
-    fun updateSubmitTimestamp() {
-        prefs.edit().apply() {
-            putLong(LAST_SUBMIT_TIMESTAMP_KEY, System.currentTimeMillis())
-            apply()
-        }
+    fun updateSubmitTimestamp() = with(prefs.edit()) {
+        putLong(LAST_SUBMIT_TIMESTAMP_KEY, System.currentTimeMillis())
+        apply()
     }
 
-    private fun createInstallationId(): String {
-        prefs.edit().apply() {
-            val uuid = UUID.randomUUID().toString()
-            putString(INSTALLATION_ID_KEY, uuid)
-            apply()
-            return uuid
-        }
+    private fun createInstallationId(): String = with(prefs.edit()) {
+        val uuid = UUID.randomUUID().toString()
+        putString(INSTALLATION_ID_KEY, uuid)
+        apply()
+        return uuid
     }
 
 }
diff --git a/library/src/main/java/com/q42/q42stats/library/collector/AccessibilityCollector.kt b/library/src/main/java/com/q42/q42stats/library/collector/AccessibilityCollector.kt
@@ -14,7 +14,7 @@ import java.io.Serializable
 import java.util.*
 
 /** Collects Accessibility-related settings and preferences, such as font scaling */
-object AccessibilityCollector {
+internal object AccessibilityCollector {
 
     fun collect(context: Context) = mutableMapOf<String, Serializable>().apply {
         val accessibilityManager =