Bug 1638877: Fix schema validation errors. #996
Conversation
| @@ -257,6 +258,7 @@ open class GleanInternalAPI internal constructor () { | |||
| * | |||
| * @param enabled When true, enable metric collection. | |||
| */ | |||
| @Synchronized | |||
There was a problem hiding this comment.
I wonder if the solution here is to call initializeCoreMetrics using Dispatchers.API.launch when called outside of the Glean.initialize. Have you tried that, @mdboom ?
My concern is that making the whole init synchronized, even though off the main thread, might have performance implications. Of course I have no data to back my claim :)
Feel free to ignore this if it feels sketchy, I'm all up for pushing this to release.
There was a problem hiding this comment.
Yeah, you could be right. I was going for a solution I was pretty sure to work, rather than the minimal one. I might spend a little more time trying to write a test to reproduce -- that would give me more confidence that a minimal solution would work.
There was a problem hiding this comment.
I think running it on the API scope should do the same thing, but I'm happy to see how this fares in nightly as I don't see any reason why this approach shouldn't work. I just have a vague feeling that this will cause some unknown synchronized mayhem, but that may be just because I was experiencing threading mayhem myself recently.
|
If this goes well, we should probably update the iOS bindings to match this logic, or at least ensure a similar issue doesn't exist there. |
Yup, we should do that for all the bindings :( |
|
I found a completely different solution to this problem -- it now won't send a dirty startup ping if Glean didn't completely finish (and the START event happens) in the previous run. That shouldn't have the performance implications of the previous synchronization approach. |
This fixes two separate issues that could lead to schema validation failures: 1. Don't send a dirty startup ping if the previous run of the application didn't successfully and completely initialize Glean. 2. Always set values for app_build and app_display_version even if there is an exception fetching them from the OS PackageManager, so that the pings aren't rejected by the schema.
| GleanInternalMetrics.appBuild.setSync("inaccessible") | ||
| GleanInternalMetrics.appDisplayVersion.setSync("inaccessible") |
There was a problem hiding this comment.
nit: would you kindly document these values in the metrics.yaml?
| @@ -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 | |||
This fixes two separate issues that could lead to schema validation failures:
Don't send a dirty startup ping if the previous run of the application didn't
successfully and completely initialize Glean.
Always set values for app_build and app_display_version even if there is
an exception fetching them from the OS PackageManager, so that the pings
aren't rejected by the schema.