Put the new telemetry submission capability behind a feature

[mhammond]

This is slightly problematic on desktop. There's a good chance a callback interface would work, but that's a little problematic in other ways (as that forces you to use a Box<dyn T> instead of an Arc<>, which is particularly problematic for tests. This seems like a reasonable compromise while we work out a better option.

Pull Request checklist

• Breaking changes: This PR follows our breaking change policy
  • This PR follows the breaking change policy:
    • This PR has no breaking API changes, or
    • There are corresponding PRs for our consumer applications that resolve the breaking changes and have been approved
• Quality: This PR builds and tests run cleanly
  • Note:
    • For changes that need extra cross-platform testing, consider adding [ci full] to the PR title.
    • If this pull request includes a breaking change, consider cutting a new release after merging.
• Tests: This PR includes thorough tests or an explanation of why it does not
• Changelog: This PR includes a changelog entry in CHANGELOG.md or an explanation of why it does not need one
  • Any breaking changes to Swift or Kotlin binding APIs are noted explicitly
• Dependencies: This PR follows our dependency management guidelines
  • Any new dependencies are accompanied by a summary of the due diligence applied in selecting them.

[bendk]

This seems like a good workaround for now until we find a better solution for trait interfaces on desktop.

[leplatrem]

If that helps, let's do it, sure!

Our original goal is to have some client telemetry for when we'll gradually migrate to the v2 API. And currently we only have glean on our desktop client. We would want the A-S client to be able to report sync uptake too, so that we can detect issue if any.

I'm not too familiar with the architecture of the megazords and the implications of disabling the feature by default. But if that's possible, I would suggest enabling the feature by default, and only disabling it where it causes issues.

Also, I chose this pattern because it seems to be a possible way to go, but I'm not familiar enough with FFi and runtime dyn pointers, etc. And if there's a better way to plug Glean into A-S, now would be a good time to redesign it because we haven't landed the iOS and Android parts yet... I believe we could mimic what the Ads or Nimbus client does, no? Would one of you have some time for some hand holding?

Thank you 🙏

[mhammond]

And currently we only have glean on our desktop client.

Ouch - it's only desktop which has this problem. And the problem is the threading model. In a mobile context, the callback fully completes on the thread it is called from. However, on Desktop, these callbacks are called from some thread other than the main thread, but the callback can only be actually called from the main thread as that's the only place JS code can run. We possibly could arrange for xpcom code to block the calling thread and marshal things to the main thread, but that's complicated and not clear it would actually meet expectations. In this particular scenario, it could be "fire and forget" as there's no return value needed, but that's not always the case.

I also think Ads is likely to have the exact same problem once they target desktop. I'm not that familiar with nimbus but AFAIK, that's not used on desktop either. There are some glean changes coming down the line which should help here - but while these specific use-cases are glean, not all of them are. So we need a better story.

But if that's possible, I would suggest enabling the feature by default, and only disabling it where it causes issues.

Ironically, the only place it is causing issues is the only place it is used. The only place where it doesn't cause issues are the places where you haven't landed the bits yet.

I think I'll just land what I have and prioritize removing the feature flag entirely.

[leplatrem]

It makes sense, thank you Mark.