Skip to content
This repository has been archived by the owner on Jul 22, 2024. It is now read-only.

Call Glean initialize prior to set startup metrtics. #3130

Merged
merged 1 commit into from
Apr 7, 2020

Conversation

daoshengmu
Copy link
Contributor

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.

@daoshengmu daoshengmu self-assigned this Apr 7, 2020
@bluemarvin bluemarvin merged commit bce60f9 into master Apr 7, 2020
@bluemarvin bluemarvin deleted the gleanDistChannel branch April 7, 2020 23:49
@daoshengmu
Copy link
Contributor Author

@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);

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?

Copy link
Contributor Author

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.

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Distribution channel name in metrics is null after Glean updates.
3 participants