Import sqlite storage as separate module#3449
Conversation
badboy
force-pushed
the
sqlite/initial-import
branch
from
b65fc42 to
d3d4e1b
Compare
| lifetime TEXT NOT NULL, | ||
| labels TEXT NOT NULL, -- can't be null or ON CONFLICT won't work | ||
| value BLOB, | ||
| UNIQUE(id, ping, labels) |
There was a problem hiding this comment.
This reflects a change in semantics, doesn't it? Previously, if a metric changed lifetime we'd store both.
Also: why are labels part of the unique constraint?
There was a problem hiding this comment.
a) That's true, but it's kind of ... is it a bug? Unintended side-effect? We can never sent out two values. And based on the order of lifetimes by which we fetch values (ping, app, user) we end up with the "longest" one.
b) Because that's what makes it unique?
A labeled metric cat.name will add these rows:
| id | ping | labels | value |
|---|---|---|---|
| cat.name | metrics | label1 | 1 |
| cat.name | custom | label1 | 1 |
| cat.name | metrics | label2 | 31 |
| cat.name | custom | label2 | 31 |
We use ON CONFLICT to do an upsert and that acts on the uniqueness constraint.
If we would not put labels in the unique constraint, we would overwrite previous rows.
Is that more clear?
| /// | ||
| /// This function will **not** panic on database errors. | ||
| pub fn clear_ping_lifetime_storage(&self, storage_name: &str) -> Result<()> { | ||
| let clear_sql = "DELETE FROM telemetry WHERE lifetime = ?1 AND ping = ?2"; |
There was a problem hiding this comment.
Why not include Lifetime::Ping.as_str() in the statement directly?
There was a problem hiding this comment.
Guess mostly to avoid copy-n-paste errors should we ever consider renaming Ping :P (I'll inline it)
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.
badboy
force-pushed
the
sqlite/initial-import
branch
from
09f39e8 to
a643cd3
Compare
|
Woops, I initially pushed 3 changes as 3 separate commits, but then locally already squashed them and repushed accidentally. Only the 3 changes can be seen here: d3d4e1b...09f39e8 |
2 participants
My plan over the next few weeks is to split up #3405 into reviewable pieces and merge them piece by piece.
For that I created the
main-sqlitebranch, which currently is the same asmain. This way any feature work/bug fixes against the current Glean can continue to go tomainwithout issues.Over time I can rebase/merge that branch against
mainto keep-up-to-date, but keep further sqlite work reviewable.this very first part is merely adding the first pieces of code, but none of that is actually part of the compilation yet (wouldn't work because it's not fully usable yet).
The details about the sqlite setup will change in later commits. Re-arranging that into a single commit that has the perfect setup would be a lot of work of splitting and rebasing commits (and squashing and merge conflicts), so doing it this way seemed easier.