issue #787 - ensure that data collection is always enabled in core ad…

[rhelmer]

…d-on when user is enrolled

Checklist for reviewer:

• [x] The description should reference a bug or github issue, if relevant.
• [x] There must be a CHANGELOG.md entry for any non-test change.
• [x] Any change to the NPM commands must be carefully reviewed to make sure it won't break the Add-ons pipeline.
• [x] Any version increase must follow the release process.

[rhelmer]

I noticed this when testing https://github.com/mozilla-rally/rally-core-addon/issues/787 - data collection will not always be enabled for Glean in the core add-on after a restart.

I believe it is because this code uses Promise.finally(), but expecting an argument which according to MDN will not be provided:

A finally callback will not receive any argument, since there's no reliable means of determining if the promise was fulfilled or rejected. This use case is for precisely when you do not care about the rejection reason, or the fulfillment value, and so there's no need to provide it.

I think this is fortunately not a huge deal, yet, because:

1. glean-based studies send their own enrollment ping
2. glean will send deletion pings regardless

I believe that finally() was used here because the intent was to initialize data collection regardless of whether the user was enrolled at the time it was initialized, which I think is the correct thing to do (for important functions like the glean deletion ping mentioned above) so I've preserved that behavior with then and catch.

[rhelmer]

I noticed this when testing #787 - data collection will not always be enabled for Glean in the core add-on after a restart.

I believe it is because this code uses Promise.finally(), but expecting an argument which according to MDN will not be provided:

A finally callback will not receive any argument, since there's no reliable means of determining if the promise was fulfilled or rejected. This use case is for precisely when you do not care about the rejection reason, or the fulfillment value, and so there's no need to provide it.

I think this is fortunately not a huge deal, yet, because:

1. glean-based studies send their own enrollment ping

2. glean will send deletion pings regardless

I believe that finally() was used here because the intent was to initialize data collection regardless of whether the user was enrolled at the time it was initialized, which I think is the correct thing to do (for important functions like the glean deletion ping mentioned above) so I've preserved that behavior with then and catch.

All that said, I think we should take this patch in the very next release, because:

1. we are turning off legacy telemetry in #787
2. it makes it much easier to QA this

[rhelmer]

Thanks! I made a small change to the error message that is logged that makes it clear that data collection is initialized, but that the user is unenrolled. Glean.js should log additional info after that, so it'll be much clearer why this is happening, I hope.