Bug 1689547 - Create plugin / events infrastructure and define afterPingCollection event
#94
Conversation
| readonly serverEndpoint?: string, | ||
| // Debug configuration. | ||
| debug?: DebugOptions, | ||
| // Optional list of plugins to instrument the current Glean instance. |
There was a problem hiding this comment.
| // Optional list of plugins to instrument the current Glean instance. | |
| // Optional list of plugins to enable in the current Glean instance. |
|
|
||
| import { DEFAULT_TELEMETRY_ENDPOINT } from "./constants"; | ||
| import GleanPlugin from "../plugins"; | ||
| import GleanEvents from "./events"; |
There was a problem hiding this comment.
They are types, we pass plugins through the ConfigurationInterface to Glean.initialize, and this is here to help define exactly what we are passing through that property.
There was a problem hiding this comment.
GleanEvents is a union of all the possible GleanEvents types (not a union right now since we only defined one event, but you get the gist). And GleanPlugin is the type of the accepted plugin class. Each plugin is always attached to a type of event. To ensure we only accept plugins that listen to valid Glean events, the final type for the plugins property is GleanPlugin<GleanEvents>[].
|
|
||
| import { GleanEvent } from "../plugins/index"; | ||
|
|
||
| export const AfterPingCollection: GleanEvent<[PingPayload], Promise<JSONObject>> = { |
There was a problem hiding this comment.
Can you document what's this? When is this called? What's it for?
There was a problem hiding this comment.
So, this is here to define:
- The type of the context provided by this event (
[PingPayload]); - The type of the result expected from this event (
[Promise<JSONObject>]); - The name of this event.
This way, when we define a plugin like this class MyPlugin extends GleanPlugin<typeof AfterPingCollection> the action in this plugin will already expected the correct arguments and return type defined by the event.
| for (const plugin of plugins) { | ||
| if (plugin.event in Glean.plugins) { | ||
| console.error( | ||
| `Attempted to register plugin '${plugin.name}', which instruments the event '${plugin.event}'.`, |
There was a problem hiding this comment.
Mh, what do you mean by "instrument"? We usually use this term to mean "add something to measure".
There was a problem hiding this comment.
Hum, maybe I am using this word wrong... checks wikipedia ... 100% I am using it wrong 🆘 Maybe I want "listens to"?
There was a problem hiding this comment.
Thankfully I can always claim that English is not my first language 🥇
| console.info(JSON.stringify(collectedPayload, null, 2)); | ||
| } | ||
|
|
||
| const modifiedPayload = await Glean.triggerEvent(AfterPingCollection, collectedPayload); |
There was a problem hiding this comment.
We should consider (not in this PR) a different way to inject the dependency here, reducing the calls to the Glean object.
There was a problem hiding this comment.
I considered making events completely unaware of the Glean singleton, but didn't try that out. I do think if we are going to go that way it should be now, otherwise we will be a bit stuck.
Let me attempt that and open an alternate to this PR with the different approach. I did not go that way before, because it would be quite different from what I defined in the proposal for this, but there is no harm in testing.
| const modifiedPayload = await Glean.triggerEvent(AfterPingCollection, collectedPayload); | ||
| let payload: JSONObject; | ||
| if (modifiedPayload) { | ||
| payload = modifiedPayload; | ||
| } else { | ||
| payload = collectedPayload; |
There was a problem hiding this comment.
| const modifiedPayload = await Glean.triggerEvent(AfterPingCollection, collectedPayload); | |
| let payload: JSONObject; | |
| if (modifiedPayload) { | |
| payload = modifiedPayload; | |
| } else { | |
| payload = collectedPayload; | |
| const modifiedPayload = await Glean.triggerEvent(AfterPingCollection, collectedPayload) ?? collectedPayload; |
What about this?
| } | ||
| } | ||
|
|
||
| await Glean.testUninitialize(); |
There was a problem hiding this comment.
I wish we could just test this in isolation. We might want to reconsider how we propagate our dependencies within Glean.js :(
|
Closing in favour of #95 |
2 participants
No description provided.