Bug 2034605 - Switch to an SQLite storage backend by badboy · Pull Request #3504 · mozilla/glean

badboy · 2026-06-17T11:39:22Z

This is essentially #3405 but from the branch we've been slowly merging into:

main <- main-sqlite.

This is what will finally get merged.
It won't need a full review again, all indidivual pieces have been reviewed previously.
This branch is rebased against main to ensure we do not lose any commits from main.

badboy · 2026-06-17T12:16:11Z

/run-ios

This is a modified version of the kvstore/skv implementation: https://searchfox.org/firefox-main/rev/cced10961b53e0d29e22e635404fec37728b2644/toolkit/components/kvstore/src/skv/connection.rs Which itself is based on application-service's sql-support. It's stripped down to what we need in Glean: * A file-backed database * A schema set up on start, potentially applying migrations if we need that * A read-write connection, which is re-used for all access.

This only integrates it into the module tree. It compiles, but not warning-free. It fully replaces the Rkv storage. No migration implemented.

Now that it's just another column this becomes straight-forward to do.

The bincode crate isn't maintained anymore. While it's been stable and without issues for us for years, switching to anotherformat is easy while we're switching the database anyway. MessagePack can be even smaller than bincode for the same data (just a couple of bytes here and there). Whether it's actually faster has not been benchmarked. Compared to everything else the (de)serialization overhead is probably a small fraction of the whole thing. Why do we need serialization anyway? Ping assembly does not have any knowledge of metrics. It only knows what's in the database. So in order to put in in the right place in the ping payload we need to know the type of the stored data. That data needs to be somewhere. By serializing the whole value (the `Metric` enum) we can deserialize it into that enum and the serde part takes care of "knowing" the type.

Same way this was done on Rkv: we just some up the size of all files in the database directory.

…f silently dropping errors

…l moments See all details: https://sqlite.org/pragma.html#pragma_synchronous The default (FULL) syncs on every write. That's slightly higher guarantees, but also costly. We're already using WAL (write-ahead log). It's safe from corruption in NORMAL mode and consistent. It does lose durability, that means data might roll back following a power loss or system crash. Note: `rkv` does NOT sync at all. It only writes to disk (and moves files around). That's strictly worse than WAL in `NORMAL` mode.

It's now easier to do: query the column and count. There's some complications when we get to dual-labeled metrics, but that comes later.

This will unify label check code: all cases are handled through the same code paths, just that for the static label variant we don't need to do any more checks.

Basically anything that assumes the database layout of rkv, now that it has been reimplemented with sqlite.

…ater point downside: slightly worse error messages, but maybe we can inline them

This is another BREAKING CHANGE in the return type. We can't return references to the labels anymore, we need owned values.

It will be applied at start if (1) no sqlite database is detected, and (2) an Rkv database is detected. Migration works by iterating through all data in the rkv "safe-mode" database and inserting it into the new database. The Rkv database will be kept on disk. This will allow for a rollback if any problems are detected in production and we can implement a recovery step then. migrate rename

These tests were disabled because they are very rkv-specific: Manually opening and writing to an Rkv database in the format that Glean expects. Then testing Glean behaves accordingly. We now do the same, but do it in SQL.

What individual tests do should be clear from their name or further comments inline.

This currently fails. The database is locked, so Glean can't access it. It's unclear how we should handle that. It's not a particular likely case to happen in practice.

The data was generated with cargo run -p glean-tests --bin verify-data -- tmp on an Rkv-powered Glean checkout. The database (`tmp/db/data.safe.bin`) was then copied into glean-core/rlb/tests/rkv-database.safe.bin

The previous refactoring duplicated some of the logic between different parts. Now we unify them again.

…ndly panicking

[doc only]

badboy · 2026-06-19T14:34:16Z

/run-ios

chutten

r+wc

