Add submitted pings table - #3585
Conversation
|
Some changes:
|
badboy
left a comment
badboy
left a comment
There was a problem hiding this comment.
this is starting to look pretty good (did a full review this time).
A few more things.
Right now getting those pings back from the outside is not implemented, but I assume that's gonna be done in a followup?
# Conflicts: # glean-core/benchmark/benches/dispatcher.rs # glean-core/benchmark/benches/lifetime_buffering.rs
edced9d to
0f436f4
Compare
badboy
left a comment
badboy
left a comment
There was a problem hiding this comment.
No major blocker anymore. Couple of questions.
You can also rebase this branch on main and squash down the commits.
| * Implement the event metric ([#3534](https://github.com/mozilla/glean/pull/3534)) | ||
| * BREAKING CHANGE: Switch from a noop feature to an `active` feature ([#3583](https://github.com/mozilla/glean/pull/3583)) | ||
| * iOS | ||
| * BREAKING CHANGE: Switch from a noop feature to an `active` feature ([#3583](https://github.com/mozilla/glean/pull/3583)) |
There was a problem hiding this comment.
This moving of the changelog line looks like a mistake.
| use chrono::Utc; | ||
| use std::fs; |
There was a problem hiding this comment.
Let's try to stick to the grouping as before, which means the use chrono::Utc should be down on line 10, next to the use glean_core
| ping TEXT NOT NULL, | ||
| date_submitted INTEGER NOT NULL, | ||
| date_uploaded INTEGER, | ||
| upload_failed BOOLEAN NOT NULL, |
There was a problem hiding this comment.
A thing I'm only thinking about now: Should this field be the positive, so upload_succeeded instead?
Or should this even be a date? As in the last time the upload failed for this ping (null if upload succeeded, at which point date_uploaded is set to a proper time)
This comes down to what we want to do with this field and how much information we want to expose
2 participants
Re-opening from #3575