Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug 1932322 - Implement the new collection-enabled API #3006

Merged
merged 18 commits into from
Nov 28, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions .circleci/config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,14 @@ commands:
name: Run Rust RLB flush test
command: |
glean-core/rlb/tests/test-ping-lifetime-flush.sh
- run:
name: Run Rust RLB enabled-pings test
command: |
glean-core/rlb/tests/test-enabled-pings.sh
- run:
name: Run Rust RLB pending-gets-removed test
command: |
glean-core/rlb/tests/test-pending-gets-removed.sh

install-rustup:
steps:
Expand Down
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,10 @@

* General
* Add methods to access current Glean debugging settings and the list of currently registered pings([Bug 1921976](https://bugzilla.mozilla.org/show_bug.cgi?id=1921976)).
* Require `glean_parser` v16.1.0 ([#3006](https://github.com/mozilla/glean/pull/3006))
* BREAKING CHANGE: Add new `collection-enabled` mode (and `follows_collection_enabled` setting for pings).
This allows to control a subset of pings independently from the Glean-wide `upload-enabled` flag.
This deprecates the `setUploadEnabled` API in favor of `setCollectionEnabled`. ([#3006](https://github.com/mozilla/glean/pull/3006))
* Rust
* Permit Glean shutdown to interrupt UploadManager Wait tasks ([bug 1928288](https://bugzilla.mozilla.org/show_bug.cgi?id=1928288))

Expand Down
2 changes: 1 addition & 1 deletion Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,7 @@ docs-python: build-python ## Build the Python documentation
.PHONY: docs docs-rust docs-swift

docs-metrics: setup-python ## Build the internal metrics documentation
$(GLEAN_PYENV)/bin/pip install glean_parser~=15.2
$(GLEAN_PYENV)/bin/pip install glean_parser~=16.1
$(GLEAN_PYENV)/bin/glean_parser translate --allow-reserved \
-f markdown \
-o ./docs/user/user/collected-metrics \
Expand Down
2 changes: 1 addition & 1 deletion glean-core/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ include = [
rust-version = "1.76"

[package.metadata.glean]
glean-parser = "15.2.0"
glean-parser = "16.1.0"

[badges]
circle-ci = { repository = "mozilla/glean", branch = "main" }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -289,7 +289,7 @@ open class GleanInternalAPI internal constructor() {
}

/**
* Enable or disable Glean collection and upload.
* **DEPRECATED** Enable or disable Glean collection and upload.
*
* Metric collection is enabled by default.
*
Expand All @@ -300,12 +300,36 @@ open class GleanInternalAPI internal constructor() {
*
* When enabling, the core Glean metrics are recreated.
*
* **DEPRECATION NOTICE**:
* This API is deprecated. Use `setCollectionEnabled` instead.
*
* @param enabled When true, enable metric collection.
*/
@Deprecated("Use `setCollectionEnabled` instead.")
fun setUploadEnabled(enabled: Boolean) {
gleanSetUploadEnabled(enabled)
}

/**
* Enable or disable Glean collection and upload.
*
* Metric collection is enabled by default.
*
* When collection is disabled, metrics aren't recorded at all and no data
* is uploaded.
* **Note**: Individual pings can be enabled if they don't follow this setting.
* See `PingType.setEnabled`.
*
* When disabling, all pending metrics, events and queued pings are cleared.
*
* When enabling, the core Glean metrics are recreated.
*
* @param enabled When true, enable metric collection.
*/
fun setCollectionEnabled(enabled: Boolean) {
gleanSetUploadEnabled(enabled)
}

/**
* Indicate that an experiment is running. Glean will then add an
* experiment annotation to the environment which is sent with pings. This
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ class PingType<ReasonCodesEnum> (
enabled: Boolean,
val schedulesPings: List<String>,
val reasonCodes: List<String>,
followsCollectionEnabled: Boolean,
) where ReasonCodesEnum : Enum<ReasonCodesEnum>, ReasonCodesEnum : ReasonCode {
private var testCallback: ((ReasonCodesEnum?) -> Unit)? = null
private val innerPing: GleanPingType
Expand All @@ -68,6 +69,7 @@ class PingType<ReasonCodesEnum> (
schedulesPings = schedulesPings,
reasonCodes = reasonCodes,
enabled = enabled,
followsCollectionEnabled = followsCollectionEnabled,
)
}

Expand Down Expand Up @@ -108,4 +110,14 @@ class PingType<ReasonCodesEnum> (
val reasonString = reason?.let { this.reasonCodes[it.code()] }
this.innerPing.submit(reasonString)
}

/**
* Enable or disable a ping.
*
* Disabling a ping causes all data for that ping to be removed from storage
* and all pending pings of that type to be deleted.
*/
fun setEnabled(enabled: Boolean) {
this.innerPing.setEnabled(enabled)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ class GleanTestRule(
enabled = true,
schedulesPings = emptyList(),
reasonCodes = emptyList(),
followsCollectionEnabled = true,
)
PingType<NoReasonCodes>(
name = "store2",
Expand All @@ -89,6 +90,7 @@ class GleanTestRule(
enabled = true,
schedulesPings = emptyList(),
reasonCodes = emptyList(),
followsCollectionEnabled = true,
)
PingType<NoReasonCodes>(
name = "store3",
Expand All @@ -99,6 +101,7 @@ class GleanTestRule(
enabled = true,
schedulesPings = emptyList(),
reasonCodes = emptyList(),
followsCollectionEnabled = true,
)

Glean.resetGlean(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ class GleanTest {

@After
fun resetGlobalState() {
Glean.setUploadEnabled(true)
Glean.setCollectionEnabled(true)
}

@Test
Expand Down Expand Up @@ -121,7 +121,7 @@ class GleanTest {
sendInPings = listOf("store1"),
),
)
Glean.setUploadEnabled(false)
Glean.setCollectionEnabled(false)

stringMetric.set("foo")
assertNull(stringMetric.testGetValue())
Expand Down Expand Up @@ -483,6 +483,7 @@ class GleanTest {
enabled = true,
schedulesPings = emptyList(),
reasonCodes = listOf(),
followsCollectionEnabled = true,
)
val stringMetric = StringMetricType(
CommonMetricData(
Expand Down Expand Up @@ -542,12 +543,12 @@ class GleanTest {
stringMetric.set("TEST VALUE")
assertEquals("TEST VALUE", stringMetric.testGetValue()!!)

Glean.setUploadEnabled(false)
Glean.setCollectionEnabled(false)
assertNull(stringMetric.testGetValue())
stringMetric.set("TEST VALUE")
assertNull(stringMetric.testGetValue())

Glean.setUploadEnabled(true)
Glean.setCollectionEnabled(true)
assertNull(stringMetric.testGetValue())
stringMetric.set("TEST VALUE")
assertEquals("TEST VALUE", stringMetric.testGetValue()!!)
Expand All @@ -557,10 +558,10 @@ class GleanTest {
fun `Core metrics should be cleared and restored when disabling and enabling uploading`() {
assertNotNull(GleanInternalMetrics.os.testGetValue())

Glean.setUploadEnabled(false)
Glean.setCollectionEnabled(false)
assertNull(GleanInternalMetrics.os.testGetValue())

Glean.setUploadEnabled(true)
Glean.setCollectionEnabled(true)
assertNotNull(GleanInternalMetrics.os.testGetValue())
}

Expand All @@ -585,7 +586,7 @@ class GleanTest {
Glean.metricsPingScheduler!!.timer != null,
)

Glean.setUploadEnabled(true)
Glean.setCollectionEnabled(true)

// Verify that the workers are still enqueued to show that setting upload enabled to true
// doesn't affect any already queued workers, since we ask consumers to set upload enabled
Expand All @@ -600,7 +601,7 @@ class GleanTest {
)

// Toggle upload enabled to false
Glean.setUploadEnabled(false)
Glean.setCollectionEnabled(false)

// Verify workers have been cancelled
assertTrue(
Expand Down Expand Up @@ -853,6 +854,7 @@ class GleanTest {
enabled = true,
schedulesPings = emptyList(),
reasonCodes = listOf(),
followsCollectionEnabled = true,
)
val stringMetric = StringMetricType(
CommonMetricData(
Expand All @@ -871,7 +873,7 @@ class GleanTest {
Glean.initialize(context, true, config, GleanBuildInfo.buildInfo)

// Glean might still be initializing. Disable upload.
Glean.setUploadEnabled(false)
Glean.setCollectionEnabled(false)

// Set data and try to submit a custom ping.
val testValue = "SomeTestValue"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -253,6 +253,7 @@ class GleanDebugActivityTest {
enabled = true,
schedulesPings = emptyList(),
reasonCodes = listOf(),
followsCollectionEnabled = true,
)

// Set the extra values and start the intent.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ class CustomPingTest {
enabled = true,
schedulesPings = emptyList(),
reasonCodes = emptyList(),
followsCollectionEnabled = true,
)

customPing.submit()
Expand Down Expand Up @@ -115,6 +116,7 @@ class CustomPingTest {
enabled = true,
schedulesPings = emptyList(),
reasonCodes = emptyList(),
followsCollectionEnabled = true,
)

// Trigger the ping twice.
Expand Down Expand Up @@ -174,6 +176,7 @@ class CustomPingTest {
enabled = true,
schedulesPings = emptyList(),
reasonCodes = emptyList(),
followsCollectionEnabled = true,
)

// Define a 'click' event
Expand Down Expand Up @@ -252,6 +255,7 @@ class CustomPingTest {
enabled = true,
schedulesPings = emptyList(),
reasonCodes = emptyList(),
followsCollectionEnabled = true,
)

customPing.submit()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ class DeletionPingTest {
val pendingDeletionRequestDir = File(Glean.getDataDir(), DELETION_PING_DIR)

// Disabling upload generates a deletion ping
Glean.setUploadEnabled(false)
Glean.setCollectionEnabled(false)
triggerWorkManager(context)

val request = server.takeRequest(2L, TimeUnit.SECONDS)!!
Expand All @@ -107,7 +107,7 @@ class DeletionPingTest {

// Re-setting upload to `false` should not generate an additional ping
// and no worker should be scheduled.
Glean.setUploadEnabled(false)
Glean.setCollectionEnabled(false)

assertFalse(getWorkerStatus(context, PingUploadWorker.PING_WORKER_TAG).isEnqueued)
// No new file should have been written
Expand Down Expand Up @@ -182,7 +182,7 @@ class DeletionPingTest {
)

// Disabling upload generates a deletion ping
Glean.setUploadEnabled(false)
Glean.setCollectionEnabled(false)
triggerWorkManager(context)

val request = server.takeRequest(2L, TimeUnit.SECONDS)!!
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ class RidealongPingTest {
enabled = true,
schedulesPings = emptyList(),
reasonCodes = emptyList(),
followsCollectionEnabled = true,
)

Glean.handleBackgroundEvent()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -258,14 +258,14 @@ class EventMetricTypeTest {
),
allowedExtraKeys = listOf("test_name"),
)
Glean.setUploadEnabled(true)
Glean.setCollectionEnabled(true)
eventMetric.record(EventMetricExtras(testName = "event1"))
val snapshot1 = eventMetric.testGetValue()!!
assertEquals(1, snapshot1.size)
Glean.setUploadEnabled(false)
Glean.setCollectionEnabled(false)
eventMetric.record(EventMetricExtras(testName = "event2"))
assertNull(eventMetric.testGetValue())
Glean.setUploadEnabled(true)
Glean.setCollectionEnabled(true)
eventMetric.record(EventMetricExtras(testName = "event3"))
val snapshot3 = eventMetric.testGetValue()!!
assertEquals(1, snapshot3.size)
Expand Down Expand Up @@ -455,10 +455,10 @@ class EventMetricTypeTest {
}

@Test
fun `overdue events are discarded if ping is not registered`() {
fun `events are discarded if ping is not registered`() {
// This is similar to the above test,
// except that we register the custom ping AFTER initialize.
// Overdue events are thus discarded because the ping is unknown at initialization time.
// Events are thus discarded upon `record` because the ping is unknown.

val server = getMockWebServer()
val context = getContext()
Expand All @@ -484,9 +484,9 @@ class EventMetricTypeTest {
allowedExtraKeys = listOf("some_extra"),
)

// Let's record a single event. This will be queued up but not be sent.
// Let's record a single event. This will NOT be queued up because the ping is not registered.
event.record(TestEventExtras(someExtra = "alternative"))
assertEquals(1, event.testGetValue()!!.size)
assertNull(event.testGetValue())

// Let's act as if the app was stopped
Glean.testDestroyGleanHandle()
Expand All @@ -511,6 +511,7 @@ class EventMetricTypeTest {
enabled = true,
schedulesPings = emptyList(),
reasonCodes = listOf(),
followsCollectionEnabled = true,
)

// Trigger worker task to upload the pings in the background.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ class ObjectMetricTypeTest {
),
)

Glean.setUploadEnabled(true)
Glean.setCollectionEnabled(true)

var balloons = BalloonsObject()
balloons.add(BalloonsObjectItem(colour = "yellow", diameter = 10))
Expand All @@ -177,7 +177,7 @@ class ObjectMetricTypeTest {
var expected: JsonElement = Json.decodeFromString(expectedJson)
assertEquals(expected, metric.testGetValue()!!)

Glean.setUploadEnabled(false)
Glean.setCollectionEnabled(false)
balloons = BalloonsObject()
balloons.add(BalloonsObjectItem(colour = "blue", diameter = 1))
metric.set(balloons)
Expand Down
Loading
Loading