Commit 3b12522a authored by Michael Droettboom's avatar Michael Droettboom
Browse files

Address comments in the PR

parent 08523f9f

components/service/glean/src/main/java/mozilla/components/service/glean/private/CustomDistributionMetricType.kt

+24 −6
Original line number Diff line number Diff line
@@ -13,6 +13,17 @@ import mozilla.components.support.base.log.logger.Logger 
/**

 * This implements the developer facing API for recording custom distribution metrics.

 *

 * Custom distributions are histograms with the following parameters that are settable on a

 * per-metric basis:

 *

 *    - `rangeMin`/`rangeMax`: The minimum and maximum values

 *    - `bucketCount`: The number of histogram buckets

 *    - `histogramType`: Whether the bucketing is linear or exponential

 *

 * This metric exists primarily for backward compatibility with histograms in

 * legacy (pre-Glean) telemetry, and its use is not recommended for newly-created

 * metrics.

 *

 * Instances of this class type are automatically generated by the parsers at build time,

 * allowing developers to record values that were previously registered in the metrics.yaml file.

 */
@@ -22,16 +33,23 @@ data class CustomDistributionMetricType( 
    override val lifetime: Lifetime,

    override val name: String,

    override val sendInPings: List<String>,

    val range: List<Long>,

    val rangeMin: Long,

    val rangeMax: Long,

    val bucketCount: Int,

    val histogramType: HistogramType

) : CommonMetricData, HistogramBase {

    init {

        assert(range.size == 2)

    }



    private val logger = Logger("glean/CustomDistributionMetricType")



    /**

     * Accumulates the provided samples in the metric.

     *

     * The unit of the samples is entirely defined by the user. We encourage the author of the

     * metric to provide a `unit` parameter in the `metrics.yaml` file, but that has no effect

     * in the client and there is no unit conversion performed here.

     *

     * @param samples the [LongArray] holding the samples to be recorded by the metric.

     */

    override fun accumulateSamples(samples: LongArray) {

        if (!shouldRecord(logger)) {

            return
@@ -42,8 +60,8 @@ data class CustomDistributionMetricType( 
            CustomDistributionsStorageEngine.accumulateSamples(

                metricData = this@CustomDistributionMetricType,

                samples = samples,

                rangeMin = range[0],

                rangeMax = range[1],

                rangeMin = rangeMin,

                rangeMax = rangeMax,

                bucketCount = bucketCount,

                histogramType = histogramType

            )

components/service/glean/src/main/java/mozilla/components/service/glean/private/TimingDistributionMetricType.kt

+10 −8
Original line number Diff line number Diff line
@@ -94,11 +94,13 @@ data class TimingDistributionMetricType( 
    /**

     * Accumulates the provided samples in the metric.

     *

    * Please note that this assumes that the provided samples are in `timeUnit`

    * and will be converted to nanoseconds for storage and sending in the ping.

     * Please note that this assumes that the provided samples are already in the

     * "unit" declared by the instance of the implementing metric type (e.g. if the

     * implementing class is a [TimingDistributionMetricType] and the instance this

     * method was called on is using [TimeUnit.Second], then `samples` are assumed

     * to be in that unit).

     *

    * @param samples the [LongArray] holding the samples to be recorded by the metric, in

    *   the unit specified by `timeUnit`.

     * @param samples the [LongArray] holding the samples to be recorded by the metric.

     */

    override fun accumulateSamples(samples: LongArray) {

        if (!shouldRecord(logger)) {

components/service/glean/src/main/java/mozilla/components/service/glean/storages/CustomDistributionsStorageEngine.kt

+6 −13
Original line number Diff line number Diff line
@@ -4,7 +4,6 @@ 


package mozilla.components.service.glean.storages



import android.annotation.SuppressLint

import android.content.SharedPreferences

import androidx.annotation.VisibleForTesting

import mozilla.components.service.glean.error.ErrorRecording
@@ -19,14 +18,9 @@ import org.json.JSONArray 
import org.json.JSONObject



/**

 * This singleton handles the in-memory storage logic for timing distributions. It is meant to be

 * used by the Timing Distribution API and the ping assembling objects.

 *

 * This class contains a reference to the Android application Context. While the IDE warns

 * us that this could leak, the application context lives as long as the application and this

 * object. For this reason, we should be safe to suppress the IDE warning.

 * This singleton handles the in-memory storage logic for custom distributions. It is meant to be

 * used by the Custom Distribution API and the ping assembling objects.

 */

@SuppressLint("StaticFieldLeak")

internal object CustomDistributionsStorageEngine : CustomDistributionsStorageEngineImplementation()



internal open class CustomDistributionsStorageEngineImplementation(
@@ -145,7 +139,7 @@ internal open class CustomDistributionsStorageEngineImplementation( 
 * @param rangeMax the maximum value that can be represented

 * @param histogramType the [HistogramType] representing the bucket layout

 * @param values a map containing the bucket index mapped to the accumulated count

 * @param sum the accumulated sum of all the samples in the timing distribution

 * @param sum the accumulated sum of all the samples in the custom distribution

 */

@VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)

data class CustomDistributionData(
@@ -310,10 +304,9 @@ data class CustomDistributionData( 
    @Suppress("MagicNumber")

    private fun getBucketsLinear(): List<Long> {

        // Written to match the bucket generation on legacy desktop telemetry:

        //   https://searchfox.org/mozilla-central/source/toolkit/components/telemetry/build_scripts/mozparsers/parse_histograms.py#65

        //   https://searchfox.org/mozilla-central/rev/e0b0c38ee83f99d3cf868bad525ace4a395039f1/toolkit/components/telemetry/build_scripts/mozparsers/parse_histograms.py#65



        val result: MutableList<Long> = mutableListOf()

        result.add(0)

        val result: MutableList<Long> = mutableListOf(0L)



        val dmin = rangeMin.toDouble()

        val dmax = rangeMax.toDouble()
@@ -334,7 +327,7 @@ data class CustomDistributionData( 
     */

    private fun getBucketsExponential(): List<Long> {

        // Written to match the bucket generation on legacy desktop telemetry:

        //   https://searchfox.org/mozilla-central/source/toolkit/components/telemetry/build_scripts/mozparsers/parse_histograms.py#75

        //   https://searchfox.org/mozilla-central/rev/e0b0c38ee83f99d3cf868bad525ace4a395039f1/toolkit/components/telemetry/build_scripts/mozparsers/parse_histograms.py#75



        // This algorithm calculates the bucket sizes using a natural log approach to get

        // `bucketCount` number of buckets, exponentially spaced between `range[MIN]` and

components/service/glean/src/test/java/mozilla/components/service/glean/private/CustomDistributionMetricTypeTest.kt

+10 −5
Original line number Diff line number Diff line
@@ -34,7 +34,8 @@ class CustomDistributionMetricTypeTest { 
            lifetime = Lifetime.Ping,

            name = "custom_distribution",

            sendInPings = listOf("store1"),

            range = listOf(0L, 60000L),

            rangeMin = 0L,

            rangeMax = 60000L,

            bucketCount = 100,

            histogramType = HistogramType.Exponential

        )
@@ -67,7 +68,8 @@ class CustomDistributionMetricTypeTest { 
            lifetime = Lifetime.Ping,

            name = "custom_distribution",

            sendInPings = listOf("store1"),

            range = listOf(0L, 60000L),

            rangeMin = 0L,

            rangeMax = 60000L,

            bucketCount = 100,

            histogramType = HistogramType.Exponential

        )
@@ -89,7 +91,8 @@ class CustomDistributionMetricTypeTest { 
            lifetime = Lifetime.Ping,

            name = "custom_distribution",

            sendInPings = listOf("store1"),

            range = listOf(0L, 60000L),

            rangeMin = 0L,

            rangeMax = 60000L,

            bucketCount = 100,

            histogramType = HistogramType.Exponential

        )
@@ -105,7 +108,8 @@ class CustomDistributionMetricTypeTest { 
            lifetime = Lifetime.Ping,

            name = "custom_distribution",

            sendInPings = listOf("store1", "store2", "store3"),

            range = listOf(0L, 60000L),

            rangeMin = 0L,

            rangeMax = 60000L,

            bucketCount = 100,

            histogramType = HistogramType.Exponential

        )
@@ -147,7 +151,8 @@ class CustomDistributionMetricTypeTest { 
            lifetime = Lifetime.Ping,

            name = "custom_distribution_samples",

            sendInPings = listOf("store1"),

            range = listOf(0L, 60000L),

            rangeMin = 0L,

            rangeMax = 60000L,

            bucketCount = 100,

            histogramType = HistogramType.Exponential

        )

components/service/glean/src/test/java/mozilla/components/service/glean/storages/CustomDistributionsStorageEngineTest.kt

+68 −21
Original line number Diff line number Diff line
@@ -42,7 +42,8 @@ class CustomDistributionsStorageEngineTest { 
            lifetime = Lifetime.Ping,

            name = "test_custom_distribution",

            sendInPings = storeNames,

            range = listOf(0L, 60000L),

            rangeMin = 0L,

            rangeMax = 60000L,

            bucketCount = 100,

            histogramType = HistogramType.Exponential

        )
@@ -80,7 +81,8 @@ class CustomDistributionsStorageEngineTest { 
            lifetime = Lifetime.Ping,

            name = "test_custom_distribution",

            sendInPings = listOf("store1", "store2"),

            range = listOf(0L, 60000L),

            rangeMin = 0L,

            rangeMax = 60000L,

            bucketCount = 100,

            histogramType = HistogramType.Exponential

        )
@@ -100,14 +102,13 @@ class CustomDistributionsStorageEngineTest { 
            "store1#telemetry.invalid_int" to -1,

            "store1#telemetry.invalid_list" to listOf("1", "2", "3"),

            "store1#telemetry.invalid_int_list" to "[1,2,3]",

            "store1#telemetry.invalid_td_name" to "{\"category\":\"telemetry\",\"bucket_count\":100,\"range\":[0,60000,12],\"histogram_type\":1,\"values\":{},\"sum\":0,\"time_unit\":2}",

            "store1#telemetry.invalid_td_bucket_count" to "{\"category\":\"telemetry\",\"name\":\"test_custom_distribution\",\"bucket_count\":\"not an int!\",\"range\":[0,60000,12],\"histogram_type\":1,\"values\":{},\"sum\":0,\"time_unit\":2}",

            "store1#telemetry.invalid_td_range" to "{\"category\":\"telemetry\",\"name\":\"test_custom_distribution\",\"bucket_count\":100,\"range\":[0,60000,12],\"histogram_type\":1,\"values\":{},\"sum\":0,\"time_unit\":2}",

            "store1#telemetry.invalid_td_range2" to "{\"category\":\"telemetry\",\"name\":\"test_custom_distribution\",\"bucket_count\":100,\"range\":[\"not\",\"numeric\"],\"histogram_type\":1,\"values\":{},\"sum\":0,\"time_unit\":2}",

            "store1#telemetry.invalid_td_histogram_type" to "{\"category\":\"telemetry\",\"name\":\"test_custom_distribution\",\"bucket_count\":100,\"range\":[0,60000,12],\"histogram_type\":-1,\"values\":{},\"sum\":0,\"time_unit\":2}",

            "store1#telemetry.invalid_td_values" to "{\"category\":\"telemetry\",\"name\":\"test_custom_distribution\",\"bucket_count\":100,\"range\":[0,60000,12],\"histogram_type\":1,\"values\":{\"0\": \"nope\"},\"sum\":0,\"time_unit\":2}",

            "store1#telemetry.invalid_td_sum" to "{\"category\":\"telemetry\",\"name\":\"test_custom_distribution\",\"bucket_count\":100,\"range\":[0,60000,12],\"histogram_type\":1,\"values\":{},\"sum\":\"nope\",\"time_unit\":2}",

            "store1#telemetry.invalid_td_time_unit" to "{\"category\":\"telemetry\",\"name\":\"test_custom_distribution\",\"bucket_count\":100,\"range\":[0,60000,12],\"histogram_type\":1,\"values\":{},\"sum\":0,\"time_unit\":-1}",

            "store1#telemetry.invalid_td_name" to "{\"category\":\"telemetry\",\"bucket_count\":100,\"range\":[0,60000,12],\"histogram_type\":1,\"values\":{},\"sum\":0}",

            "store1#telemetry.invalid_td_bucket_count" to "{\"category\":\"telemetry\",\"name\":\"test_custom_distribution\",\"bucket_count\":\"not an int!\",\"range\":[0,60000,12],\"histogram_type\":1,\"values\":{},\"sum\":0}",

            "store1#telemetry.invalid_td_range" to "{\"category\":\"telemetry\",\"name\":\"test_custom_distribution\",\"bucket_count\":100,\"range\":[0,60000,12],\"histogram_type\":1,\"values\":{},\"sum\":0}",

            "store1#telemetry.invalid_td_range2" to "{\"category\":\"telemetry\",\"name\":\"test_custom_distribution\",\"bucket_count\":100,\"range\":[\"not\",\"numeric\"],\"histogram_type\":1,\"values\":{},\"sum\":0}",

            "store1#telemetry.invalid_td_histogram_type" to "{\"category\":\"telemetry\",\"name\":\"test_custom_distribution\",\"bucket_count\":100,\"range\":[0,60000,12],\"histogram_type\":-1,\"values\":{},\"sum\":0}",

            "store1#telemetry.invalid_td_values" to "{\"category\":\"telemetry\",\"name\":\"test_custom_distribution\",\"bucket_count\":100,\"range\":[0,60000,12],\"histogram_type\":1,\"values\":{\"0\": \"nope\"},\"sum\":0}",

            "store1#telemetry.invalid_td_sum" to "{\"category\":\"telemetry\",\"name\":\"test_custom_distribution\",\"bucket_count\":100,\"range\":[0,60000,12],\"histogram_type\":1,\"values\":{},\"sum\":\"nope\"}",

            "store1#telemetry.test_custom_distribution" to td.toJsonObject().toString()

        )
@@ -145,7 +146,8 @@ class CustomDistributionsStorageEngineTest { 
            lifetime = Lifetime.User,

            name = "test_custom_distribution",

            sendInPings = listOf("store1"),

            range = listOf(0L, 60000L),

            rangeMin = 0L,

            rangeMax = 60000L,

            bucketCount = 100,

            histogramType = HistogramType.Exponential

        )
@@ -167,7 +169,8 @@ class CustomDistributionsStorageEngineTest { 
                lifetime = Lifetime.User,

                name = "test_custom_distribution",

                sendInPings = listOf("store1", "store2"),

                range = listOf(0L, 60000L),

                rangeMin = 0L,

                rangeMax = 60000L,

                bucketCount = 100,

                histogramType = HistogramType.Exponential

            )
@@ -237,7 +240,8 @@ class CustomDistributionsStorageEngineTest { 
            lifetime = Lifetime.User,

            name = "test_custom_distribution",

            sendInPings = listOf("store1"),

            range = listOf(0L, 60000L),

            rangeMin = 0L,

            rangeMax = 60000L,

            bucketCount = 100,

            histogramType = HistogramType.Exponential

        )
@@ -263,7 +267,8 @@ class CustomDistributionsStorageEngineTest { 
            lifetime = Lifetime.User,

            name = "test_custom_distribution",

            sendInPings = listOf("store1"),

            range = listOf(0L, 60000L),

            rangeMin = 0L,

            rangeMax = 60000L,

            bucketCount = 100,

            histogramType = HistogramType.Exponential

        )
@@ -293,7 +298,8 @@ class CustomDistributionsStorageEngineTest { 
            lifetime = Lifetime.User,

            name = "test_custom_distribution",

            sendInPings = listOf("store1"),

            range = listOf(0L, rangeMax),

            rangeMin = 0L,

            rangeMax = 60000L,

            bucketCount = 100,

            histogramType = HistogramType.Exponential

        )
@@ -313,7 +319,7 @@ class CustomDistributionsStorageEngineTest { 
    }



    @Test

    fun `getBuckets() correctly populates the buckets property`() {

    fun `getBuckets() correctly populates the buckets properly for exponential distributions`() {

        // Hand calculated values using current default range 0 - 60000 and bucket count of 100.

        // NOTE: The final bucket, regardless of width, represents the overflow bucket to hold any

        // values beyond the maximum (in this case the maximum is 60000)
@@ -332,7 +338,8 @@ class CustomDistributionsStorageEngineTest { 
            lifetime = Lifetime.User,

            name = "test_custom_distribution",

            sendInPings = listOf("store1"),

            range = listOf(0L, 60000L),

            rangeMin = 0L,

            rangeMax = 60000L,

            bucketCount = 100,

            histogramType = HistogramType.Exponential

        )
@@ -355,7 +362,7 @@ class CustomDistributionsStorageEngineTest { 
    }



    @Test

    fun `samples go in the correct bucket`() {

    fun `samples go in the correct bucket for exponential bucketing`() {

        val td = CustomDistributionData(

            category = "telemetry",

            name = "test_custom_distribution",
@@ -388,7 +395,7 @@ class CustomDistributionsStorageEngineTest { 
    }



    @Test

    fun `linear bucketing`() {

    fun `validate the generated linear buckets`() {

        val td = CustomDistributionData(

            category = "telemetry",

            name = "test_custom_distribution",
@@ -422,7 +429,7 @@ class CustomDistributionsStorageEngineTest { 
    }



    @Test

    fun `accumulate finds the correct bucket`() {

    fun `accumulate finds the correct bucket for exponential distributions`() {

        // Define a custom distribution metric, which will be stored in "store1".

        val metric = CustomDistributionMetricType(

            disabled = false,
@@ -430,7 +437,8 @@ class CustomDistributionsStorageEngineTest { 
            lifetime = Lifetime.User,

            name = "test_custom_distribution",

            sendInPings = listOf("store1"),

            range = listOf(0L, 60000L),

            rangeMin = 0L,

            rangeMax = 60000L,

            bucketCount = 100,

            histogramType = HistogramType.Exponential

        )
@@ -463,6 +471,45 @@ class CustomDistributionsStorageEngineTest { 
            1L, snapshot.values[9262])

    }



    @Test

    fun `accumulate finds the correct bucket for linear distributions`() {

        // Define a custom distribution metric, which will be stored in "store1".

        val metric = CustomDistributionMetricType(

            disabled = false,

            category = "telemetry",

            lifetime = Lifetime.User,

            name = "test_custom_distribution",

            sendInPings = listOf("store1"),

            rangeMin = 0L,

            rangeMax = 60000L,

            bucketCount = 100,

            histogramType = HistogramType.Linear

        )



        // Check that a few values correctly fall into the correct buckets (as calculated by hand)

        // to validate the linear bucket search algorithm



        // Attempt to accumulate a sample to force metric to be stored

        metric.accumulateSamples(listOf(1L, 10L, 100L, 1000L, 10000L).toLongArray())



        // Check that custom distribution was recorded.

        assertTrue("Accumulating values records data", metric.testHasValue())



        // Make sure that the samples are in the correct buckets

        val snapshot = metric.testGetValue()



        // Check sum and count

        assertEquals("Accumulating updates the sum", 11111, snapshot.sum)

        assertEquals("Accumulating updates the count", 5, snapshot.count)



        assertEquals("Accumulating should increment correct bucket",

            3L, snapshot.values[0])

        assertEquals("Accumulating should increment correct bucket",

            1L, snapshot.values[612])

        assertEquals("Accumulating should increment correct bucket",

            1L, snapshot.values[9796])

    }



    @Test

    fun `toJsonObject correctly converts a CustomDistributionData object`() {

        // Define a CustomDistributionData object