As done in bug 1302681 for events, we need to extend the Scalar Telemetry core to support recording from addons. That means: * Extend the API for registration of new scalars * Extending the snapshotting API if needed * Adding test coverage through: (1) adding a YAML registry like an addon for test builds (2) generating a JSON blob from it (3) registering the blob dynamically (from xpcshell test, no addon needed) (4) recording into the scalars, snapshotting them, checking the results

Before moving to the implementation, we'll take a stab a the design in this bug.

To extend the API, we only need a new function to register the dynamic scalars (see [1] for the events one). We will add a new |registerScalars| function to the nsITelemetry IDL that ingests: - an unique group name for the scalars; - a list of scalar definitions that go into the previously mentioned group [2]; [1] - https://firefox-source-docs.mozilla.org/toolkit/components/telemetry/telemetry/collection/events.html#registerevents [2] - https://firefox-source-docs.mozilla.org/toolkit/components/telemetry/telemetry/collection/scalars.html#the-yaml-definition-file

The design document is available here: https://docs.google.com/document/d/1ppECSQIhnEcu2RPWU0PvWRkCILcC44hSawC5QTYay68/edit#

There's one aspect that needs some investigation regarding the dynamic scalars: letting the children processes know about the scalar definitions. These are needed in order to know the format of each scalar when a child process tries to write to a dynamic scalar. One option would be to send an IPC message from the parent process to each child process every time a new scalar is registered, with the new definition, to have the scalar definition in that process as well. @Chris, @Georg - any other suggestion? Please note that we don't have this problem with the events, as the data in the event IPC message already contains all that's needed, so we simply record [1]. [1] - http://searchfox.org/mozilla-central/rev/51b3d67a5ec1758bd2fe7d7b6e75ad6b6b5da223/toolkit/components/telemetry/TelemetryEvent.cpp#505

I suppose we could make a DynamicScalarAction that takes the group+name strings and send it to the parent each and every time. Not terribly performant, but this is coming from addon code, so...? Or we require that the addons register their scalars in every process they hope to use them in? (Spitballin')

There are two approaches here, both with different trade-offs: 1) Making child processes aware of registered scalars. 2) Sending up unknown scalars to the parent. Both approaches seem to make this a bit more complex than expected. ### 1) Making child processes aware of registered scalars. This requires additional IPC machinery to: - Broadcast newly registered scalars to current child processes. - Make child processes launched in the future aware of them too. ### 2) Sending up unknown scalars to the parent. This requires additional logic for sending. It would also change semantics for how we treat unknown scalars in the child. There are some options here: a) Sending up all scalars as variants. - This could be limited to the JS API. - We're not sure if we can easily send up variants: - nsIVariants are not thread-safe and - we don't see an easy way to convert to mozilla::Variant. b) Sending up unknown scalars as variants. - See variant concerns above. c) Sending up scalars as specific types. - This brings up concerns about keeping coercion working: - E.g. what about recording a numeric 1 into a bool scalar?

(In reply to Chris H-C :chutten from comment #5) > I suppose we could make a DynamicScalarAction that takes the group+name > strings and send it to the parent each and every time. Not terribly > performant, but this is coming from addon code, so...? Thanks Chris! That sort of overlaps with what Georg suggested below (2). Unfortunately, the caveat there is that we have different types of scalars that store different things (uint, bool, nsString). Knowing the scalar type is needed in order to send accumulated measurements to the parent process: http://searchfox.org/mozilla-central/rev/67f38de2443e6b613d874fcf4d2cd1f2fc3d5e97/toolkit/components/telemetry/ipc/TelemetryComms.h#143-173 (In reply to Georg Fritzsche [:gfritzsche] from comment #6) > There are two approaches here, both with different trade-offs: > [..] > Both approaches seem to make this a bit more complex than expected. > > ### 1) Making child processes aware of registered scalars. > This requires additional IPC machinery to: > - Broadcast newly registered scalars to current child processes. > - Make child processes launched in the future aware of them too. I talked this through with Nathan (:nfroydnj) and looks like this may be a better option: the other one seems at odds with the strongly-typed nature of other stuff in Telemetry; moreover, there may be a risk of things not working as expected for the consumers if we go for option (2). IPC overhead shouldn't matter too much as well, since we won't be registering too many addon scalars.

Friendly reminder for Gabor, who already gave me a couple of good pointers [1][2]:-D > 1) Every time somebody wants to register a dynamic scalar on the parent process, I need to send a message to all the other processes with the scalar definition > 2) when a new process spawns, it needs to request the scalars definition from the parent process How can we get the list of running processes (content, extension and gpu)? [1] - http://searchfox.org/mozilla-central/rev/67f38de2443e6b613d874fcf4d2cd1f2fc3d5e97/caps/DomainPolicy.cpp#29 [2] - http://searchfox.org/mozilla-central/rev/67f38de2443e6b613d874fcf4d2cd1f2fc3d5e97/dom/ipc/ContentParent.cpp#716

