Skip to content
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.

Sign up for GitHub

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 1638877: Fix schema validation errors. #996

Merged
merged 5 commits into from
Jun 23, 2020
Merged

Bug 1638877: Fix schema validation errors. #996

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2f937fb
Bug 1638877: Fix schema validation
mdboom Jun 23, 2020
4915547
Move to Android section
mdboom Jun 23, 2020
8405a9b
Add docs about "inaccessible"
mdboom Jun 23, 2020
fbe015b
Add more docs
mdboom Jun 23, 2020
d9f3056
Improve docs
mdboom Jun 23, 2020
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
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

* General
* Add rate limiting capabilities to the upload manager. ([1543612](https://bugzilla.mozilla.org/show_bug.cgi?id=1543612))
* Android
* BUGFIX: baseline pings with reason "dirty startup" are no longer sent if Glean did not full initialize in the previous run.

[Full changelog](https://github.com/mozilla/glean/compare/v31.1.2...main)

Expand Down
4 changes: 2 additions & 2 deletions docs/user/pings/index.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,9 +42,9 @@ Optional fields are marked accordingly.

| Field name | Type | Description |
|---|---|---|
| `app_build` | String | The build identifier generated by the CI system (e.g. "1234/A") |
| `app_build` | String | The build identifier generated by the CI system (e.g. "1234/A"). In the unlikely event this value can not be obtained from the OS, it is set to "inaccessible". |
| `app_channel` | String | *Optional* The product-provided release channel (e.g. "beta") |
| `app_display_version` | String | The user-visible version string (e.g. "1.0.3") |
| `app_display_version` | String | The user-visible version string (e.g. "1.0.3"). In the unlikely event this value can not be obtained from the OS, it is set to "inaccessible". If it is accessible, but not set by the application, it is set to "Unknown". |
| `architecture` | String | The architecture of the device (e.g. "arm", "x86") |
| `client_id` | UUID | *Optional* A UUID identifying a profile and allowing user-oriented correlation of data |
| `device_manufacturer` | String | *Optional* The manufacturer of the device |
Expand Down
37 changes: 25 additions & 12 deletions glean-core/android/src/main/java/mozilla/telemetry/glean/Glean.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ package mozilla.telemetry.glean
import android.app.ActivityManager
import android.util.Log
import android.content.Context
import android.content.pm.PackageInfo
import android.content.pm.PackageManager
import android.os.Build
import android.os.Process
Expand Down Expand Up @@ -164,6 +165,14 @@ open class GleanInternalAPI internal constructor () {
return@executeTask
}

// Get the current value of the dirty flag so we know whether to
// send a dirty startup baseline ping below. Immediately set it to
// `false` so that dirty startup pings won't be sent if Glean
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice trick!

// initialization does not complete successfully. It is set to `true`
// again in the ON_START lifecycle event.
val isDirtyFlagSet = LibGleanFFI.INSTANCE.glean_is_dirty_flag_set().toBoolean()
LibGleanFFI.INSTANCE.glean_set_dirty_flag(false.toByte())

// If any pings were registered before initializing, do so now.
// We're not clearing this queue in case Glean is reset by tests.
synchronized(this@GleanInternalAPI) {
Expand Down Expand Up @@ -197,11 +206,8 @@ open class GleanInternalAPI internal constructor () {
// Check if the "dirty flag" is set. That means the product was probably
// force-closed. If that's the case, submit a 'baseline' ping with the
// reason "dirty_startup". We only do that from the second run.
if (!isFirstRun && LibGleanFFI.INSTANCE.glean_is_dirty_flag_set().toBoolean()) {
if (!isFirstRun && isDirtyFlagSet) {
submitPingByNameSync("baseline", "dirty_startup")
// Note: while in theory we should set the "dirty flag" to true
// here, in practice it's not needed: if it hits this branch, it
// means the value was `true` and nothing needs to be done.
}

// From the second time we run, after all startup pings are generated,
Expand Down Expand Up @@ -444,23 +450,30 @@ open class GleanInternalAPI internal constructor () {
GleanInternalMetrics.appChannel.setSync(it)
}

val packageInfo: PackageInfo

try {
val packageInfo = applicationContext.packageManager.getPackageInfo(
packageInfo = applicationContext.packageManager.getPackageInfo(
applicationContext.packageName, 0
)
@Suppress("DEPRECATION")
GleanInternalMetrics.appBuild.setSync(packageInfo.versionCode.toString())

GleanInternalMetrics.appDisplayVersion.setSync(
packageInfo.versionName?.let { it } ?: "Unknown"
)
} catch (e: PackageManager.NameNotFoundException) {
Log.e(
LOG_TAG,
"Could not get own package info, unable to report build id and display version"
)
throw AssertionError("Could not get own package info, aborting init")

GleanInternalMetrics.appBuild.setSync("inaccessible")
GleanInternalMetrics.appDisplayVersion.setSync("inaccessible")
Comment on lines +465 to +466
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: would you kindly document these values in the metrics.yaml?


return
}

@Suppress("DEPRECATION")
GleanInternalMetrics.appBuild.setSync(packageInfo.versionCode.toString())

GleanInternalMetrics.appDisplayVersion.setSync(
packageInfo.versionName?.let { it } ?: "Unknown"
)
}

/**
Expand Down
12 changes: 12 additions & 0 deletions glean-core/android/src/test/java/mozilla/telemetry/glean/GleanTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -798,4 +798,16 @@ class GleanTest {
Glean.pingTypeQueue.add(it)
}
}

@Test
fun `test dirty flag is reset to false`() {
// Set the dirty flag.
LibGleanFFI.INSTANCE.glean_set_dirty_flag(true.toByte())

resetGlean(context, Glean.configuration.copy(
logPings = true
), false)

assertFalse(LibGleanFFI.INSTANCE.glean_is_dirty_flag_set().toBoolean())
}
}
9 changes: 9 additions & 0 deletions glean-core/metrics.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,9 @@ glean.internal.metrics:
- glean_client_info
description: |
The build identifier generated by the CI system (e.g. "1234/A").

In the unlikely event that the build identifier can not be retrieved from
the OS, it is set to "inaccessible".
bugs:
- https://bugzilla.mozilla.org/1508305
data_reviews:
Expand All @@ -164,6 +167,12 @@ glean.internal.metrics:
- glean_client_info
description: |
The user visible version string (e.g. "1.0.3").

In the unlikely event that the build identifier can not be retrieved from
the OS, it is set to "inaccessible".

If it is retrievable but no value has been set by the application, it is
set to "Unknown".
bugs:
- https://bugzilla.mozilla.org/1508305
data_reviews:
Expand Down