-
Notifications
You must be signed in to change notification settings - Fork 217
Call Glean initialize prior to set startup metrtics. #3130
Conversation
@Dexterp37 Please help review this change if it makes in Glean. Thanks. |
@@ -46,13 +46,14 @@ public static void init(Context aContext) { | |||
initialized = true; | |||
|
|||
final boolean telemetryEnabled = SettingsStore.getInstance(aContext).isTelemetryEnabled(); | |||
Configuration config = new Configuration(Configuration.DEFAULT_TELEMETRY_ENDPOINT, BuildConfig.BUILD_TYPE); |
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.
@daoshengmu This change is not require nor useful: any metric set before glean initialized is batched and recorded after glean initializes, automatically (up to 100 batched actions). Why did you move this?
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.
Based on the conversation with @travis in Matrix
It should still work, as Glean will cache up to 100 operations before you call initialize, but if there are several things going on at that time then it could be a factor in why you are seeing nulls. If GleanMetricsService.start() performs some recording, then I would try and call initialize prior to that, if possible. If telemetry is disabled, then Glean.initialize will do the right things in that case, so I'm also a little curious why you aren't passing your telemetryEnabled into Glean.initialize
Otherwise, we have no other choice can do, and our code freeze is on Apr. 10th.
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.
That conversation with @travis79 is correct, and it's easy to verify without making this change, since Glean reports a metric about this problem. The conversation also assumes you're on a recent version of Glean SDK, but you're not.
You should always try to update to the latest version of the Glean SDK to get the latest bugfixes and improvements. This is also required for things such as #3096.
Please note that the problem you're seeing is most probably not your product dropping data due to the pre-init queue overflowing, but this bug that was recently fixed.
Moreover, the reason why you're seeing missing distribution_name
field sometimes, is probably due to the fact that the application is crashing and not able to close properly. If you were on a more recent version of the SDK, then you could tell by examining the "ping reasons".
My tl;dr - please revert this change, as on the version you are on this might cause dataloss, update to a more recent version of glean as soon as it fits your schedule.
This reverts commit bce60f9.
Fixes #3129
Based on the recent Glean updates ( https://github.com/mozilla/glean/blob/master/CHANGELOG.md#v2300-2020-01-07).
GleanMetricsService.start() need to be called after Glean system initialization.