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

Kotlin: Allow configuring delayPingLifetimeIo in Kotlin #2851

Merged
merged 5 commits into from
May 31, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@

[Full changelog](https://github.com/mozilla/glean/compare/v60.2.0...main)

* Android
* Allow configuring `delayPingLifetimeIo` in Kotlin and auto-flush this data after 1000 writes.
It is also auto-flushed on background. ([#2851](https://github.com/mozilla/glean/pull/2851))

# v60.2.0 (2024-05-23)

[Full changelog](https://github.com/mozilla/glean/compare/v60.1.0...v60.2.0)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -252,7 +252,7 @@ open class GleanInternalAPI internal constructor() {
languageBindingName = LANGUAGE_BINDING_NAME,
uploadEnabled = uploadEnabled,
maxEvents = null,
delayPingLifetimeIo = false,
delayPingLifetimeIo = configuration.delayPingLifetimeIo,
appBuild = "none",
useCoreMps = false,
trimDataToRegisteredPings = false,
Expand Down Expand Up @@ -446,6 +446,9 @@ open class GleanInternalAPI internal constructor() {
* Handle the background event and send the appropriate pings.
*/
internal fun handleBackgroundEvent() {
// Persist data on backgrounding the app
persistPingLifetimeData()

gleanHandleClientInactive()
}

Expand Down Expand Up @@ -499,6 +502,16 @@ open class GleanInternalAPI internal constructor() {
return gleanSetSourceTags(tags.toList())
}

/**
* Asks the database to persist ping-lifetime data to disk. Probably expensive to call.
* Only has effect when Glean is configured with `delay_ping_lifetime_io: true`.
* If Glean hasn't been initialized this will dispatch and return Ok(()),
* otherwise it will block until the persist is done and return its Result.
*/
fun persistPingLifetimeData() {
return gleanPersistPingLifetimeData()
}

/**
* Set configuration to override metrics' enabled/disabled state, typically from a remote_settings
* experiment or rollout.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import mozilla.telemetry.glean.net.PingUploader
* @property experimentationId An experimentation identifier derived by the application
* to be sent with all pings.
* @property enableInternalPings Whether to enable internal pings.
* @property delayPingLifetimeIo Whether Glean should delay persistence of data from metrics with ping lifetime.
*/
data class Configuration @JvmOverloads constructor(
val serverEndpoint: String = DEFAULT_TELEMETRY_ENDPOINT,
Expand All @@ -38,6 +39,7 @@ data class Configuration @JvmOverloads constructor(
val enableEventTimestamps: Boolean = true,
val experimentationId: String? = null,
val enableInternalPings: Boolean = true,
val delayPingLifetimeIo: Boolean = false,
) {
companion object {
/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -877,4 +877,111 @@ class MetricsPingSchedulerTest {
server.shutdown()
}
}

fun `smoke-test for delayPingLifetimeIo=true`() {
// Essentially a copy of the
// "Data recorded before Glean inits must not get into overdue pings" test.
//
// Setting `delayPingLifetimeIo = true` on init to ensure data is flushed.
// This should then all work as expected.

// Reset Glean and do not start it right away.
Glean.testDestroyGleanHandle()

// Let's create a fake time the metrics ping was sent: this is required for
// us to not send a 'metrics' ping the first time we init glean.
val fakeNowDoNotSend = Calendar.getInstance()
fakeNowDoNotSend.clear()
fakeNowDoNotSend.set(2015, 6, 11, 4, 0, 0)
SystemClock.setCurrentTimeMillis(fakeNowDoNotSend.timeInMillis)

// Create a fake instance of the metrics ping scheduler just to set the last
// collection time.
val fakeMpsSetter = spy(MetricsPingScheduler(context, GleanBuildInfo.buildInfo))
fakeMpsSetter.updateSentDate(getISOTimeString(fakeNowDoNotSend, truncateTo = TimeUnit.DAY))

// Create a metric and set its value. We expect this to be sent in the ping that gets
// generated the SECOND time we start glean.
val expectedStringMetric = StringMetricType(
CommonMetricData(
disabled = false,
category = "telemetry",
lifetime = Lifetime.PING,
name = "expected_metric",
sendInPings = listOf("metrics"),
),
)
val expectedValue = "must-exist-in-the-first-ping"

// Start the web-server that will receive the metrics ping.
val server = getMockWebServer()
val config = Configuration(
serverEndpoint = "http://" + server.hostName + ":" + server.port,
delayPingLifetimeIo = true,
)

// Reset Glean and start it for the FIRST time, then record a value.
resetGlean(
context = context,
config = config,
)
expectedStringMetric.set(expectedValue)

// Destroy glean: it will retain the previously stored metric.
Glean.testDestroyGleanHandle()

// Create a metric and attempt to record data before Glean is initialized. This
// will be queued in the dispatcher.
val stringMetric = StringMetricType(
CommonMetricData(
disabled = false,
category = "telemetry",
lifetime = Lifetime.PING,
name = "canary_metric",
sendInPings = listOf("metrics"),
),
)
val canaryValue = "must-not-be-in-the-first-ping"
stringMetric.set(canaryValue)

// Set the current system time to a known datetime: this should make the metrics ping
// overdue and trigger it at startup.
val fakeNowTriggerPing = Calendar.getInstance()
fakeNowTriggerPing.clear()
fakeNowTriggerPing.set(2015, 6, 12, 7, 0, 0)
SystemClock.setCurrentTimeMillis(fakeNowTriggerPing.timeInMillis)

try {
// Initialize Glean the SECOND time: it will send the expected string metric (stored
// from the previous run) but must not send the canary string, which would be sent
// next time the 'metrics' ping is collected after this one.
resetGlean(
context = context,
clearStores = false,
uploadEnabled = true,
config = config,
)

// Trigger worker task to upload the pings in the background.
triggerWorkManager(context)

// Wait for the metrics ping to be received.
val request = server.takeRequest(20L, AndroidTimeUnit.SECONDS)!!
val docType = request.path!!.split("/")[3]
assertEquals("The received ping must be a 'metrics' ping", "metrics", docType)

val metricsJsonData = request.getPlainBody()

assertFalse(
"The canary metric must not be present in this ping",
metricsJsonData.contains("must-not-be-in-the-first-ping"),
)
assertTrue(
"The expected metric must be in this ping",
metricsJsonData.contains(expectedValue),
)
} finally {
server.shutdown()
}
}
}
2 changes: 1 addition & 1 deletion glean-core/rlb/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -294,7 +294,7 @@ pub fn get_timestamp_ms() -> u64 {
/// If Glean hasn't been initialized this will dispatch and return Ok(()),
/// otherwise it will block until the persist is done and return its Result.
pub fn persist_ping_lifetime_data() {
glean_core::persist_ping_lifetime_data();
glean_core::glean_persist_ping_lifetime_data();
}

#[cfg(test)]
Expand Down
65 changes: 65 additions & 0 deletions glean-core/src/database/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ use std::io;
use std::num::NonZeroU64;
use std::path::Path;
use std::str;
#[cfg(target_os = "android")]
use std::sync::atomic::{AtomicUsize, Ordering};
use std::sync::RwLock;

use crate::ErrorKind;
Expand Down Expand Up @@ -167,6 +169,13 @@ use crate::Glean;
use crate::Lifetime;
use crate::Result;

/// The number of writes we accept writes to the ping-lifetime in-memory map
/// before data is flushed to disk.
///
/// Only considered if `delay_ping_lifetime_io` is set to `true`.
#[cfg(target_os = "android")]
const PING_LIFETIME_THRESHOLD: usize = 1000;

pub struct Database {
/// Handle to the database environment.
rkv: Rkv,
Expand All @@ -184,6 +193,14 @@ pub struct Database {
/// so as to persist them to disk using rkv in bulk on demand.
ping_lifetime_data: Option<RwLock<BTreeMap<String, Metric>>>,

/// A count of how many database writes have been done since the last ping-lifetime flush.
///
/// A ping-lifetime flush is automatically done after `PING_LIFETIME_THRESHOLD` writes.
///
/// Only relevant if `delay_ping_lifetime_io` is set to `true`,
#[cfg(target_os = "android")]
ping_lifetime_count: AtomicUsize,

/// Initial file size when opening the database.
file_size: Option<NonZeroU64>,

Expand Down Expand Up @@ -263,6 +280,8 @@ impl Database {
ping_store,
application_store,
ping_lifetime_data,
#[cfg(target_os = "android")]
ping_lifetime_count: AtomicUsize::new(0),
file_size,
rkv_load_state,
};
Expand Down Expand Up @@ -528,6 +547,9 @@ impl Database {
.write()
.expect("Can't read ping lifetime data");
data.insert(final_key, metric.clone());

// flush ping lifetime
self.persist_ping_lifetime_data_if_full(&data)?;
return Ok(());
}
}
Expand Down Expand Up @@ -609,6 +631,9 @@ impl Database {
entry.insert(transform(Some(old_value)));
}
}

// flush ping lifetime
self.persist_ping_lifetime_data_if_full(&data)?;
return Ok(());
}
}
Expand Down Expand Up @@ -802,6 +827,10 @@ impl Database {
.read()
.expect("Can't read ping lifetime data");

// We can reset the write-counter. Current data has been persisted.
#[cfg(target_os = "android")]
self.ping_lifetime_count.store(0, Ordering::Release);

self.write_with_store(Lifetime::Ping, |mut writer, store| {
for (key, value) in data.iter() {
let encoded =
Expand All @@ -817,6 +846,42 @@ impl Database {
}
Ok(())
}

pub fn persist_ping_lifetime_data_if_full(
&self,
data: &BTreeMap<String, Metric>,
) -> Result<()> {
#[cfg(target_os = "android")]
{
self.ping_lifetime_count.fetch_add(1, Ordering::Release);

let write_count = self.ping_lifetime_count.load(Ordering::Relaxed);
if write_count < PING_LIFETIME_THRESHOLD {
return Ok(());
}

self.ping_lifetime_count.store(0, Ordering::Release);
let write_result = self.write_with_store(Lifetime::Ping, |mut writer, store| {
for (key, value) in data.iter() {
let encoded =
bincode::serialize(&value).expect("IMPOSSIBLE: Serializing metric failed");
// There is no need for `get_storage_key` here because
// the key is already formatted from when it was saved
// to ping_lifetime_data.
store.put(&mut writer, key, &rkv::Value::Blob(&encoded))?;
}
writer.commit()?;
Ok(())
});

return write_result;
}
#[cfg(not(target_os = "android"))]
{
_ = data; // suppress unused_variables warning.
Ok(())
}
}
}

#[cfg(test)]
Expand Down
2 changes: 2 additions & 0 deletions glean-core/src/glean.udl
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,8 @@ namespace glean {
boolean glean_set_source_tags(sequence<string> tags);
void glean_set_log_pings(boolean value);

void glean_persist_ping_lifetime_data();

void glean_handle_client_active();
void glean_handle_client_inactive();

Expand Down
12 changes: 9 additions & 3 deletions glean-core/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -697,7 +697,7 @@ pub fn shutdown() {
/// Only has effect when Glean is configured with `delay_ping_lifetime_io: true`.
/// If Glean hasn't been initialized this will dispatch and return Ok(()),
/// otherwise it will block until the persist is done and return its Result.
pub fn persist_ping_lifetime_data() {
pub fn glean_persist_ping_lifetime_data() {
// This is async, we can't get the Error back to the caller.
crate::launch_with_glean(|glean| {
let _ = glean.persist_ping_lifetime_data();
Expand Down Expand Up @@ -1129,8 +1129,14 @@ pub fn glean_test_destroy_glean(clear_stores: bool, data_path: Option<String>) {

// Only useful if Glean initialization finished successfully
// and set up the storage.
let has_storage =
core::with_opt_glean(|glean| glean.storage_opt().is_some()).unwrap_or(false);
let has_storage = core::with_opt_glean(|glean| {
// We need to flush the ping lifetime data before a full shutdown.
glean
.storage_opt()
.map(|storage| storage.persist_ping_lifetime_data())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh, wonder if this'll uncover any bugs in Desktop tests.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only one way to figure that out!

.is_some()
})
.unwrap_or(false);
if has_storage {
uploader_shutdown();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ package org.mozilla.samples.gleancore
import android.app.Application
import android.util.Log
import mozilla.telemetry.glean.Glean
import mozilla.telemetry.glean.config.Configuration
import org.mozilla.samples.gleancore.GleanMetrics.Basic
import org.mozilla.samples.gleancore.GleanMetrics.Custom
import org.mozilla.samples.gleancore.GleanMetrics.GleanBuildInfo
Expand Down Expand Up @@ -35,6 +36,7 @@ class GleanApplication : Application() {
applicationContext = applicationContext,
uploadEnabled = true,
buildInfo = GleanBuildInfo.buildInfo,
configuration = Configuration(delayPingLifetimeIo = true),
)

Test.timespan.start()
Expand Down
Loading