(In reply to Alessio Placitelli [:Dexter] from comment #8) > Friendly reminder for Gabor, who already gave me a couple of good pointers > [1][2]:-D > > > 1) Every time somebody wants to register a dynamic scalar on the parent process, I need to send a message to all the other processes with the scalar definition I think I would go for the ContentParent::GetAll and maybe on the receiver end you can assert the type of the process or just ignore it for unwanted process types. Probably you will want to have file process as well maybe big alloc? I'd say just store the type and then you can filter on it later. The GPU process might be tricky... I don't think it's a content process, I think you should communicate to it via GPUParent, please ask for guidance from :dvander. > > 2) when a new process spawns, it needs to request the scalars definition from the parent process Instead of a request the usual approach for this is setting the initialProcessData like: [1] and use that in the child process when the process is created. [1]: http://searchfox.org/mozilla-central/rev/44c693914255638d74bcf1ec3b0bcd448dfab2fd/dom/ipc/ContentParent.cpp#2222

As per IRC, Gabor also mentioned another interesting API to solve both (1) and (2): https://wiki.mozilla.org/IPDL/Shmem The caveat there is that only one process at a time can have read/write access to the shmem: we can't have, AFAICT, have child processes have read-only access. I will ping :baku about this API, since he seem to know more.

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #9) > > > 2) when a new process spawns, it needs to request the scalars definition from the parent process > > Instead of a request the usual approach for this is setting the > initialProcessData like: [1] and use that in the child process when the > process is created. > > [1]: > http://searchfox.org/mozilla-central/rev/ > 44c693914255638d74bcf1ec3b0bcd448dfab2fd/dom/ipc/ContentParent.cpp#2222 Turns out using initialProcessData might be a bit tricky, as it's a JS struct. A simpler alternative could be to use the XPCOMInitData: no JSAPI code from c++ (and no JS access to it). This shouldn't be a problem for us, as we need this for core C++ Telemetry. http://searchfox.org/mozilla-central/rev/44c693914255638d74bcf1ec3b0bcd448dfab2fd/dom/ipc/ContentParent.cpp#2167

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #9) > (In reply to Alessio Placitelli [:Dexter] from comment #8) > > > 1) Every time somebody wants to register a dynamic scalar on the parent process, I need to send a message to all the other processes with the scalar definition > > I think I would go for the ContentParent::GetAll and maybe on the receiver > end you can assert the type of the process or just ignore it for unwanted > process types. Probably you will want to have file process as well maybe big > alloc? I'd say just store the type and then you can filter on it later. > > The GPU process might be tricky... I don't think it's a content process, I > think you should communicate to it via GPUParent, please ask for guidance > from :dvander. @dvander, is there any similar facility for the GPU process? > > > 2) when a new process spawns, it needs to request the scalars definition from the parent process @dvander, is there any way to set the initial data for a GPU child process? Something similar to what's pointed out in comment 11.

Clearing the ni?, as we don't really need/want/have addons to recording telemetry in the GPU process :-P (thanks Chris)

Quick recap on the current state: (In reply to Georg Fritzsche [:gfritzsche] from comment #6) > There are two approaches here, both with different trade-offs: > 1) Making child processes aware of registered scalars. > 2) Sending up unknown scalars to the parent. We want for this option. Thanks to the help from Gabor (1) was addressed using the XPCOMInit data structure: it gets passed at startup to every new content process. (2) will be addressed using the suggested "ContentParent::GetAll" API. I've got a sort-of-working patch and I'm in the process of fixing the test fallout.

Comment on attachment 8907518 [details] Bug 1393801 - Enable dynamic scalar registration. f=gfritzsche, Hey Georg, any chance you could take an high level look at this? The API lives in the nsITelemetry.idl file. Documentation update will come in a separate commit/patch.

Comment on attachment 8907518 [details] Bug 1393801 - Enable dynamic scalar registration. f=gfritzsche, https://reviewboard.mozilla.org/r/179224/#review185984 This looks good overall, cheers! There are some things we need to fix and can improve, but i see no substantial problem. ::: dom/ipc/PContent.ipdl:248 (Diff revision 1) > int32_t feature; > int32_t status; > nsCString failureId; > }; > > +struct TelemetryDynamicScalarDefinition Can we move this kind of definition to *TelemetryComms.h* or so? That keeps our Telemetry IPC structures in one central place. ::: dom/ipc/PContent.ipdl:676 (Diff revision 1) > + * IPC message to propagate dynamic scalar definitions, added after the > + * content process is spawned, from the parent to the child. This only mentions process spawning. I assume this needs an update and that we: - Send all the current dynamic scalar definitions when a content process spawns. - Send the newly registered scalar definitions after a `registerScalars()` call. ::: toolkit/components/telemetry/TelemetryScalar.cpp:176 (Diff revision 1) > + > +/** > + * An helper object to get the information for both > + * static and dynamic scalars. > + */ > +class ScalarInfoWrapper It seems we should *either* use `ScalarInfoWrapper` *or* have `DynamicScalarInfo` inherit from `ScalarInfo`. Dynamic & normal scalar infos share a common subset of data, but i think they don't overlap sufficiently to warrant re-using the `ScalarInfo` for dynamic ones. How about doing a `BaseScalarInfo` with some virtual/must-override functions and provide concrete classes for normal and dynamic scalar infos? ::: toolkit/components/telemetry/ipc/TelemetryIPC.h:103 (Diff revision 1) > +/** > + * Set the dynamic scalar definitions coming from the parent process > + * to the current child process. > + * @param aDefs - The array that contains the scalar definitions. > + */ > +void SetDynamicScalarDefinitions(const nsTArray<mozilla::dom::TelemetryDynamicScalarDefinition>& aDefs); Does this `set` all the scalar definitions? Or does it `add` new scalar definitions? I expect the latter, we should name & document this more clearly. ::: toolkit/components/telemetry/ipc/TelemetryIPCAccumulator.h:28 (Diff revision 1) > // Scalar accumulation functions. > -void RecordChildScalarAction(mozilla::Telemetry::ScalarID aId, > +void RecordChildScalarAction(uint32_t aId, bool aDynamic, > mozilla::Telemetry::ScalarActionType aAction, > const mozilla::Telemetry::ScalarVariant& aValue); I like how small the impact is here! This means the scalar accumulation code didn't change significantly and most changes really affect dynamic registration and dynamic scalar recording. ::: toolkit/components/telemetry/nsITelemetry.idl:500 (Diff revision 1) > + * @param aScalarData An object that contains registration data for multiple scalars in the form: > + * { > + * "sample_scalar": { > + * "kind": Ci.nsITelemetry.SCALAR_TYPE_COUNT, > + * "keyed": true, //optional, defaults to false > + * "release_channel_collection: Ci.nsITelemetry.DATASET_RELEASE_CHANNEL_OPTOUT, // optional We changed this semantic, this should be `record_on_release: true/false`. See the event registration mechanics.

Comment on attachment 8907518 [details] Bug 1393801 - Enable dynamic scalar registration. f=gfritzsche, https://reviewboard.mozilla.org/r/179224/#review185984 > This only mentions process spawning. > I assume this needs an update and that we: > - Send all the current dynamic scalar definitions when a content process spawns. > - Send the newly registered scalar definitions after a `registerScalars()` call. Looking at that again, this is just being called when definitions are broadcasted after the process is spawned. I've added a note in the comment to clarify. > It seems we should *either* use `ScalarInfoWrapper` *or* have `DynamicScalarInfo` inherit from `ScalarInfo`. > > Dynamic & normal scalar infos share a common subset of data, but i think they don't overlap sufficiently to warrant re-using the `ScalarInfo` for dynamic ones. > > How about doing a `BaseScalarInfo` with some virtual/must-override functions and provide concrete classes for normal and dynamic scalar infos? Good point! I followed your suggestion and provided a base class for both, getting rid of the wrapper. > I like how small the impact is here! > This means the scalar accumulation code didn't change significantly and most changes really affect dynamic registration and dynamic scalar recording. \o/ We did a good job back then ;-)

@billm, would you kindly review the attached patch and have a look specifically a the changes in dom/ipc/* and the implementation of the internal_BroadcastDefinitions function in TelemetryScalar.cpp? The context here is that we're adding a new scalar mechanism to enable addons to register new scalars. We need to let all the processes know about the new (dynamic) scalar definitions. For more context, see comment 3. @chutten, this is the mammoth patch I talked about in the last meeting. Let me guide you through the implementation: - dom/ipc/* files are the implementing the IPC mechanism (1) described in comment 6; - TelemetryComms.h defines the DynamicScalarDefinition structure that's used to pass definitions around the IPC calls; - TelemetryEvent*/TelemetryCommon* changes are due to moving the IsValidIdentifierString/IsCharValid functions out of TelemetryEvent to the TelemetryCommon module, since we're using them for the scalars implementation as well; - ScalarInfo.h defines a new base class common to both the dynamic scalar definitions and the "static" (Scalars.yaml) ones; - TelemetryIPC* contains boilerplate code to wrap TelemetryScalar* calls (for calling from dom/ipc/*); - TelemetryScalar.cpp - here's the hardcore part: * It defines DynamicScalarInfo (derived from BaseScalarInfo) to hold the dynamic scalar definitions; * Implements internal_RegisterScalars/RegisterScalars to perform data validation and the insertion of the definitions; * Introduces ScalarKey, an helper struct used to identify both static and dynamic scalars (it's a uint32_t+bool combo). Most of the changes in this file are related to this, since we no longer rely on the ScalarID enum alone but prefer using ScalarKey. - The changes to the other files are mostly due to the use of ScalarKey.

Comment on attachment 8907518 [details] Bug 1393801 - Enable dynamic scalar registration. f=gfritzsche, https://reviewboard.mozilla.org/r/179224/#review187538 Mostly small things, some pondering about bikeshed colours. Some deeper conversations about docs, what we should permit, and a couple of missing test cases. ::: toolkit/components/telemetry/ScalarInfo.h:36 (Diff revision 2) > + , dataset(aDataset) > + , record_in_processes(aRecordInProcess) > + , keyed(aKeyed) > + {} > + > + // The following functions will read the stored text will? Or may? I thought for static scalars, nothing had changed (except for a vtable dispatch), and it would continue to pick it out from the table. ::: toolkit/components/telemetry/TelemetryEvent.cpp:960 (Diff revision 2) > nsresult > TelemetryEvent::RegisterEvents(const nsACString& aCategory, > JS::Handle<JS::Value> aEventData, > JSContext* cx) > { > - if (!IsValidIdentifierString(aCategory, 30, true)) { > + if (!IsValidIdentifierString(aCategory, 30, true, false)) { Ugh, inline magic numbers. (Nothing to do with your patch. Just Ugh.) ::: toolkit/components/telemetry/TelemetryScalar.cpp:120 (Diff revision 2) > StringTooLong, > // Unsigned Scalar Errors > UnsignedNegativeValue, > - UnsignedTruncatedValue > + UnsignedTruncatedValue, > + // Dynamic Scalar Errors > + AlreadyRegistered Add a defensive comma? ::: toolkit/components/telemetry/TelemetryScalar.cpp:178 (Diff revision 2) > > +// Dynamic scalar definitions. > +StaticAutoPtr<nsTArray<DynamicScalarInfo>> gDynamicScalarInfo; > + > +const BaseScalarInfo& > +internal_GetScalarInfo(const ScalarKey& aKey) aId ::: toolkit/components/telemetry/TelemetryScalar.cpp:196 (Diff revision 2) > } > > +// TODO: We should explicitly pass a lock here. Unfortunately this needs > +// some refactoring of the callers as well. See bug 1362957. > +bool > +internal_IsValidId(const ScalarKey& aKey) aId ::: toolkit/components/telemetry/TelemetryScalar.cpp:1012 (Diff revision 2) > } > > /** > * Check if we are allowed to record the provided scalar. > * > - * @param aId The scalar id. > + * @param aKey The scalar identifier. aId ::: toolkit/components/telemetry/TelemetryScalar.cpp:1049 (Diff revision 2) > * NS_ERROR_FAILURE if this was called before init is completed. > * NS_ERROR_INVALID_ARG if the name can't be found in the scalar definitions. > * NS_OK if the scalar was found and aId contains a valid enum id. > */ > nsresult > -internal_GetEnumByScalarName(const nsACString& aName, mozilla::Telemetry::ScalarID* aId) > +internal_GetEnumByScalarName(const nsACString& aName, ScalarKey* aKey) aId ::: toolkit/components/telemetry/TelemetryScalar.cpp:1081 (Diff revision 2) > * NS_ERROR_NOT_AVAILABLE if the scalar is expired. > * NS_OK if the scalar was found. If that's the case, aResult contains a > * valid pointer to a scalar type. > */ > nsresult > -internal_GetScalarByEnum(mozilla::Telemetry::ScalarID aId, > +internal_GetScalarByEnum(const ScalarKey& aKey, Hrm. aKey or aId... aId makes it less likely to impact interactions with keyed scalars, but the type is specifically ScalarKey. The type name itself might be confusing, using a ScalarKey in a non-keyed Scalar... ...Hrm. Well, I don't have a better typename than ScalarKey, but I think we should call it aId universally. ::: toolkit/components/telemetry/TelemetryScalar.cpp:1346 (Diff revision 2) > +/** > + * Helper function to convert an array of |DynamicScalarInfo| > + * to |DynamicScalarDefinition| used by the IPC calls. > + */ > +void > +internal_DynamicScalarToIPC(const nsTArray<DynamicScalarInfo>& aDynamicScalarsDefs, aDynamicScalarInfos? ::: toolkit/components/telemetry/TelemetryScalar.cpp:1349 (Diff revision 2) > + */ > +void > +internal_DynamicScalarToIPC(const nsTArray<DynamicScalarInfo>& aDynamicScalarsDefs, > + nsTArray<DynamicScalarDefinition>& aIPCDefs) > +{ > + for (auto iter = aDynamicScalarsDefs.begin(); I'm going to pretend to be :gfritzsche here and ask if you can use c++ foreach-style `for (auto info : aDynamicScalarInfos) {` instead of iterating. ::: toolkit/components/telemetry/TelemetryScalar.cpp:1351 (Diff revision 2) > +internal_DynamicScalarToIPC(const nsTArray<DynamicScalarInfo>& aDynamicScalarsDefs, > + nsTArray<DynamicScalarDefinition>& aIPCDefs) > +{ > + for (auto iter = aDynamicScalarsDefs.begin(); > + iter != aDynamicScalarsDefs.end(); iter++) { > + DynamicScalarDefinition stubDefinition; Should we have a DynamicScalarDefinition ctor that just takes a DynamicScalarInfo&? ::: toolkit/components/telemetry/TelemetryScalar.cpp:1369 (Diff revision 2) > + */ > +void > +internal_BroadcastDefinitions(const nsTArray<DynamicScalarInfo>& scalarInfos) > +{ > + nsTArray<mozilla::dom::ContentParent*> parents; > + mozilla::dom::ContentParent::GetAll(parents); WE CAN HAVE MULTIPLE CONTENTPARENTS?! ::: toolkit/components/telemetry/TelemetryScalar.cpp:1379 (Diff revision 2) > + // Convert the internal scalar representation to a stripped down IPC one. > + nsTArray<DynamicScalarDefinition> ipcDefinitions; > + internal_DynamicScalarToIPC(scalarInfos, ipcDefinitions); > + > + // Broadcast the definitions to the other content processes. > + for (uint32_t i = 0; i < parents.Length(); i++) { again imagine :gfritzsche asking if we can use new loops here. ::: toolkit/components/telemetry/TelemetryScalar.cpp:1400 (Diff revision 2) > + // Register the new scalars. > + if (!gDynamicScalarInfo) { > + gDynamicScalarInfo = new nsTArray<DynamicScalarInfo>(); > + } > + > + for (uint32_t i = 0, len = scalarInfos.Length(); i < len; ++i) { You use the fancy loop style above... ::: toolkit/components/telemetry/TelemetryScalar.cpp:2286 (Diff revision 2) > + JSContext* cx) > +{ > + MOZ_ASSERT(XRE_IsParentProcess(), > + "Dynamic scalars should only be created in the parent process."); > + > + if (!IsValidIdentifierString(aGroupName, 40, true, false)) { Magic number... :( ::: toolkit/components/telemetry/TelemetryScalar.cpp:2303 (Diff revision 2) > + JS::Rooted<JS::IdVector> scalarPropertyIds(cx, JS::IdVector(cx)); > + if (!JS_Enumerate(cx, obj, &scalarPropertyIds)) { > + return NS_ERROR_FAILURE; > + } > + > + // Collect the event data into local storage first. event data? ::: toolkit/components/telemetry/TelemetryScalar.cpp:2314 (Diff revision 2) > + nsAutoJSString scalarName; > + if (!scalarName.init(cx, scalarPropertyIds[i])) { > + return NS_ERROR_FAILURE; > + } > + > + if (!IsValidIdentifierString(NS_ConvertUTF16toUTF8(scalarName), 40, false, true)) { Another magic number ::: toolkit/components/telemetry/TelemetryScalar.cpp:2708 (Diff revision 2) > + MOZ_ASSERT(!XRE_IsParentProcess()); > + > + nsTArray<DynamicScalarInfo> dynamicStubs; > + > + // Populate the definitions array before acquiring the lock. > + for (auto iter = aDefs.begin(); fancy for loop? ::: toolkit/components/telemetry/TelemetrySession.jsm:1255 (Diff revision 2) > // Parent histograms are added to the top-level payload object instead of the process payload. > if (processType == "parent" && (key == "histograms" || key == "keyedHistograms")) { > payloadLoc = payloadObj; > } > - // Dynamic processes only collect events. > - if (processType == "dynamic" && key != "events") { > + // The Dynamic process only collects events and scalars. > + if (processType == "dynamic" && !["events", "scalrs"].includes(key)) { scalrs? This typo suggests we need your test to check the payload as well as just the snapshots. ::: toolkit/components/telemetry/docs/collection/scalars.rst:44 (Diff revision 2) > + > +.. code-block:: js > + > + Services.telemetry.registerScalars(group, scalarData); > + > +Register new scalars from add-ons. I suppose there's nothing stopping non-addons code from registering dynamic scalars, is there... Do we care if a scalar is dynamic or static? The aggregation behaviour is different, but our storage is roughly as performant (and transmission, too). Data review's a little easier to ensure for static scalars since we monitor Scalars.yaml... I dunno. Do we care if non-addon code registers dynamic scalars? ::: toolkit/components/telemetry/docs/collection/scalars.rst:46 (Diff revision 2) > + > + Services.telemetry.registerScalars(group, scalarData); > + > +Register new scalars from add-ons. > + > +* ``group`` - *(required, string)* The unique group the scalars are registered in. Should mention that the group name has some restrictions (length, characters). ::: toolkit/components/telemetry/docs/collection/scalars.rst:47 (Diff revision 2) > + Services.telemetry.registerScalars(group, scalarData); > + > +Register new scalars from add-ons. > + > +* ``group`` - *(required, string)* The unique group the scalars are registered in. > +* ``scalarData`` - *(required, object)* An object of the form ``{scalarName1: scalar1Data, ...}`` that contains registration data for multiple scalars, where each scalar is an object with the following properties: Should mention that `scalarNameN` are required strings that must meet the scalar name requirements. ::: toolkit/components/telemetry/docs/collection/scalars.rst:56 (Diff revision 2) > + * ``record_on_release`` - *(optional, bool)* Whether to record this data on release. Defaults to false. > + * ``expired`` - *(optional, bool)* Whether this scalar entry is expired. This allows recording it without error, but it will be discarded. Defaults to false. > + > +For scalars recorded from add-ons, registration happens at runtime. Any new scalar must first be registered through this function before they can be recorded. > + > +After registration, the scalars can be recorded through the usual scalar JS API. They will be submitted in the main pings payload under ``processes.dynamic.scalars``. Should mention the unique multi-process characteristics? Doesn't work on non-content child processes. Aggregates all accumulations (parent and content children) together instead of keeping them separate. ::: toolkit/components/telemetry/docs/collection/scalars.rst:58 (Diff revision 2) > + > +For scalars recorded from add-ons, registration happens at runtime. Any new scalar must first be registered through this function before they can be recorded. > + > +After registration, the scalars can be recorded through the usual scalar JS API. They will be submitted in the main pings payload under ``processes.dynamic.scalars``. > + > +New scalars registered here are subject to the same limitations as the ones registered through ``Scalars.yaml``. Such as? (I'd like a 'for example' here) ::: toolkit/components/telemetry/docs/collection/scalars.rst:72 (Diff revision 2) > + keyed: false, > + record_on_release: false > + }, > + }); > + // Now scalars can be recorded. > + Services.telemetry.scalarSet("myAddon.group.counter_scalar", 37); How's the timing of when a scalar registered on the parent (as all must be) is accumulatable on the child? Is it immediate? Are there best practices for communicating with the content scripts that might want to accumulate to a scalar that they don't know if it's been registered or not? ::: toolkit/components/telemetry/docs/collection/scalars.rst:281 (Diff revision 2) > =============== > > - Firefox 50: Initial scalar support (`bug 1276195 <https://bugzilla.mozilla.org/show_bug.cgi?id=1276195>`_). > - Firefox 51: Added keyed scalars (`bug 1277806 <https://bugzilla.mozilla.org/show_bug.cgi?id=1277806>`_). > - Firefox 53: Added child process scalars (`bug 1278556 <https://bugzilla.mozilla.org/show_bug.cgi?id=1278556>`_). > +- Firefox 58: Added support for recording new probes from add-ons (`bug 1393801 <bug https://bugzilla.mozilla.org/show_bug.cgi?id=1393801>`_). s/probes/scalars/ ::: toolkit/components/telemetry/nsITelemetry.idl:490 (Diff revision 2) > */ > [implicit_jscontext] > void registerEvents(in ACString aCategory, in jsval aEventData); > > /** > + * Register new scalars to record them from addons. This allows registering multiple Should mention this is parent-process-only. ::: toolkit/components/telemetry/nsITelemetry.idl:512 (Diff revision 2) > + * @param aScalarData.<name>.record_on_release Optional, whether to record this data on release. > + * Defaults to false. > + * @param aScalarData.<name>.expired Optional, whether this scalar entry is expired. This allows > + * recording it without error, but it will be discarded. Defaults to false. > + */ > + [implicit_jscontext] [chromeonly]? Isn't there a way to expose this only to parent-process JS? ::: toolkit/components/telemetry/tests/browser/browser_DynamicScalars.js:45 (Diff revision 2) > + async function(browser) { > + // Make sure our new browser is in its own process. The processCreated > + // promise should have already resolved by this point. > + await processCreated; > + let newPid = browser.frameLoader.tabParent.osPid; > + ok(currentPid != newPid, "The new tab must spawn its own process"); Test will fail if running in !e10s (what does forceNewProcess even do when you pass --disable-e10s?). Should we specify e10s-only in the test manifest? ::: toolkit/components/telemetry/tests/unit/test_TelemetryScalars.js:691 (Diff revision 2) > + "Dynamic expired scalars must not be recorded."); > + Assert.ok("telemetry.test.dynamic.record_optin" in scalars, > + "Dynamic opt-in scalars must be recorded."); > + Assert.equal(scalars["telemetry.test.dynamic.record_optin"], true, > + "The recorded scalar must contain the right value."); > +}); Might I also ask for a test that checks that scalars added to from parent and child are both aggregated together into the same value in the dynamic process? on_parent() { add(1); } on_child() { add(3); } should (eventually) result in a scalar of 4.

Comment on attachment 8907518 [details] Bug 1393801 - Enable dynamic scalar registration. f=gfritzsche, https://reviewboard.mozilla.org/r/179224/#review187710 The dom/ipc stuff seems fine to me. ::: dom/ipc/ContentChild.h:633 (Diff revision 2) > virtual mozilla::ipc::IPCResult > + RecvAddDynamicScalars(nsTArray<DynamicScalarDefinition>&& aDefs) override; This is in the middle of two related things. Please move it down a bit.

Comment on attachment 8907518 [details] Bug 1393801 - Enable dynamic scalar registration. f=gfritzsche, https://reviewboard.mozilla.org/r/179224/#review187538 > will? Or may? > > I thought for static scalars, nothing had changed (except for a vtable dispatch), and it would continue to pick it out from the table. Yes, that's a misplaced comment due to a copy/paste :( Dropping it. > Hrm. aKey or aId... aId makes it less likely to impact interactions with keyed scalars, but the type is specifically ScalarKey. > > The type name itself might be confusing, using a ScalarKey in a non-keyed Scalar... > > ...Hrm. > > Well, I don't have a better typename than ScalarKey, but I think we should call it aId universally. Yeah, I agree with you on this. I wasn't able to find a better name for the type, but let me use aId instead of aKey everywhere. > Should we have a DynamicScalarDefinition ctor that just takes a DynamicScalarInfo&? This would require us to move `DynamicScalarDefinition` out of TelemetryComms.h or move `DynamicScalarDefinition` to ScalarInfo.h (and then include it in TelemetryComms.h). I'd like to keep `DynamicScalarDefinition` in TelemetryScalar.cpp since it's mostly an internal definition. Can you see any way around this or have any suggestion? > scalrs? > > This typo suggests we need your test to check the payload as well as just the snapshots. Added to the browser_DynamicScalars.js file. > I suppose there's nothing stopping non-addons code from registering dynamic scalars, is there... > > Do we care if a scalar is dynamic or static? The aggregation behaviour is different, but our storage is roughly as performant (and transmission, too). Data review's a little easier to ensure for static scalars since we monitor Scalars.yaml... > > I dunno. Do we care if non-addon code registers dynamic scalars? I believe we still want non-addons to use the "static" scalar route, since the expiration mechanism is different and better defined: you can say that a static scalar expires in version X while you can't say the same for dynamic ones, for which new registration code needs to land to set the appropriate "expired" flag. > Should mention that the group name has some restrictions (length, characters). I've added a link to the section that discusses the limits, since they're the same between static and dynamic scalars. > Such as? (I'd like a 'for example' here) Added a couple of examples and a reference to the limitations section. > How's the timing of when a scalar registered on the parent (as all must be) is accumulatable on the child? Is it immediate? Are there best practices for communicating with the content scripts that might want to accumulate to a scalar that they don't know if it's been registered or not? Good point. That's bounded to the time it takes to send an IPC message from the parent to the children processes, so I suspect this is not an issue unless we high high congestion on the IPC channel. If a content script tries to accumulate and the definition didn't make it yet there, the action is simply dropped. With that said, I'm not sure if this can be an issue. > [chromeonly]? Isn't there a way to expose this only to parent-process JS? Not that I know of, since we're using IDL for Telemetry :( There's a way to do this using [WebIDL](https://developer.mozilla.org/it/docs/Mozilla/WebIDL_bindings#ChromeOnly).

Comment on attachment 8907518 [details] Bug 1393801 - Enable dynamic scalar registration. f=gfritzsche, https://reviewboard.mozilla.org/r/179224/#review187960 r+ pending a nit, and pending your other update to the docs about timing. ::: toolkit/components/telemetry/docs/collection/scalars.rst:60 (Diff revisions 2 - 3) > > After registration, the scalars can be recorded through the usual scalar JS API. They will be submitted in the main pings payload under ``processes.dynamic.scalars``. > > -New scalars registered here are subject to the same limitations as the ones registered through ``Scalars.yaml``. > +.. note:: > + > + Accumulating in dynamic scalars only works in content child processes. All the accumulations (parent and content chldren) are aggregated together . And in the parent process, too

Comment on attachment 8907518 [details] Bug 1393801 - Enable dynamic scalar registration. f=gfritzsche, https://reviewboard.mozilla.org/r/179224/#review187538 > This would require us to move `DynamicScalarDefinition` out of TelemetryComms.h or move `DynamicScalarDefinition` to ScalarInfo.h (and then include it in TelemetryComms.h). I'd like to keep `DynamicScalarDefinition` in TelemetryScalar.cpp since it's mostly an internal definition. Can you see any way around this or have any suggestion? You're right, it's not worth it. > I believe we still want non-addons to use the "static" scalar route, since the expiration mechanism is different and better defined: you can say that a static scalar expires in version X while you can't say the same for dynamic ones, for which new registration code needs to land to set the appropriate "expired" flag. Fair.

Pushed by alessio.placitelli@gmail.com: https://hg.mozilla.org/integration/autoland/rev/5f8037cd838f Enable dynamic scalar registration. f=gfritzsche,r=billm,chutten