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
Merged

Call Glean initialize prior to set startup metrtics. #3130

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions app/src/common/shared/org/mozilla/vrbrowser/telemetry/GleanMetricsService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link

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.

Copy link

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.

Glean.INSTANCE.initialize(aContext, telemetryEnabled, config);

if (telemetryEnabled) {
GleanMetricsService.start();
} else {
GleanMetricsService.stop();
}
Configuration config = new Configuration(Configuration.DEFAULT_TELEMETRY_ENDPOINT, BuildConfig.BUILD_TYPE);
Glean.INSTANCE.initialize(aContext, true, config);
}

// It would be called when users turn on/off the setting of telemetry.
Expand Down