1715542 - What sort of `Glean` JS global should we have to appease eslint and ergonomics?

Status: RESOLVED FIXED

(Toolkit :: Telemetry, task)

Description

[Chris H-C :chutten]

:Standard8 had a pertinent comment in a review about adding Glean collection in JS. We do have a Glean global, but it's unclear whether the way we have it as a global is ideal.

Reproduced here:

I don't think this [ed: adding Glean to privileged.js] is right.
From what I can tell, Glean is only defined in XPCOM components:

https://searchfox.org/mozilla-central/rev/e8904db16ac45bff0ffe65e7289f8d2f00c48c48/js/xpconnect/src/Sandbox.cpp#1081

BrowserGlue is an XPCOM component, so I guess that is why it is working there.

Other places have to use Cu.importGlobalProperties(["Glean"]); which is why ESLint is failing.

I guess we could create another environment for XPCOM components,
unless Glean is likely to be moved to privileged scope and not need the 
Cu.importGlobalProperties(["Glean"]); that we have in other places.

This bug is for documenting why things are the way they are, and what possibilities we might pursue. A good deliverable out of this would be a page in the FOG developer docs explaining the choice.

We have few enough JS consumers at present we can still change the design if there's one that's better for our consumers. Our JS API Design was quite rigorous, though, so it may also be that we are in the best of possible worlds and so changing the design isn't warranted.

Comment 1

[Chris H-C :chutten]

Is this roughly what you had in mind, :Standard8? If so, could you take a quick gander at bug 1635238 and see if we covered the necessary ground or if we should ni? :smaug or others for a discussion?

Comment 2

[Mark Banner (:standard8)]

The bit I can't see in the docs / other bug, is the reason that Glean is defined for XPCOM components only (via DefineInXPCComponents) and not globally for anywhere privileged.

If there is a specific reason for that and for needing Cu.importGlobalProperties(["Glean"]); in most places but not all, then I'm okay with that.

If we do keep it as it is, this will mean that we'll probably want a specific ESLint set-up to define Glean in XPCOM components scopes only, that might be a bit difficult to do but I think it is possible.

:smaug, any ideas on the reasons for Glean being XPCOM components only?

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

What does "globally" mean here?
Isn't Glean exposed also for Window contexts.

Do you have example of an interface which is exposed in the places you'd want Glean to be exposed?

Comment 4

[Mark Banner (:standard8)]

(In reply to Olli Pettay [:smaug] from comment #3)

What does "globally" mean here?

For any privileged JS context, e.g. modules (jsm), privileged code in browser windows etc.

Isn't Glean exposed also for Window contexts.

I think it is only exposed for XPCOM components. For example if I remove the importGlobalProperties line here then test_backgroundupdate_glean.js fails as Glean isn't defined.

Do you have example of an interface which is exposed in the places you'd want Glean to be exposed?

All the ones in ESLint's privileged.js, e.g. MediaStream.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Glean is in Window context:
https://searchfox.org/mozilla-central/rev/d97dca5907e2a0a77a2435b24ef980268cd3c4fe/dom/webidl/Window.webidl#709

Perhaps we're talking about different things.
There is no Glean interface, there is GleanImpl
https://searchfox.org/mozilla-central/rev/d97dca5907e2a0a77a2435b24ef980268cd3c4fe/dom/chrome-webidl/Glean.webidl#21
which also Window uses.

Comment 6

[Chris H-C :chutten]

I just ran into this as a problem when trying to make a Glean mochitest. They seem to run with their global being window meaning I can't use Cu.importGlobalProperties without getting NS_ERROR_NOT_AVAILABLE or something. I also can't use it without saying anything about it because eslint gives me no-undef error 'Glean' is not defined.

So I end up const Glean = window.Glean; at the top and putting in a dorky comment.

There's gotta be a more ergonomic way to do this.

Comment 7

[Mark Banner (:standard8)]

I think I've got a bit more of an idea of what's going on now and the terminology.

The problem for ESLint is that Glean is exposed on window and to XPCOM components. It is not exposed via the backstage pass to chrome privileged JavaScript - i.e. xpcshell-tests and javascript modules in particular.

Our ESLint configuration isn't currently in a position to handle this. We don't have a way to distinguish XPCOM components vs regular jsms, nor do we have an easy way to distinguish window contexts vs non-window.

Could we expose Glean via the backstage pass to other chrome contexts? That would avoid the need for Cu.importGlobalProperties and would make Glean consistent with what we've done for webidl interfaces where if they are exposed to window they can also be used in System exposed interfaces (xref bug 1489301).

This would make it far easier on ESLint. To be clear, I think we could probably work out a way for ESLint to work, but this is currently a special case so I'm hoping to avoid it.

(In reply to Chris H-C :chutten from comment #6)

So I end up const Glean = window.Glean; at the top and putting in a dorky comment.

You can put in /* global Glean:false */ to make ESLint happy. Though obviously we should try and resolve this.

Comment 8

[Chris H-C :chutten]

I'm happy to expose Glean (and GleanPings) via the backstage pass. I think having it implicitly available is just peachy.

Is that something we should seek permission for, or should I just start researching the "how to" of it?

Comment 9

[Mark Banner (:standard8)]

I don't think there would be any issue with it, though I'm not a expert in the area. Asking Nika or Olli might be a good start and they'll probably be able to tell you where to look to enable it.

Comment 10

[Chris H-C :chutten]

Filp a coin annnnd, Nika!

Consensus here seems to be that maybe we should extend the Glean and GleanPings global reach from just window and XPCOM components to also be exposed via the backstage pass to chrome-privileged JS (xpcshell and ye olde standarde JSMs, specifically). 1) Do you see reasons this'd be a bad idea? 2) How would I go about doing this?

Comment 11

[Nika Layzell [:nika] (ni? for response)]

Clearing needinfo as we've discussed on matrix.

Comment 12

[Chris H-C :chutten]

To summarize: things are tough!

We rather unfortunately can't use webidl namespace like ChromeUtils does. By spec, webidl namespaces don't support the named getters we're using to not instantiate every Glean metric category and ping. Alas.

So what we can do instead is add Glean and GleanPings to globals in the same way we add profiling functions (startProfiling, getMaxGCPauseSinceClear, and friends). Seems to be working out nicely, so I'll be aiming in that direction, removing them from the list of importable globals as I go. (Leaving it on Window)

In other things that came out of the convo, see also bug 1742133 to tidy up nsIFOG. Might do that at the same time since I'm in the mood.

Now to figure out how to back eslint off from complaining about these...

Comment 13

[Mark Banner (:standard8)]

If you're adding the globals everywhere privileged, then we can add them to https://searchfox.org/mozilla-central/source/tools/lint/eslint/eslint-plugin-mozilla/lib/environments/privileged.js

Comment 14

[Chris H-C :chutten]

Attachment: Bug 1715542 - Give Glean and GleanPings a BackstagePass r?nika!,janerik!

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a707741b99f1
Give Glean and GleanPings a BackstagePass r=janerik,application-update-reviewers,nalexander

Comment 16

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/a707741b99f1