From f404a2e653edad93db50dca096e00c821826b587 Mon Sep 17 00:00:00 2001 From: Naomi Kirby Date: Wed, 8 Jan 2025 10:44:21 -0800 Subject: [PATCH 01/10] Add test to check timeToMainScreen telemetry. There is some concern that we might have a regression in app loading times so lets try adding a test to ensure it remains within a reasonable bound. --- tests/functional/testNavigation.js | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/tests/functional/testNavigation.js b/tests/functional/testNavigation.js index cf9945277b..e9da663da2 100644 --- a/tests/functional/testNavigation.js +++ b/tests/functional/testNavigation.js @@ -125,6 +125,13 @@ describe('Navigation bar', async function() { await vpn.waitForQueryAndClick(queries.navBar.SETTINGS.visible()); assert.equal(await navigationBarVisible(), 'true'); }); + + it("Time to main screen is recorded", async () => { + // Check that the time to the main screen was greater than 1ms but less than a full second. + let timing = await vpn.gleanTestGetValue("performance", "timeToMainScreen", ""); + assert(timing.sum > 1000000); + assert(timing.sum < 1000000000); + }) }); /* TODO:... From dff03443feddc99a356250d042a75af4d47367c9 Mon Sep 17 00:00:00 2001 From: Naomi Kirby Date: Mon, 6 Jan 2025 16:25:40 -0800 Subject: [PATCH 02/10] Rewrite glean/inspector to use C++ instead of QML/JS It turns out that the distribution metrics can't be serialized by the QML/JS engine, so we'll need to clean up that hack and implement metric serialization in C++ instead. --- src/inspector/inspectorhandler.cpp | 111 +++++++++++++++++------------ 1 file changed, 65 insertions(+), 46 deletions(-) diff --git a/src/inspector/inspectorhandler.cpp b/src/inspector/inspectorhandler.cpp index cff2769ee6..0ad72b1920 100644 --- a/src/inspector/inspectorhandler.cpp +++ b/src/inspector/inspectorhandler.cpp @@ -68,33 +68,58 @@ InspectorItemPicker* s_itemPicker = nullptr; std::function s_constructorCallback; InspectorHotreloader* s_hotreloader = nullptr; -QJsonValue qjsValueToQJsonValue(const QJSValue& qjsValue) { - if (qjsValue.isUndefined() || qjsValue.isNull()) { - return QJsonValue(QJsonValue::Undefined); - } else if (qjsValue.isBool()) { - return QJsonValue(qjsValue.toBool()); - } else if (qjsValue.isNumber()) { - return QJsonValue(qjsValue.toNumber()); - } else if (qjsValue.isString()) { - return QJsonValue(qjsValue.toString()); - } else if (qjsValue.isArray()) { - QJsonArray jsonArray; - quint32 length = qjsValue.property("length").toUInt(); - for (quint32 i = 0; i < length; ++i) { - jsonArray.append(qjsValueToQJsonValue(qjsValue.property(i))); - } - return QJsonValue(jsonArray); - } else if (qjsValue.isObject()) { - QJsonObject jsonObject; - QJSValueIterator it(qjsValue); - while (it.hasNext()) { - it.next(); - jsonObject.insert(it.name(), qjsValueToQJsonValue(it.value())); +QJsonObject gleanMetricToResult(const QVariant& metric, const QString& pingName) { + QJsonObject obj; + + // Encode the results by metric type. + if (metric.canConvert()) { + EventMetric* event = metric.value(); + QJsonArray results; + for (const QJsonObject& value : event->testGetValue(pingName)) { + results.append(value); } - return QJsonValue(jsonObject); + obj["value"] = results; + return obj; + } + if (metric.canConvert()) { + BooleanMetric* bval = metric.value(); + obj["value"] = QJsonValue(bval->testGetValue(pingName)); + return obj; + } + if (metric.canConvert()) { + CounterMetric* counter = metric.value(); + obj["value"] = QJsonValue(counter->testGetValue(pingName)); + return obj; + } + if (metric.canConvert()) { + DatetimeMetric* dt = metric.value(); + obj["value"] = QJsonValue(dt->testGetValueAsString(pingName)); + return obj; + } + if (metric.canConvert()) { + QuantityMetric* quantity = metric.value(); + obj["value"] = QJsonValue(quantity->testGetValue(pingName)); + return obj; + } + if (metric.canConvert()) { + StringMetric* str = metric.value(); + obj["value"] = QJsonValue(str->testGetValue(pingName)); + return obj; + } + if (metric.canConvert()) { + UuidMetric* uuid = metric.value(); + obj["value"] = QJsonValue(uuid->testGetValue(pingName)); + return obj; } - // Handle other types as needed - return QJsonValue(QJsonValue::Undefined); + // TODO: Distribution metrics. + + // Otherwise, we don't support this metric type. + QString className = "unknown"; + if (metric.canConvert()) { + className = metric.value()->metaObject()->className(); + } + obj["error"] = QString("not a supported metric type (%1)").arg(className); + return obj; } } // namespace @@ -699,32 +724,26 @@ static QList s_commands{ InspectorCommand{ "glean_test_get_value", "Get value of a Glean metric", 3, [](InspectorHandler*, const QList& arguments) { - QString metricCategory = QString(arguments[1]); - QString metricName = QString(arguments[2]); QString ping = QString(arguments[3]); - // Hack: let's run the code on the QML side, - // because I (Bea) could not figure out a nice way - // to access C++ namespaces and namespace properties - // with runtime strings. - QJSEngine* engine = QmlEngineHolder::instance()->engine(); - QJSValue function = QmlEngineHolder::instance()->engine()->evaluate( - QString("(Glean) => Glean.%1.%2.testGetValue(\"%3\")") - .arg(metricCategory) - .arg(metricName) - .arg(ping)); - - QJSValue result = function.call(QJSValueList{ - engine->toScriptValue(__DONOTUSE__GleanMetrics::instance())}); + QObject* instance = __DONOTUSE__GleanMetrics::instance(); + QVariant qvCategory = instance->property(arguments[1].constData()); + QObject* category = qvCategory.value(); + if (category == nullptr) { + QJsonObject obj; + obj["error"] = QString(arguments[1]) + "is not a valid category"; + return obj; + } - QJsonObject obj; - if (result.isError()) { - obj["error"] = result.toString(); - } else { - obj["value"] = qjsValueToQJsonValue(result); + QVariant metric = category->property(arguments[2].constData()); + if (!metric.isValid()) { + QJsonObject obj; + obj["error"] = QString(arguments[2]) + "is not a valid metric"; + return obj; } + + return gleanMetricToResult(metric, ping); - return obj; }}, InspectorCommand{"glean_test_reset", "Resets Glean for testing", 0, From 04cf82e268fb0c66c00707c264a54ed6124b6393 Mon Sep 17 00:00:00 2001 From: Naomi Kirby Date: Wed, 8 Jan 2025 10:53:24 -0800 Subject: [PATCH 03/10] Create a BaseMetric type to simplify metric serialization in testing --- qtglean/CMakeLists.txt | 1 + qtglean/include/glean/basemetric.h | 26 ++++++++ qtglean/include/glean/boolean.h | 10 ++- qtglean/include/glean/counter.h | 11 ++-- qtglean/include/glean/customdistribution.h | 12 ++-- qtglean/include/glean/datetime.h | 12 ++-- qtglean/include/glean/event.h | 11 ++-- qtglean/include/glean/memorydistribution.h | 12 ++-- qtglean/include/glean/metrictypes.h | 1 + qtglean/include/glean/quantity.h | 11 ++-- qtglean/include/glean/string.h | 11 ++-- qtglean/include/glean/timingdistribution.h | 12 ++-- qtglean/include/glean/uuid.h | 10 ++- qtglean/src/cpp/boolean.cpp | 9 +-- qtglean/src/cpp/counter.cpp | 9 +-- qtglean/src/cpp/customdistribution.cpp | 19 ++---- qtglean/src/cpp/datetime.cpp | 15 ++--- qtglean/src/cpp/event.cpp | 19 ++---- qtglean/src/cpp/memorydistribution.cpp | 19 ++---- qtglean/src/cpp/quantity.cpp | 9 +-- qtglean/src/cpp/string.cpp | 9 +-- qtglean/src/cpp/timingdistribution.cpp | 19 ++---- qtglean/src/cpp/uuid.cpp | 9 +-- src/inspector/inspectorhandler.cpp | 72 +++++----------------- 24 files changed, 132 insertions(+), 216 deletions(-) create mode 100644 qtglean/include/glean/basemetric.h diff --git a/qtglean/CMakeLists.txt b/qtglean/CMakeLists.txt index 99e6d8055a..b100d238a4 100644 --- a/qtglean/CMakeLists.txt +++ b/qtglean/CMakeLists.txt @@ -12,6 +12,7 @@ execute_process( ## Add a static library for the Glean C++ code. add_library(qtglean STATIC + ${CMAKE_CURRENT_SOURCE_DIR}/include/glean/basemetric.h ${CMAKE_CURRENT_SOURCE_DIR}/include/glean/boolean.h ${CMAKE_CURRENT_SOURCE_DIR}/include/glean/datetime.h ${CMAKE_CURRENT_SOURCE_DIR}/include/glean/quantity.h diff --git a/qtglean/include/glean/basemetric.h b/qtglean/include/glean/basemetric.h new file mode 100644 index 0000000000..7605aadf3d --- /dev/null +++ b/qtglean/include/glean/basemetric.h @@ -0,0 +1,26 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +#ifndef BASEMETRIC_H +#define BASEMETRIC_H +#include + +#include "errortype.h" + +class BaseMetric : public QObject { + Q_OBJECT + Q_DISABLE_COPY_MOVE(BaseMetric) + + public: + explicit BaseMetric(int aId) : m_id(aId) {}; + + // Test only functions + virtual QJsonValue testGetValue(const QString& pingName = "") const = 0; + virtual int32_t testGetNumRecordedErrors(ErrorType errorType) const = 0; + + protected: + const int m_id; +}; + +#endif // BASEMETRIC_H diff --git a/qtglean/include/glean/boolean.h b/qtglean/include/glean/boolean.h index 2152f3a058..efda9bf1b7 100644 --- a/qtglean/include/glean/boolean.h +++ b/qtglean/include/glean/boolean.h @@ -6,9 +6,10 @@ #define BOOLEAN_H #include +#include "basemetric.h" #include "errortype.h" -class BooleanMetric final : public QObject { +class BooleanMetric final : public BaseMetric { Q_OBJECT Q_DISABLE_COPY_MOVE(BooleanMetric) @@ -18,11 +19,8 @@ class BooleanMetric final : public QObject { Q_INVOKABLE void set(bool value = true) const; // Test only functions - Q_INVOKABLE bool testGetValue(const QString& pingName = "") const; - Q_INVOKABLE int32_t testGetNumRecordedErrors(ErrorType errorType) const; - - private: - const int m_id; + virtual QJsonValue testGetValue(const QString& pingName = "") const; + virtual int32_t testGetNumRecordedErrors(ErrorType errorType) const; }; #endif // BOOLEAN_H diff --git a/qtglean/include/glean/counter.h b/qtglean/include/glean/counter.h index e0901ed04f..8b9ad8b630 100644 --- a/qtglean/include/glean/counter.h +++ b/qtglean/include/glean/counter.h @@ -6,9 +6,10 @@ #define COUNTER_H #include +#include "basemetric.h" #include "errortype.h" -class CounterMetric final : public QObject { +class CounterMetric final : public BaseMetric { Q_OBJECT Q_DISABLE_COPY_MOVE(CounterMetric) @@ -18,12 +19,8 @@ class CounterMetric final : public QObject { Q_INVOKABLE void add(int amount = 1) const; // Test only functions - - Q_INVOKABLE int32_t testGetValue(const QString& pingName = "") const; - Q_INVOKABLE int32_t testGetNumRecordedErrors(ErrorType errorType) const; - - private: - const int m_id; + virtual QJsonValue testGetValue(const QString& pingName = "") const; + virtual int32_t testGetNumRecordedErrors(ErrorType errorType) const; }; #endif // COUNTER_H diff --git a/qtglean/include/glean/customdistribution.h b/qtglean/include/glean/customdistribution.h index bdc5efaf28..1ebcf1c94a 100644 --- a/qtglean/include/glean/customdistribution.h +++ b/qtglean/include/glean/customdistribution.h @@ -8,10 +8,10 @@ #include #include -#include "distributiondata.h" +#include "basemetric.h" #include "errortype.h" -class CustomDistributionMetric final : public QObject { +class CustomDistributionMetric final : public BaseMetric { Q_OBJECT Q_DISABLE_COPY_MOVE(CustomDistributionMetric) @@ -21,12 +21,8 @@ class CustomDistributionMetric final : public QObject { Q_INVOKABLE void accumulate_single_sample(qint64 sample) const; // Test only functions - - Q_INVOKABLE DistributionData testGetValue(const QString& pingName = "") const; - Q_INVOKABLE int32_t testGetNumRecordedErrors(ErrorType errorType) const; - - private: - const int m_id; + virtual QJsonValue testGetValue(const QString& pingName = "") const; + virtual int32_t testGetNumRecordedErrors(ErrorType errorType) const; }; #endif // CUSTOM_DISTRIBUTION_H diff --git a/qtglean/include/glean/datetime.h b/qtglean/include/glean/datetime.h index 360a81b77c..f3edabb94b 100644 --- a/qtglean/include/glean/datetime.h +++ b/qtglean/include/glean/datetime.h @@ -9,13 +9,14 @@ #include #include +#include "basemetric.h" #include "errortype.h" // NOTE: While most of datetime is implemented, one piece is not yet: // - The ability to set an arbitrary datestamp // More details: https://mozilla-hub.atlassian.net/browse/VPN-4173 -class DatetimeMetric final : public QObject { +class DatetimeMetric final : public BaseMetric { Q_OBJECT Q_DISABLE_COPY_MOVE(DatetimeMetric) @@ -25,13 +26,8 @@ class DatetimeMetric final : public QObject { Q_INVOKABLE void set() const; // Test only functions - - Q_INVOKABLE QDateTime testGetValue(const QString& pingName = "") const; - Q_INVOKABLE QString testGetValueAsString(const QString& pingName = "") const; - Q_INVOKABLE int32_t testGetNumRecordedErrors(ErrorType errorType) const; - - private: - const int m_id; + virtual QJsonValue testGetValue(const QString& pingName = "") const; + virtual int32_t testGetNumRecordedErrors(ErrorType errorType) const; }; #endif // DATETIME_H diff --git a/qtglean/include/glean/event.h b/qtglean/include/glean/event.h index 3ba2fa6ce8..3c858ea74c 100644 --- a/qtglean/include/glean/event.h +++ b/qtglean/include/glean/event.h @@ -9,6 +9,7 @@ #include #include +#include "basemetric.h" #include "errortype.h" struct FfiExtra { @@ -54,7 +55,7 @@ struct EventMetricExtraParser { } }; -class EventMetric final : public QObject { +class EventMetric final : public BaseMetric { Q_OBJECT Q_DISABLE_COPY_MOVE(EventMetric) @@ -70,13 +71,11 @@ class EventMetric final : public QObject { void record(const EventMetricExtra& extras); - Q_INVOKABLE int32_t testGetNumRecordedErrors(ErrorType errorType) const; - - Q_INVOKABLE QList testGetValue( - const QString& pingName = "") const; + // Test only functions + virtual QJsonValue testGetValue(const QString& pingName = "") const; + virtual int32_t testGetNumRecordedErrors(ErrorType errorType) const; private: - const int m_id; EventMetricExtraParser* m_parser; }; diff --git a/qtglean/include/glean/memorydistribution.h b/qtglean/include/glean/memorydistribution.h index d6c77c6ed3..b50651ee0f 100644 --- a/qtglean/include/glean/memorydistribution.h +++ b/qtglean/include/glean/memorydistribution.h @@ -8,7 +8,7 @@ #include #include -#include "distributiondata.h" +#include "basemetric.h" #include "errortype.h" // !!!IMPORTANT!!! @@ -20,7 +20,7 @@ // stayed in so that this work is not lost, in the event we want to use // this data type in the future. -class MemoryDistributionMetric final : public QObject { +class MemoryDistributionMetric final : public BaseMetric { Q_OBJECT Q_DISABLE_COPY_MOVE(MemoryDistributionMetric) @@ -30,12 +30,8 @@ class MemoryDistributionMetric final : public QObject { Q_INVOKABLE void accumulate(qint64 sample) const; // Test only functions - - Q_INVOKABLE DistributionData testGetValue(const QString& pingName = "") const; - Q_INVOKABLE int32_t testGetNumRecordedErrors(ErrorType errorType) const; - - private: - const int m_id; + virtual QJsonValue testGetValue(const QString& pingName = "") const; + virtual int32_t testGetNumRecordedErrors(ErrorType errorType) const; }; #endif // MEMORY_DISTRIBUTION_H diff --git a/qtglean/include/glean/metrictypes.h b/qtglean/include/glean/metrictypes.h index fc48f9d0d4..18fc19782e 100644 --- a/qtglean/include/glean/metrictypes.h +++ b/qtglean/include/glean/metrictypes.h @@ -5,6 +5,7 @@ #ifndef METRICTYPES_H #define METRICTYPES_H +#include "basemetric.h" #include "boolean.h" #include "counter.h" #include "customdistribution.h" diff --git a/qtglean/include/glean/quantity.h b/qtglean/include/glean/quantity.h index b28e4e366b..78ef628a4a 100644 --- a/qtglean/include/glean/quantity.h +++ b/qtglean/include/glean/quantity.h @@ -7,9 +7,10 @@ #include +#include "basemetric.h" #include "errortype.h" -class QuantityMetric final : public QObject { +class QuantityMetric final : public BaseMetric { Q_OBJECT Q_DISABLE_COPY_MOVE(QuantityMetric) @@ -19,12 +20,8 @@ class QuantityMetric final : public QObject { Q_INVOKABLE void set(int value = 0) const; // Test only functions - - Q_INVOKABLE int64_t testGetValue(const QString& pingName = "") const; - Q_INVOKABLE int32_t testGetNumRecordedErrors(ErrorType errorType) const; - - private: - const int m_id; + virtual QJsonValue testGetValue(const QString& pingName = "") const; + virtual int32_t testGetNumRecordedErrors(ErrorType errorType) const; }; #endif // QUANTITY_H diff --git a/qtglean/include/glean/string.h b/qtglean/include/glean/string.h index baa4332409..f1d65aac57 100644 --- a/qtglean/include/glean/string.h +++ b/qtglean/include/glean/string.h @@ -7,9 +7,10 @@ #include #include +#include "basemetric.h" #include "errortype.h" -class StringMetric final : public QObject { +class StringMetric final : public BaseMetric { Q_OBJECT Q_DISABLE_COPY_MOVE(StringMetric) @@ -19,12 +20,8 @@ class StringMetric final : public QObject { Q_INVOKABLE void set(QString value = "") const; // Test only functions - - Q_INVOKABLE QString testGetValue(const QString& pingName = "") const; - Q_INVOKABLE int32_t testGetNumRecordedErrors(ErrorType errorType) const; - - private: - const int m_id; + virtual QJsonValue testGetValue(const QString& pingName = "") const; + virtual int32_t testGetNumRecordedErrors(ErrorType errorType) const; }; #endif // STRING_H diff --git a/qtglean/include/glean/timingdistribution.h b/qtglean/include/glean/timingdistribution.h index 04f04db6d3..ade57f553f 100644 --- a/qtglean/include/glean/timingdistribution.h +++ b/qtglean/include/glean/timingdistribution.h @@ -8,10 +8,10 @@ #include #include -#include "distributiondata.h" +#include "basemetric.h" #include "errortype.h" -class TimingDistributionMetric final : public QObject { +class TimingDistributionMetric final : public BaseMetric { Q_OBJECT Q_DISABLE_COPY_MOVE(TimingDistributionMetric) @@ -23,12 +23,8 @@ class TimingDistributionMetric final : public QObject { Q_INVOKABLE void cancel(qint64 timerId) const; // Test only functions - - Q_INVOKABLE DistributionData testGetValue(const QString& pingName = "") const; - Q_INVOKABLE int32_t testGetNumRecordedErrors(ErrorType errorType) const; - - private: - const int m_id; + virtual QJsonValue testGetValue(const QString& pingName = "") const; + virtual int32_t testGetNumRecordedErrors(ErrorType errorType) const; }; #endif // TIMING_DISTRIBUTION_H diff --git a/qtglean/include/glean/uuid.h b/qtglean/include/glean/uuid.h index ea7fdbc16b..3e020a704e 100644 --- a/qtglean/include/glean/uuid.h +++ b/qtglean/include/glean/uuid.h @@ -6,9 +6,10 @@ #define UUID_H #include +#include "basemetric.h" #include "errortype.h" -class UuidMetric final : public QObject { +class UuidMetric final : public BaseMetric { Q_OBJECT Q_DISABLE_COPY_MOVE(UuidMetric) @@ -20,11 +21,8 @@ class UuidMetric final : public QObject { Q_INVOKABLE QString generateAndSet() const; // Test only functions - Q_INVOKABLE QString testGetValue(const QString& pingName = "") const; - Q_INVOKABLE int32_t testGetNumRecordedErrors(ErrorType errorType) const; - - private: - const int m_id; + virtual QJsonValue testGetValue(const QString& pingName = "") const; + virtual int32_t testGetNumRecordedErrors(ErrorType errorType) const; }; #endif // UUID_H diff --git a/qtglean/src/cpp/boolean.cpp b/qtglean/src/cpp/boolean.cpp index 38f8e38d08..a2e0a4f514 100644 --- a/qtglean/src/cpp/boolean.cpp +++ b/qtglean/src/cpp/boolean.cpp @@ -5,12 +5,13 @@ #include "glean/boolean.h" #include +#include #ifndef __wasm__ # include "qtglean.h" #endif -BooleanMetric::BooleanMetric(int id) : m_id(id) {} +BooleanMetric::BooleanMetric(int id) : BaseMetric(id) {} void BooleanMetric::set(bool value) const { #ifndef __wasm__ @@ -25,9 +26,9 @@ int32_t BooleanMetric::testGetNumRecordedErrors(ErrorType errorType) const { return 0; } -bool BooleanMetric::testGetValue(const QString& pingName) const { +QJsonValue BooleanMetric::testGetValue(const QString& pingName) const { #ifndef __wasm__ - return glean_boolean_test_get_value(m_id, pingName.toUtf8()); + return QJsonValue(glean_boolean_test_get_value(m_id, pingName.toUtf8())); #endif - return false; + return QJsonValue(false); } diff --git a/qtglean/src/cpp/counter.cpp b/qtglean/src/cpp/counter.cpp index defd47093f..1c88193c17 100644 --- a/qtglean/src/cpp/counter.cpp +++ b/qtglean/src/cpp/counter.cpp @@ -5,12 +5,13 @@ #include "glean/counter.h" #include +#include #ifndef __wasm__ # include "qtglean.h" #endif -CounterMetric::CounterMetric(int id) : m_id(id) {} +CounterMetric::CounterMetric(int id) : BaseMetric(id) {} void CounterMetric::add(int amount) const { #ifndef __wasm__ @@ -29,11 +30,11 @@ int32_t CounterMetric::testGetNumRecordedErrors(ErrorType errorType) const { #endif } -int32_t CounterMetric::testGetValue(const QString& pingName) const { +QJsonValue CounterMetric::testGetValue(const QString& pingName) const { #ifndef __wasm__ - return glean_counter_test_get_value(m_id, pingName.toUtf8()); + return QJsonValue(glean_counter_test_get_value(m_id, pingName.toUtf8())); #else Q_UNUSED(pingName); - return 0; + return QJsonValue(0); #endif } diff --git a/qtglean/src/cpp/customdistribution.cpp b/qtglean/src/cpp/customdistribution.cpp index b73ee166d8..7368a12819 100644 --- a/qtglean/src/cpp/customdistribution.cpp +++ b/qtglean/src/cpp/customdistribution.cpp @@ -13,7 +13,7 @@ #include #include -CustomDistributionMetric::CustomDistributionMetric(int id) : m_id(id) {} +CustomDistributionMetric::CustomDistributionMetric(int id) : BaseMetric(id) {} void CustomDistributionMetric::accumulate_single_sample(qint64 sample) const { #ifndef __wasm__ @@ -34,26 +34,15 @@ int32_t CustomDistributionMetric::testGetNumRecordedErrors( #endif } -DistributionData CustomDistributionMetric::testGetValue( +QJsonValue CustomDistributionMetric::testGetValue( const QString& pingName) const { #ifndef __wasm__ auto value = QJsonDocument::fromJson( glean_custom_distribution_test_get_value(m_id, pingName.toUtf8())); - DistributionData result; - if (!value.isEmpty()) { - result.sum = value["sum"].toInt(); - result.count = value["count"].toInt(); - - QJsonObject values = value["values"].toObject(); - foreach (const QString& key, values.keys()) { - result.values.insert(key.toInt(), values.take(key).toInt()); - } - } - - return result; + return QJsonValue(value.object()); #else Q_UNUSED(pingName); - return DistributionData(); + return QJsonValue(QJsonObject()); #endif } diff --git a/qtglean/src/cpp/datetime.cpp b/qtglean/src/cpp/datetime.cpp index 2a7df982a3..17ef65c556 100644 --- a/qtglean/src/cpp/datetime.cpp +++ b/qtglean/src/cpp/datetime.cpp @@ -5,12 +5,13 @@ #include "glean/datetime.h" #include +#include #ifndef __wasm__ # include "qtglean.h" #endif -DatetimeMetric::DatetimeMetric(int id) : m_id(id) {} +DatetimeMetric::DatetimeMetric(int id) : BaseMetric(id) {} void DatetimeMetric::set() const { #ifndef __wasm__ @@ -25,14 +26,10 @@ int32_t DatetimeMetric::testGetNumRecordedErrors(ErrorType errorType) const { return 0; } -QString DatetimeMetric::testGetValueAsString(const QString& pingName) const { +QJsonValue DatetimeMetric::testGetValue(const QString& pingName) const { #ifndef __wasm__ - return glean_datetime_test_get_value_as_string(m_id, pingName.toUtf8()); + auto value = glean_datetime_test_get_value_as_string(m_id, pingName.toUtf8()); + return QJsonValue(value); #endif - return ""; -} - -QDateTime DatetimeMetric::testGetValue(const QString& pingName) const { - return QDateTime::fromString(testGetValueAsString(pingName), - Qt::ISODateWithMs); + return QJsonValue(""); } diff --git a/qtglean/src/cpp/event.cpp b/qtglean/src/cpp/event.cpp index 1dc3b2c8f1..53475b78e0 100644 --- a/qtglean/src/cpp/event.cpp +++ b/qtglean/src/cpp/event.cpp @@ -14,7 +14,7 @@ #include EventMetric::EventMetric(int id, EventMetricExtraParser* parser) - : m_id(id), m_parser(parser) {} + : BaseMetric(id), m_parser(parser) {} void EventMetric::record() const { #ifndef __wasm__ @@ -98,21 +98,14 @@ int32_t EventMetric::testGetNumRecordedErrors(ErrorType errorType) const { #endif } -QList EventMetric::testGetValue(const QString& pingName) const { +QJsonValue EventMetric::testGetValue(const QString& pingName) const { #ifndef __wasm__ - auto value = glean_event_test_get_value(m_id, pingName.toUtf8()); - auto recordedEvents = QJsonDocument::fromJson(value).array(); - QList result; - if (!recordedEvents.isEmpty()) { - for (const QJsonValue& recordedEvent : recordedEvents) { - Q_ASSERT(recordedEvent.isObject()); - result.append(recordedEvent.toObject()); - } - } + auto value = QJsonDocument::fromJson( + glean_event_test_get_value(m_id, pingName.toUtf8())); - return result; + return QJsonValue(value.array()); #else Q_UNUSED(pingName); - return QList(); + return QJsonValue(QJsonArray()); #endif } diff --git a/qtglean/src/cpp/memorydistribution.cpp b/qtglean/src/cpp/memorydistribution.cpp index b117f9d8dc..dc0f41f0bd 100644 --- a/qtglean/src/cpp/memorydistribution.cpp +++ b/qtglean/src/cpp/memorydistribution.cpp @@ -22,7 +22,7 @@ // stayed in so that this work is not lost, in the event we want to use // this data type in the future. -MemoryDistributionMetric::MemoryDistributionMetric(int id) : m_id(id) {} +MemoryDistributionMetric::MemoryDistributionMetric(int id) : BaseMetric(id) {} void MemoryDistributionMetric::accumulate(qint64 sample) const { #ifndef __wasm__ @@ -43,26 +43,15 @@ int32_t MemoryDistributionMetric::testGetNumRecordedErrors( #endif } -DistributionData MemoryDistributionMetric::testGetValue( +QJsonValue MemoryDistributionMetric::testGetValue( const QString& pingName) const { #ifndef __wasm__ auto value = QJsonDocument::fromJson( glean_memory_distribution_test_get_value(m_id, pingName.toUtf8())); - DistributionData result; - if (!value.isEmpty()) { - result.sum = value["sum"].toInt(); - result.count = value["count"].toInt(); - - QJsonObject values = value["values"].toObject(); - foreach (const QString& key, values.keys()) { - result.values.insert(key.toInt(), values.take(key).toInt()); - } - } - - return result; + return QJsonValue(value.object()); #else Q_UNUSED(pingName); - return DistributionData(); + return QJsonValue(QJsonObject()); #endif } diff --git a/qtglean/src/cpp/quantity.cpp b/qtglean/src/cpp/quantity.cpp index c3821030ec..61c271ee38 100644 --- a/qtglean/src/cpp/quantity.cpp +++ b/qtglean/src/cpp/quantity.cpp @@ -5,12 +5,13 @@ #include "glean/quantity.h" #include +#include #ifndef __wasm__ # include "qtglean.h" #endif -QuantityMetric::QuantityMetric(int id) : m_id(id) {} +QuantityMetric::QuantityMetric(int id) : BaseMetric(id) {} void QuantityMetric::set(int value) const { #ifndef __wasm__ @@ -25,9 +26,9 @@ int32_t QuantityMetric::testGetNumRecordedErrors(ErrorType errorType) const { return 0; } -int64_t QuantityMetric::testGetValue(const QString& pingName) const { +QJsonValue QuantityMetric::testGetValue(const QString& pingName) const { #ifndef __wasm__ - return glean_quantity_test_get_value(m_id, pingName.toUtf8()); + return QJsonValue(glean_quantity_test_get_value(m_id, pingName.toUtf8())); #endif - return 0; + return QJsonValue(0); } diff --git a/qtglean/src/cpp/string.cpp b/qtglean/src/cpp/string.cpp index ad67f54b99..bd770e8fd0 100644 --- a/qtglean/src/cpp/string.cpp +++ b/qtglean/src/cpp/string.cpp @@ -5,12 +5,13 @@ #include "glean/string.h" #include +#include #ifndef __wasm__ # include "qtglean.h" #endif -StringMetric::StringMetric(int id) : m_id(id) {} +StringMetric::StringMetric(int id) : BaseMetric(id) {} void StringMetric::set(QString value) const { #ifndef __wasm__ @@ -25,9 +26,9 @@ int32_t StringMetric::testGetNumRecordedErrors(ErrorType errorType) const { return 0; } -QString StringMetric::testGetValue(const QString& pingName) const { +QJsonValue StringMetric::testGetValue(const QString& pingName) const { #ifndef __wasm__ - return glean_string_test_get_value(m_id, pingName.toUtf8()); + return QJsonValue(glean_string_test_get_value(m_id, pingName.toUtf8())); #endif - return ""; + return QJsonValue(""); } diff --git a/qtglean/src/cpp/timingdistribution.cpp b/qtglean/src/cpp/timingdistribution.cpp index b3e016da51..0ae9abd2dd 100644 --- a/qtglean/src/cpp/timingdistribution.cpp +++ b/qtglean/src/cpp/timingdistribution.cpp @@ -13,7 +13,7 @@ #include #include -TimingDistributionMetric::TimingDistributionMetric(int id) : m_id(id) {} +TimingDistributionMetric::TimingDistributionMetric(int id) : BaseMetric(id) {} qint64 TimingDistributionMetric::start() const { #ifndef __wasm__ @@ -50,26 +50,15 @@ int32_t TimingDistributionMetric::testGetNumRecordedErrors( #endif } -DistributionData TimingDistributionMetric::testGetValue( +QJsonValue TimingDistributionMetric::testGetValue( const QString& pingName) const { #ifndef __wasm__ auto value = QJsonDocument::fromJson( glean_timing_distribution_test_get_value(m_id, pingName.toUtf8())); - DistributionData result; - if (!value.isEmpty()) { - result.sum = value["sum"].toInt(); - result.count = value["count"].toInt(); - - QJsonObject values = value["values"].toObject(); - foreach (const QString& key, values.keys()) { - result.values.insert(key.toInt(), values.take(key).toInt()); - } - } - - return result; + return QJsonValue(value.object()); #else Q_UNUSED(pingName); - return DistributionData(); + return QJsonValue(QJsonObject()); #endif } diff --git a/qtglean/src/cpp/uuid.cpp b/qtglean/src/cpp/uuid.cpp index cc1a769592..8dcf6dffe2 100644 --- a/qtglean/src/cpp/uuid.cpp +++ b/qtglean/src/cpp/uuid.cpp @@ -5,12 +5,13 @@ #include "glean/uuid.h" #include +#include #ifndef __wasm__ # include "qtglean.h" #endif -UuidMetric::UuidMetric(int id) : m_id(id) {} +UuidMetric::UuidMetric(int id) : BaseMetric(id) {} void UuidMetric::set(const QString& uuid) const { #ifndef __wasm__ @@ -32,9 +33,9 @@ int32_t UuidMetric::testGetNumRecordedErrors(ErrorType errorType) const { return 0; } -QString UuidMetric::testGetValue(const QString& pingName) const { +QJsonValue UuidMetric::testGetValue(const QString& pingName) const { #ifndef __wasm__ - return glean_uuid_test_get_value(m_id, pingName.toUtf8()); + return QJsonValue(glean_uuid_test_get_value(m_id, pingName.toUtf8())); #endif - return ""; + return QJsonValue(""); } diff --git a/src/inspector/inspectorhandler.cpp b/src/inspector/inspectorhandler.cpp index 0ad72b1920..af22fe61c9 100644 --- a/src/inspector/inspectorhandler.cpp +++ b/src/inspector/inspectorhandler.cpp @@ -67,60 +67,6 @@ InspectorItemPicker* s_itemPicker = nullptr; std::function s_constructorCallback; InspectorHotreloader* s_hotreloader = nullptr; - -QJsonObject gleanMetricToResult(const QVariant& metric, const QString& pingName) { - QJsonObject obj; - - // Encode the results by metric type. - if (metric.canConvert()) { - EventMetric* event = metric.value(); - QJsonArray results; - for (const QJsonObject& value : event->testGetValue(pingName)) { - results.append(value); - } - obj["value"] = results; - return obj; - } - if (metric.canConvert()) { - BooleanMetric* bval = metric.value(); - obj["value"] = QJsonValue(bval->testGetValue(pingName)); - return obj; - } - if (metric.canConvert()) { - CounterMetric* counter = metric.value(); - obj["value"] = QJsonValue(counter->testGetValue(pingName)); - return obj; - } - if (metric.canConvert()) { - DatetimeMetric* dt = metric.value(); - obj["value"] = QJsonValue(dt->testGetValueAsString(pingName)); - return obj; - } - if (metric.canConvert()) { - QuantityMetric* quantity = metric.value(); - obj["value"] = QJsonValue(quantity->testGetValue(pingName)); - return obj; - } - if (metric.canConvert()) { - StringMetric* str = metric.value(); - obj["value"] = QJsonValue(str->testGetValue(pingName)); - return obj; - } - if (metric.canConvert()) { - UuidMetric* uuid = metric.value(); - obj["value"] = QJsonValue(uuid->testGetValue(pingName)); - return obj; - } - // TODO: Distribution metrics. - - // Otherwise, we don't support this metric type. - QString className = "unknown"; - if (metric.canConvert()) { - className = metric.value()->metaObject()->className(); - } - obj["error"] = QString("not a supported metric type (%1)").arg(className); - return obj; -} } // namespace struct InspectorSettingCommand { @@ -725,25 +671,35 @@ static QList s_commands{ "glean_test_get_value", "Get value of a Glean metric", 3, [](InspectorHandler*, const QList& arguments) { QString ping = QString(arguments[3]); + QJsonObject obj; QObject* instance = __DONOTUSE__GleanMetrics::instance(); QVariant qvCategory = instance->property(arguments[1].constData()); QObject* category = qvCategory.value(); if (category == nullptr) { - QJsonObject obj; obj["error"] = QString(arguments[1]) + "is not a valid category"; return obj; } QVariant metric = category->property(arguments[2].constData()); if (!metric.isValid()) { - QJsonObject obj; obj["error"] = QString(arguments[2]) + "is not a valid metric"; return obj; } - - return gleanMetricToResult(metric, ping); + if (metric.canConvert()) { + BaseMetric* baseMetric = metric.value(); + obj["value"] = baseMetric->testGetValue(ping); + return obj; + } + + QString className = "unknown"; + if (metric.canConvert()) { + className = metric.value()->metaObject()->className(); + } + obj["error"] = + QString("not a supported metric type (%1)").arg(className); + return obj; }}, InspectorCommand{"glean_test_reset", "Resets Glean for testing", 0, From e6124431e6667664a5e122c0eb7bad89652abae3 Mon Sep 17 00:00:00 2001 From: Naomi Kirby Date: Wed, 8 Jan 2025 11:07:10 -0800 Subject: [PATCH 04/10] Add some checks to make sure CustomDistributionMetrics work too --- tests/functional/testConnection.js | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/tests/functional/testConnection.js b/tests/functional/testConnection.js index b686ef1926..3ee733f795 100644 --- a/tests/functional/testConnection.js +++ b/tests/functional/testConnection.js @@ -84,15 +84,28 @@ describe('Connectivity', function() { return await vpn.getQueryProperty( queries.screenHome.CONTROLLER_TITLE, 'text') == 'VPN is on'; }); + + // Leave the VPN connected for 10 seconds to accumulate transfer statistics + // and then deactivate. + await vpn.wait(10 * 1000); await vpn.deactivate(); // No test for disconnecting because often it's too fast to be tracked. - await vpn.waitForCondition(async () => { return await vpn.getQueryProperty( queries.screenHome.CONTROLLER_TITLE, 'text') === 'VPN is off'; }); + // Fetch the data transfer telemetry. + await vpn.waitForCondition(async () => { + let rx = await vpn.gleanTestGetValue("connectionHealth", "dataTransferredRx", ""); + return rx.sum > 0; + }); + await vpn.waitForCondition(async () => { + let tx = await vpn.gleanTestGetValue("connectionHealth", "dataTransferredTx", ""); + return tx.sum > 0; + }); + assert.equal( await vpn.getQueryProperty( queries.screenHome.CONTROLLER_SUBTITLE, 'text'), From 4281c11fe80d28d47243457379b4d821701904c4 Mon Sep 17 00:00:00 2001 From: Naomi Kirby Date: Wed, 8 Jan 2025 11:19:37 -0800 Subject: [PATCH 05/10] A lowly lapse has left a little lint lying about. --- qtglean/include/glean/basemetric.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/qtglean/include/glean/basemetric.h b/qtglean/include/glean/basemetric.h index 7605aadf3d..867ab84f1e 100644 --- a/qtglean/include/glean/basemetric.h +++ b/qtglean/include/glean/basemetric.h @@ -13,7 +13,7 @@ class BaseMetric : public QObject { Q_DISABLE_COPY_MOVE(BaseMetric) public: - explicit BaseMetric(int aId) : m_id(aId) {}; + explicit BaseMetric(int aId) : m_id(aId){}; // Test only functions virtual QJsonValue testGetValue(const QString& pingName = "") const = 0; From 05ac02c400048160ff158e475646c0856a076091 Mon Sep 17 00:00:00 2001 From: Naomi Kirby Date: Wed, 8 Jan 2025 11:57:01 -0800 Subject: [PATCH 06/10] Fix type warning in quantity metric --- qtglean/src/cpp/quantity.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/qtglean/src/cpp/quantity.cpp b/qtglean/src/cpp/quantity.cpp index 61c271ee38..775bae1681 100644 --- a/qtglean/src/cpp/quantity.cpp +++ b/qtglean/src/cpp/quantity.cpp @@ -28,7 +28,8 @@ int32_t QuantityMetric::testGetNumRecordedErrors(ErrorType errorType) const { QJsonValue QuantityMetric::testGetValue(const QString& pingName) const { #ifndef __wasm__ - return QJsonValue(glean_quantity_test_get_value(m_id, pingName.toUtf8())); + qint64 value = glean_quantity_test_get_value(m_id, pingName.toUtf8()); + return QJsonValue(value); #endif return QJsonValue(0); } From d3d230d69c7245a23aa9d82a120a207fe3970eeb Mon Sep 17 00:00:00 2001 From: Naomi Kirby Date: Wed, 8 Jan 2025 13:43:01 -0800 Subject: [PATCH 07/10] Fixup unit tests --- tests/unit/testconnectionhealth.cpp | 57 +++++++++++++++++------------ tests/unit/testconnectionhealth.h | 2 +- tests/unit_tests/testnavigator.cpp | 42 ++++++++++----------- 3 files changed, 56 insertions(+), 45 deletions(-) diff --git a/tests/unit/testconnectionhealth.cpp b/tests/unit/testconnectionhealth.cpp index 18b49ea8ad..ca42a7c5b9 100644 --- a/tests/unit/testconnectionhealth.cpp +++ b/tests/unit/testconnectionhealth.cpp @@ -139,17 +139,15 @@ void TestConnectionHealth::metricsTestTimespan(int expectedStablePeriods, // test the 3 timespans // Expect one timespan for each period except the current one. QCOMPARE( - getTimingDistCountFromValues( - mozilla::glean::connection_health::stable_time.testGetValue().values), + getTimingDistCount( + mozilla::glean::connection_health::stable_time.testGetValue()), expectedStablePeriods); - QCOMPARE(getTimingDistCountFromValues( - mozilla::glean::connection_health::unstable_time.testGetValue() - .values), - expectedUnstablePeriods); - QCOMPARE(getTimingDistCountFromValues( - mozilla::glean::connection_health::no_signal_time.testGetValue() - .values), - expectedNoSignalPeriods); + QCOMPARE(getTimingDistCount( + mozilla::glean::connection_health::unstable_time.testGetValue()), + expectedUnstablePeriods); + QCOMPARE(getTimingDistCount( + mozilla::glean::connection_health::no_signal_time.testGetValue()), + expectedNoSignalPeriods); } void TestConnectionHealth::metricsTestErrorAndChange( @@ -176,9 +174,12 @@ void TestConnectionHealth::metricsTestErrorAndChange( mozilla::glean::connection_health::changed_to_unstable.testGetValue(); auto changeToNoSignalEvents = mozilla::glean::connection_health::changed_to_no_signal.testGetValue(); - QCOMPARE(changeToStableEvents.length(), expectedStablePeriods); - QCOMPARE(changeToUnstableEvents.length(), expectedUnstablePeriods); - QCOMPARE(changeToNoSignalEvents.length(), expectedNoSignalPeriods); + QCOMPARE(changeToStableEvents.isArray(), true); + QCOMPARE(changeToStableEvents.toArray().count(), expectedStablePeriods); + QCOMPARE(changeToUnstableEvents.isArray(), true); + QCOMPARE(changeToUnstableEvents.toArray().count(), expectedUnstablePeriods); + QCOMPARE(changeToNoSignalEvents.isArray(), true); + QCOMPARE(changeToNoSignalEvents.toArray().count(), expectedNoSignalPeriods); } void TestConnectionHealth::metricsTestCount(int expectedStablePeriods, @@ -186,23 +187,33 @@ void TestConnectionHealth::metricsTestCount(int expectedStablePeriods, int expectedNoSignalPeriods) { // test the 3 counters // Expect a non-zero counter if there has been at least one period. - QCOMPARE(mozilla::glean::connection_health::stable_count.testGetValue() > 0, - expectedStablePeriods > 0); - QCOMPARE(mozilla::glean::connection_health::unstable_count.testGetValue() > 0, - expectedUnstablePeriods > 0); - QCOMPARE( - mozilla::glean::connection_health::no_signal_count.testGetValue() > 0, - expectedNoSignalPeriods > 0); + auto stableCount = + mozilla::glean::connection_health::stable_count.testGetValue(); + auto unstableCount = + mozilla::glean::connection_health::unstable_count.testGetValue(); + auto noSignalCount = + mozilla::glean::connection_health::no_signal_count.testGetValue(); + QCOMPARE(stableCount.isDouble(), true); + QCOMPARE(stableCount.toInt(), expectedStablePeriods); + QCOMPARE(unstableCount.isDouble(), true); + QCOMPARE(unstableCount.toInt(), expectedUnstablePeriods); + QCOMPARE(noSignalCount.isDouble(), true); + QCOMPARE(noSignalCount.toInt(), expectedNoSignalPeriods); } // .count is always returning 0, so using this function for now. // Change after https://mozilla-hub.atlassian.net/browse/VPN-6186 -int TestConnectionHealth::getTimingDistCountFromValues(QHash values) { +int TestConnectionHealth::getTimingDistCount(const QJsonValue& metric) { + QJsonObject values = metric["values"].toObject(); int count = 0; for (auto i = values.constBegin(); i != values.constEnd(); ++i) { - count += i.value(); + bool okay = false; + qlonglong key = i.key().toLongLong(&okay, 10); + if (!okay) { + continue; + } + count += i.value().toInt(); } - return count; } diff --git a/tests/unit/testconnectionhealth.h b/tests/unit/testconnectionhealth.h index 0e2c554498..a2fe6e4495 100644 --- a/tests/unit/testconnectionhealth.h +++ b/tests/unit/testconnectionhealth.h @@ -40,7 +40,7 @@ class TestConnectionHealth final : public TestHelper { private: SettingsHolder* m_settingsHolder = nullptr; - int getTimingDistCountFromValues(QHash values); + int getTimingDistCount(const QJsonValue& metric); void metricsTestErrorAndChange(int expectedStablePeriods, int expectedUnstablePeriods, int expectedNoSignalPeriods); diff --git a/tests/unit_tests/testnavigator.cpp b/tests/unit_tests/testnavigator.cpp index 4e203b0268..6afe797ad8 100644 --- a/tests/unit_tests/testnavigator.cpp +++ b/tests/unit_tests/testnavigator.cpp @@ -81,9 +81,9 @@ void TestNavigator::testNavbarButtonTelemetry() { // Test homeSelected event // Verify number of events is 0 before the test auto homeSelectedEvents = - mozilla::glean::interaction::home_selected.testGetValue(); + mozilla::glean::interaction::home_selected.testGetValue().toArray(); - QCOMPARE(homeSelectedEvents.length(), expectedHomeSelectedEventsCount); + QCOMPARE(homeSelectedEvents.count(), expectedHomeSelectedEventsCount); // Click the navbar home button Navigator::instance()->requestScreenFromBottomBar(MozillaVPN::ScreenHome); @@ -91,9 +91,9 @@ void TestNavigator::testNavbarButtonTelemetry() { // Verify number of events is still 0 after the test // because the view does not have a telemetryScreenId property homeSelectedEvents = - mozilla::glean::interaction::home_selected.testGetValue(); + mozilla::glean::interaction::home_selected.testGetValue().toArray(); - QCOMPARE(homeSelectedEvents.length(), expectedHomeSelectedEventsCount); + QCOMPARE(homeSelectedEvents.count(), expectedHomeSelectedEventsCount); QString telemetryScreenId = "test-screen-id"; @@ -104,21 +104,21 @@ void TestNavigator::testNavbarButtonTelemetry() { // Test homeSelected event // Verify number of events is still 0 before the test homeSelectedEvents = - mozilla::glean::interaction::home_selected.testGetValue(); + mozilla::glean::interaction::home_selected.testGetValue().toArray(); - QCOMPARE(homeSelectedEvents.length(), expectedHomeSelectedEventsCount); + QCOMPARE(homeSelectedEvents.count(), expectedHomeSelectedEventsCount); // Click the navbar home button Navigator::instance()->requestScreenFromBottomBar(MozillaVPN::ScreenHome); expectedHomeSelectedEventsCount++; homeSelectedEvents = - mozilla::glean::interaction::home_selected.testGetValue(); + mozilla::glean::interaction::home_selected.testGetValue().toArray(); // Verify number of events and event extras after test - QCOMPARE(homeSelectedEvents.length(), expectedHomeSelectedEventsCount); + QCOMPARE(homeSelectedEvents.count(), expectedHomeSelectedEventsCount); - auto homeSelectedEventsExtras = homeSelectedEvents[0]["extra"].toObject(); + auto homeSelectedEventsExtras = homeSelectedEvents[0][QString("extra")].toObject(); QCOMPARE(homeSelectedEventsExtras["screen"].toString(), telemetryScreenId); @@ -136,9 +136,9 @@ void TestNavigator::testNavbarButtonTelemetry() { // Test messagesSelected event // Verify number of events is 0 before the test auto messagesSelectedEvents = - mozilla::glean::interaction::messages_selected.testGetValue(); + mozilla::glean::interaction::messages_selected.testGetValue().toArray(); - QCOMPARE(messagesSelectedEvents.length(), + QCOMPARE(messagesSelectedEvents.count(), expectedMessagesSelectedEventsCount); // Click the navbar messages button @@ -147,14 +147,14 @@ void TestNavigator::testNavbarButtonTelemetry() { expectedMessagesSelectedEventsCount++; messagesSelectedEvents = - mozilla::glean::interaction::messages_selected.testGetValue(); + mozilla::glean::interaction::messages_selected.testGetValue().toArray(); // Verify number of events and event extras after test - QCOMPARE(messagesSelectedEvents.length(), + QCOMPARE(messagesSelectedEvents.count(), expectedMessagesSelectedEventsCount); auto messagesSelectedEventsExtras = - messagesSelectedEvents[0]["extra"].toObject(); + messagesSelectedEvents[0][QString("extra")].toObject(); QCOMPARE(messagesSelectedEventsExtras["screen"].toString(), telemetryScreenId); @@ -176,9 +176,9 @@ void TestNavigator::testNavbarButtonTelemetry() { // Test settingsSelected event // Verify number of events is 0 before the test auto settingsSelectedEvents = - mozilla::glean::interaction::settings_selected.testGetValue(); + mozilla::glean::interaction::settings_selected.testGetValue().toArray(); - QCOMPARE(settingsSelectedEvents.length(), + QCOMPARE(settingsSelectedEvents.count(), expectedSettingsSelectedEventsCount); // Click the navbar settings button @@ -186,14 +186,14 @@ void TestNavigator::testNavbarButtonTelemetry() { expectedSettingsSelectedEventsCount++; settingsSelectedEvents = - mozilla::glean::interaction::settings_selected.testGetValue(); + mozilla::glean::interaction::settings_selected.testGetValue().toArray(); // Verify number of events and event extras after test - QCOMPARE(settingsSelectedEvents.length(), + QCOMPARE(settingsSelectedEvents.count(), expectedSettingsSelectedEventsCount); auto settingsSelectedEventsExtras = - settingsSelectedEvents[0]["extra"].toObject(); + settingsSelectedEvents[0][QString("extra")].toObject(); QCOMPARE(settingsSelectedEventsExtras["screen"].toString(), telemetryScreenId); @@ -230,8 +230,8 @@ void TestNavigator::testNavbarButtonTelemetryNoLayers() { // Test homeSelected event and verify number of events is 0 because telemetry // is not recorded when there are no layers on the screen auto homeSelectedEvents = - mozilla::glean::interaction::home_selected.testGetValue(); - QCOMPARE(homeSelectedEvents.length(), 0); + mozilla::glean::interaction::home_selected.testGetValue().toArray(); + QCOMPARE(homeSelectedEvents.count(), 0); } static TestNavigator s_testNavigator; From edc775408be35cadaabb0c573f4c2284b129cc6e Mon Sep 17 00:00:00 2001 From: Naomi Kirby Date: Wed, 8 Jan 2025 13:59:14 -0800 Subject: [PATCH 08/10] Egadz! The Lint is Alive! --- tests/unit/testconnectionhealth.cpp | 14 +++++++------- tests/unit_tests/testnavigator.cpp | 15 ++++++--------- 2 files changed, 13 insertions(+), 16 deletions(-) diff --git a/tests/unit/testconnectionhealth.cpp b/tests/unit/testconnectionhealth.cpp index ca42a7c5b9..02e20e6b40 100644 --- a/tests/unit/testconnectionhealth.cpp +++ b/tests/unit/testconnectionhealth.cpp @@ -138,14 +138,14 @@ void TestConnectionHealth::metricsTestTimespan(int expectedStablePeriods, int expectedNoSignalPeriods) { // test the 3 timespans // Expect one timespan for each period except the current one. - QCOMPARE( - getTimingDistCount( - mozilla::glean::connection_health::stable_time.testGetValue()), - expectedStablePeriods); QCOMPARE(getTimingDistCount( - mozilla::glean::connection_health::unstable_time.testGetValue()), - expectedUnstablePeriods); + mozilla::glean::connection_health::stable_time.testGetValue()), + expectedStablePeriods); QCOMPARE(getTimingDistCount( + mozilla::glean::connection_health::unstable_time.testGetValue()), + expectedUnstablePeriods); + QCOMPARE( + getTimingDistCount( mozilla::glean::connection_health::no_signal_time.testGetValue()), expectedNoSignalPeriods); } @@ -189,7 +189,7 @@ void TestConnectionHealth::metricsTestCount(int expectedStablePeriods, // Expect a non-zero counter if there has been at least one period. auto stableCount = mozilla::glean::connection_health::stable_count.testGetValue(); - auto unstableCount = + auto unstableCount = mozilla::glean::connection_health::unstable_count.testGetValue(); auto noSignalCount = mozilla::glean::connection_health::no_signal_count.testGetValue(); diff --git a/tests/unit_tests/testnavigator.cpp b/tests/unit_tests/testnavigator.cpp index 6afe797ad8..4733689dac 100644 --- a/tests/unit_tests/testnavigator.cpp +++ b/tests/unit_tests/testnavigator.cpp @@ -118,7 +118,8 @@ void TestNavigator::testNavbarButtonTelemetry() { // Verify number of events and event extras after test QCOMPARE(homeSelectedEvents.count(), expectedHomeSelectedEventsCount); - auto homeSelectedEventsExtras = homeSelectedEvents[0][QString("extra")].toObject(); + auto homeSelectedEventsExtras = + homeSelectedEvents[0][QString("extra")].toObject(); QCOMPARE(homeSelectedEventsExtras["screen"].toString(), telemetryScreenId); @@ -138,8 +139,7 @@ void TestNavigator::testNavbarButtonTelemetry() { auto messagesSelectedEvents = mozilla::glean::interaction::messages_selected.testGetValue().toArray(); - QCOMPARE(messagesSelectedEvents.count(), - expectedMessagesSelectedEventsCount); + QCOMPARE(messagesSelectedEvents.count(), expectedMessagesSelectedEventsCount); // Click the navbar messages button Navigator::instance()->requestScreenFromBottomBar( @@ -150,8 +150,7 @@ void TestNavigator::testNavbarButtonTelemetry() { mozilla::glean::interaction::messages_selected.testGetValue().toArray(); // Verify number of events and event extras after test - QCOMPARE(messagesSelectedEvents.count(), - expectedMessagesSelectedEventsCount); + QCOMPARE(messagesSelectedEvents.count(), expectedMessagesSelectedEventsCount); auto messagesSelectedEventsExtras = messagesSelectedEvents[0][QString("extra")].toObject(); @@ -178,8 +177,7 @@ void TestNavigator::testNavbarButtonTelemetry() { auto settingsSelectedEvents = mozilla::glean::interaction::settings_selected.testGetValue().toArray(); - QCOMPARE(settingsSelectedEvents.count(), - expectedSettingsSelectedEventsCount); + QCOMPARE(settingsSelectedEvents.count(), expectedSettingsSelectedEventsCount); // Click the navbar settings button Navigator::instance()->requestScreenFromBottomBar(MozillaVPN::ScreenSettings); @@ -189,8 +187,7 @@ void TestNavigator::testNavbarButtonTelemetry() { mozilla::glean::interaction::settings_selected.testGetValue().toArray(); // Verify number of events and event extras after test - QCOMPARE(settingsSelectedEvents.count(), - expectedSettingsSelectedEventsCount); + QCOMPARE(settingsSelectedEvents.count(), expectedSettingsSelectedEventsCount); auto settingsSelectedEventsExtras = settingsSelectedEvents[0][QString("extra")].toObject(); From ea6eb4f3c44fb74a1ddca325652d539e09da0e7c Mon Sep 17 00:00:00 2001 From: Naomi Kirby Date: Wed, 8 Jan 2025 14:34:44 -0800 Subject: [PATCH 09/10] Skip telemetry checks in WASM --- tests/functional/testConnection.js | 21 ++++++++++++--------- tests/functional/testNavigation.js | 5 +++++ 2 files changed, 17 insertions(+), 9 deletions(-) diff --git a/tests/functional/testConnection.js b/tests/functional/testConnection.js index 3ee733f795..e10c0d4f64 100644 --- a/tests/functional/testConnection.js +++ b/tests/functional/testConnection.js @@ -96,15 +96,18 @@ describe('Connectivity', function() { queries.screenHome.CONTROLLER_TITLE, 'text') === 'VPN is off'; }); - // Fetch the data transfer telemetry. - await vpn.waitForCondition(async () => { - let rx = await vpn.gleanTestGetValue("connectionHealth", "dataTransferredRx", ""); - return rx.sum > 0; - }); - await vpn.waitForCondition(async () => { - let tx = await vpn.gleanTestGetValue("connectionHealth", "dataTransferredTx", ""); - return tx.sum > 0; - }); + // Telemetry doesn't work in WASM + if (!this.ctx.wasm) { + // Fetch the data transfer telemetry. + await vpn.waitForCondition(async () => { + let rx = await vpn.gleanTestGetValue("connectionHealth", "dataTransferredRx", ""); + return rx.sum > 0; + }); + await vpn.waitForCondition(async () => { + let tx = await vpn.gleanTestGetValue("connectionHealth", "dataTransferredTx", ""); + return tx.sum > 0; + }); + } assert.equal( await vpn.getQueryProperty( diff --git a/tests/functional/testNavigation.js b/tests/functional/testNavigation.js index e9da663da2..6fa7adb491 100644 --- a/tests/functional/testNavigation.js +++ b/tests/functional/testNavigation.js @@ -127,6 +127,11 @@ describe('Navigation bar', async function() { }); it("Time to main screen is recorded", async () => { + // Telemetry doesn't work in WASM + if (this.ctx.wasm) { + return; + } + // Check that the time to the main screen was greater than 1ms but less than a full second. let timing = await vpn.gleanTestGetValue("performance", "timeToMainScreen", ""); assert(timing.sum > 1000000); From 92cda00c5af0652c3038c64557d2f5aa22c8ee33 Mon Sep 17 00:00:00 2001 From: Naomi Kirby Date: Wed, 8 Jan 2025 14:53:01 -0800 Subject: [PATCH 10/10] Remove unusued DistributionData class --- qtglean/CMakeLists.txt | 1 - qtglean/include/glean/distributiondata.h | 27 ------------------------ tests/unit/testconnectionhealth.cpp | 3 +-- 3 files changed, 1 insertion(+), 30 deletions(-) delete mode 100644 qtglean/include/glean/distributiondata.h diff --git a/qtglean/CMakeLists.txt b/qtglean/CMakeLists.txt index b100d238a4..17009c5bc0 100644 --- a/qtglean/CMakeLists.txt +++ b/qtglean/CMakeLists.txt @@ -21,7 +21,6 @@ add_library(qtglean STATIC ${CMAKE_CURRENT_SOURCE_DIR}/include/glean/customdistribution.h ${CMAKE_CURRENT_SOURCE_DIR}/include/glean/event.h ${CMAKE_CURRENT_SOURCE_DIR}/include/glean/ping.h - ${CMAKE_CURRENT_SOURCE_DIR}/include/glean/distributiondata.h ${CMAKE_CURRENT_SOURCE_DIR}/include/glean/timingdistribution.h ${CMAKE_CURRENT_SOURCE_DIR}/include/glean/memorydistribution.h ${CMAKE_CURRENT_SOURCE_DIR}/include/glean/uuid.h diff --git a/qtglean/include/glean/distributiondata.h b/qtglean/include/glean/distributiondata.h deleted file mode 100644 index 4c9d5803c4..0000000000 --- a/qtglean/include/glean/distributiondata.h +++ /dev/null @@ -1,27 +0,0 @@ -/* This Source Code Form is subject to the terms of the Mozilla Public - * License, v. 2.0. If a copy of the MPL was not distributed with this - * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ - -#ifndef DISTRIBUTION_DATA_H -#define DISTRIBUTION_DATA_H - -#include - -// Based on Glean's DistributionData struct. -// https://github.com/mozilla/glean/blob/main/glean-core/src/metrics/mod.rs#L80 -// -// A snapshot of all buckets and the accumulated sum of a distribution. -struct DistributionData { - // A map containig the bucket index mapped to the accumulated count. - // - // This can contain buckets with a count of `0`. - QHash values; - // The accumulated sum of all the samples in the distribution. - int sum; - // The total number of entries in the distribution. - int count; - - DistributionData() : sum(0), count(0) {} -}; - -#endif // DISTRIBUTION_DATA_H diff --git a/tests/unit/testconnectionhealth.cpp b/tests/unit/testconnectionhealth.cpp index 02e20e6b40..6a9364bc9c 100644 --- a/tests/unit/testconnectionhealth.cpp +++ b/tests/unit/testconnectionhealth.cpp @@ -201,8 +201,7 @@ void TestConnectionHealth::metricsTestCount(int expectedStablePeriods, QCOMPARE(noSignalCount.toInt(), expectedNoSignalPeriods); } -// .count is always returning 0, so using this function for now. -// Change after https://mozilla-hub.atlassian.net/browse/VPN-6186 +// Count the number of timing distribution samples that were recorded. int TestConnectionHealth::getTimingDistCount(const QJsonValue& metric) { QJsonObject values = metric["values"].toObject(); int count = 0;