Possible later augmentation: on filesystem errors creating the db, go in-memory (https://sqlite.org/inmemorydb.html) so we're still able to report the failure.

chutten · 2026-06-19T15:19:35Z

+# 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/.
+
+# This file defines the metrics that are recorded by the Glean SDK. They are


copy-pasta comment?

chutten · 2026-06-19T15:19:52Z

+# 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/.
+
+# This file defines the built-in pings that are recorded by the Glean SDK. They


chutten · 2026-06-19T15:20:47Z


 impl<'a, K> MetricIdentifier<'a> for ObjectMetric<K> {
-    fn get_identifiers(&'a self) -> (&'a str, &'a str, Option<&'a str>) {
+    fn get_identifiers(&'a self) -> (&'a str, &'a str, Option<String>) {


musing: I do wonder how much more expensive this will prove to be in practice

chutten · 2026-06-19T15:24:01Z

+                .set_raw_sync(&glean, state.duration);
+        }
+
+        if let Some(()) = glean.data_store.as_mut().unwrap().migration_error.take() {


This looks goofy. "If the error is something, but a zero-sized something, report a single error." Not sure there's any improvement to be made, I guess, but it looked odd so I thought I'd err on the side of saying something.

chutten · 2026-06-19T15:26:36Z

+            };
+
+            warn_on_error(
+                data.clear_lifetime_storage(Lifetime::User, "glean_internal_info"),


Should we use INTERNAL_STORAGE here?

chutten · 2026-06-19T16:28:06Z

+    pub failed_metrics: CounterMetric,
+
+    /// The duration for one full migration run at startup
+    pub migration_duration: TimespanMetric,


May want to change this to timing_distribution as the other state and error metrics account for multiple migrations in the same ping

chutten · 2026-06-19T16:30:22Z

+    {
+        let path = temp.path().join("db").join("glean.sqlite");
+        fs::remove_file(&path).unwrap();
+        fs::write(&path, "not sqlite").unwrap();


(not a bug): I suppose if Magritte's PR is ever merged to make "not sqlite" a valid sqlite db, this test will fail until we find a new sentinel...

chutten · 2026-06-19T16:32:07Z

+    let (glean, _temp) = new_glean(Some(temp));
+
+    let client_id = clientid_metric().get_value(&glean, None);
+    assert!(client_id.is_some());


Is there an error/loadstate metric we could also validate here?

chutten · 2026-06-19T16:32:54Z

+    let (glean, _temp) = new_glean(Some(temp));
+
+    let client_id = clientid_metric().get_value(&glean, None);
+    assert!(client_id.is_some());


ditto, can we check an error metric or something

chutten · 2026-06-19T16:34:18Z

+    }
+}
+
+// TODO:


TODO should either be resolved or a follow-up bug filed and referenced

badboy force-pushed the main-sqlite branch 4 times, most recently from 13689af to 899269d Compare June 17, 2026 12:13

badboy force-pushed the main-sqlite branch from ab15738 to af853bc Compare June 18, 2026 13:14

badboy added the sqlite Any changes to the new SQLite storage backend label Jun 18, 2026

badboy added 23 commits June 19, 2026 14:40

Rust dependency: Add rusqlite

1c0e3e3

Vetting newly imported crates

eca6ca8

Use the sqlite module as the database layer

bf23411

This only integrates it into the module tree. It compiles, but not warning-free. It fully replaces the Rkv storage. No migration implemented.

sqlite: Implement clearing of lifetime storage

69c83a0

Now that it's just another column this becomes straight-forward to do.

sqlite: Measure database on-disk size by scanning the directory

37d126e

Same way this was done on Rkv: we just some up the size of all files in the database directory.

Use correct parameter and properly warn when clearing fails instead o…

70a71f7

…f silently dropping errors

Implement the label verifier against sqlite storage

ea64406

It's now easier to do: query the column and count. There's some complications when we get to dual-labeled metrics, but that comes later.

Correctly assemble payload for labeled metrics

e098fe4

Simplify getting metrics out again

31e3f4d

Handle static labels like dynamic ones: stored along with the metric

3f4669b

This will unify label check code: all cases are handled through the same code paths, just that for the static label variant we don't need to do any more checks.

Copy over is_ping_enabled check instead of is_upload_enabled

5d6c405

Rework how dynamic labels are validated

d53a0c0

Correctly validate usage of key-only/category-only dual-labels

7b57fe3

Remove now-unused database code

7bc67bd

Basically anything that assumes the database layout of rkv, now that it has been reimplemented with sqlite.

Switch to get_metric everywhere

2abfc54

Return a specific LabelCheck that knows how to report an error at a l…

353aa6f

…ater point downside: slightly worse error messages, but maybe we can inline them

Remove strip_label usage now that labels are stored separately

9506008

Implement get_identifiers based on the new label storage.

400557f

This is another BREAKING CHANGE in the return type. We can't return references to the labels anymore, we need owned values.

Rename to MetricLabel now that it's always used to store the label

29e556b

badboy added 15 commits June 19, 2026 14:40

Re-enable client_id tests

76d0946

These tests were disabled because they are very rkv-specific: Manually opening and writing to an Rkv database in the format that Glean expects. Then testing Glean behaves accordingly. We now do the same, but do it in SQL.

Test: ensure we don't record errors if we aren't enabled

4ed77f1

Test several sqlite edge cases

100af3a

What individual tests do should be clear from their name or further comments inline.

Test what happens if database is otherwise locked

0d7dbf3

This currently fails. The database is locked, so Glean can't access it. It's unclear how we should handle that. It's not a particular likely case to happen in practice.

Test that all metrics are properly migrated

ae9c8b7

The data was generated with cargo run -p glean-tests --bin verify-data -- tmp on an Rkv-powered Glean checkout. The database (`tmp/db/data.safe.bin`) was then copied into glean-core/rlb/tests/rkv-database.safe.bin

Handle error recording in a single place again

aa9e344

The previous refactoring duplicated some of the logic between different parts. Now we unify them again.

Handle sqlite load errors

6bc4e3a

Handle possible SQLite query errors and bubble them up instead of bli…

fd34f1f

…ndly panicking

Record status metrics after migration

128c352

sqlite: Better variable names in dual-labeled metric code

44dccba

sqlite: Improve comments to better explain what's happening (or not)

52721c2

sqlite: Avoid a potential panic and warn instead

a913dfc

sqlite: Add todo bugs to the open todos

2c28c09

sqlite: Log metric name during label checking

fff641d

Add correct bug for new sqlite migration metrics

43c4f6b

[doc only]

badboy force-pushed the main-sqlite branch from af853bc to 43c4f6b Compare June 19, 2026 12:42

badboy added 2 commits June 19, 2026 15:26

Keep rkv_load_error active while Rkv-powered Glean is still in use

f016952

Changelog entry for SQLite storage

a6fab2f

badboy marked this pull request as ready for review June 19, 2026 14:34

badboy requested a review from a team as a code owner June 19, 2026 14:34

badboy requested review from jeddai and removed request for a team June 19, 2026 14:34

badboy requested review from chutten and travis79 June 19, 2026 14:34

chutten approved these changes Jun 19, 2026

View reviewed changes

Conversation

badboy commented Jun 17, 2026

Uh oh!

badboy commented Jun 17, 2026

Uh oh!

badboy commented Jun 19, 2026

Uh oh!

chutten left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants