-
Notifications
You must be signed in to change notification settings - Fork 124
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 1656589 - Track the database size on initialization #1141
Conversation
97c12e8
to
6ebab93
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM, but how does it make the tests on Android fail!?
I think I know why: we now have a metric available for each metrics ping. That brings up the questions:
|
It doesn't, I compiled the wrong code. |
I think Ping lifetime is fine. If we restart multiple times before sending a metric ping, we're sending a timing distribution anyway, so we'll record multiple values.
Yes, I believe it is, still. We make no guarantee or assumption about this ping not being sent. |
6ebab93
to
4c73754
Compare
I had to resort to sort of a small hack to get tests passing by ignoring the metrics ping (by essentially delaying it). That touches far too many tests to my liking, but it's the unfortunate truth how it all works. I'd like some review on that. It also means we now always have a metrics ping because we are recording metrics (but in actual applications we might not be the only ones anyway, so probably not a real problem?) |
Ah! I also think the same has to be done for iOS, which I haven't done yet. I also never ran the tests for iOS yet. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
6c8faaa
to
e560ae7
Compare
This avoids the need to deal with an "overdue" metrics ping on start.
e560ae7
to
d956dbd
Compare
No description provided.