Closed Bug 1452552 Opened 7 years ago Closed 7 years ago

Add "products" properties to measurements

Categories

(Toolkit :: Telemetry, enhancement, P1)

Product:
Toolkit
Component:
Telemetry
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox61 --- wontfix
firefox62 --- fixed

People

(Reporter: Dexter, Assigned: Dexter)

References

Details

Attachments

(8 files, 9 obsolete files)

Bug 1452552 - Add "products" property to scalars.
59 bytes, text/x-review-board-request
Dexter
: review+
Details
Bug 1452552 - Add test coverage for product-dependent scalars.
59 bytes, text/x-review-board-request
Dexter
: review+
Details
Bug 1452552 - Add "products" property to events.
59 bytes, text/x-review-board-request
Dexter
: review+
Details
Bug 1452552 - Add test coverage for product-dependent events.
59 bytes, text/x-review-board-request
Dexter
: review+
Details
Bug 1452552 - Add "products" property to histograms.
59 bytes, text/x-review-board-request
Dexter
: review+
Details
Bug 1452552 - Add test coverage for product-dependent histograms.
59 bytes, text/x-review-board-request
Dexter
: review+
Details
Bug 1452552 - Add a method to reset the product during runtime on Android.
59 bytes, text/x-review-board-request
Dexter
: review+
Details
Bug 1452552 - Test geckoview-only scalar probe in geckoview-like mode.
59 bytes, text/x-review-board-request
Dexter
: review+
Details
We need to add a "platform" property to all the collection primitives supported by GeckoView (histograms, scalars, events), as described in [1], to act as a whitelist for probes. In particular: - The property will contain an array of supported platforms for each probe (i.e. "geckoview", "firefox"); - When the property is missing, the collection defaults to "firefox" (i.e. desktop only). - When accumulating, Telemetry checks if the current platform is supported. If not, no error is thrown and the accumulated value is simply discarded. [1] - https://docs.google.com/document/d/1k0VWVLk2Ao_MMoPgCDXyZR2pLj74SvHgSmo5yuUBJCo/edit?ts=5ac63db9#heading=h.7shvseu9mo2p
Blocks: 1452550
Priority: -- → P2
Blocks: 1455319
Let's make sure nothing breaks upstream before landing this. We might need to check: - https://github.com/mozilla/python_moztelemetry/tree/master/moztelemetry - the dataset generation jobs (Longitudinal/Main summary?) - probe scraper (there's bug 1455319 for that) Frank, does this sound right? Am I missing something?
Flags: needinfo?(fbertsch)
Assignee: nobody → jrediger
Priority: P2 → P1
(In reply to Alessio Placitelli [:Dexter] from comment #1) > Let's make sure nothing breaks upstream before landing this. We might need > to check: > > - https://github.com/mozilla/python_moztelemetry/tree/master/moztelemetry > - the dataset generation jobs (Longitudinal/Main summary?) > - probe scraper (there's bug 1455319 for that) > > Frank, does this sound right? Am I missing something? This covers it on the pipeline side!
Flags: needinfo?(fbertsch)
Comment on attachment 8970583 [details] Bug 1452552 - Add "platforms" property to scalars. https://reviewboard.mozilla.org/r/239328/#review245078 This looks good, great job! I just have a few nits, check below. I don't particularly like the idea of using a pref to decide the platform we're on, but we have no better option :'( ::: toolkit/components/telemetry/TelemetryCommon.h:31 (Diff revision 1) > AllChildren = 0xFFFFFFFF - 1, // All the child processes (i.e. content, gpu, ...) > All = 0xFFFFFFFF // All the processes > }; > MOZ_MAKE_ENUM_CLASS_BITWISE_OPERATORS(RecordedProcessType); > > +enum class Platform : uint32_t { nit: I would use something more verbose, e.g. `SupportedPlatform` ::: toolkit/components/telemetry/TelemetryCommon.h:35 (Diff revision 1) > > +enum class Platform : uint32_t { > + Firefox = (1 << 0), > + Fennec = (1 << 1), > + Geckoview = (1 << 2), > +}; Do we need to support an "All" here? ::: toolkit/components/telemetry/docs/collection/scalars.rst:169 (Diff revision 1) > --------------- > > - ``cpp_guard``: A string that gets inserted as an ``#ifdef`` directive around the automatically generated C++ declaration. This is typically used for platform-specific scalars, e.g. ``ANDROID``. > - ``release_channel_collection``: This can be either ``opt-in`` (default) or ``opt-out``. With the former the scalar is submitted by default on pre-release channels, unless the user has opted out. With the latter the scalar is submitted by default on release and pre-release channels, unless the user has opted out. > - ``keyed``: A boolean that determines whether this is a keyed scalar. It defaults to ``False``. > +- ``platforms``: A list of platforms the event can be recorded on. It defaults to ``firefox, fennec``. Currently supported values are: "...the scalar can be recorded on." ::: toolkit/components/telemetry/parse_scalars.py:293 (Diff revision 1) > + """Get the non-empty list of platforms to record data on""" > + return self._definition.get('platforms', ["firefox", "fennec"]) > + > + @property > + def platforms_enum(self): > + """Get the non-empty list of flats representing platforms to record data on""" nit: typo, "flags" :) ::: toolkit/components/telemetry/shared_telemetry_utils.py:26 (Diff revision 1) > 'gpu': 'Gpu', > # Historical Values > 'all_childs': 'AllChildren', # Supporting files from before bug 1363725 > } > > +KNOWN_PLATFORMS = { nit: I'd call it `SUPPORTED_PLATFORMS`, as the platforms we know about are more than the ones we support :)
Attachment #8970583 - Flags: review?(alessio.placitelli) → review+
Comment on attachment 8970582 [details] Bug 1452552 - Extract commonly usable function into commons module. https://reviewboard.mozilla.org/r/239326/#review245088 ::: toolkit/components/telemetry/TelemetryCommon.h:135 (Diff revision 1) > */ > JSString* > ToJSString(JSContext* cx, const nsAString& aStr); > > +/** > + * Read an array of strings from a JavaScript object. Is this ever used outside of TelemetryEvents.cpp? ::: toolkit/components/telemetry/TelemetryCommon.h:137 (Diff revision 1) > ToJSString(JSContext* cx, const nsAString& aStr); > > +/** > + * Read an array of strings from a JavaScript object. > + * > + * @param cx The JS context nit: let's add trailing '.' at the end of all the params. ::: toolkit/components/telemetry/TelemetryCommon.h:144 (Diff revision 1) > + * @param property The property name to read > + * @param results The array to hold the parsed values > + * @returns true if the array could be read or false otherwise. Logs a JS error if it fails. > + */ > +bool > +GetArrayPropertyValues(JSContext* cx, JS::HandleObject obj, const char* property, nit: even though the code style isn't completely uniform across all the files, let's try to keep a consistent style for functions we share/common. Let's change the name of the parameters to start with "a", e.g. `aObj`, `aProperty` ::: toolkit/components/telemetry/TelemetryCommon.cpp:188 (Diff revision 1) > { > return JS_NewUCStringCopyN(cx, aStr.Data(), aStr.Length()); > } > > +bool > +GetArrayPropertyValues(JSContext* cx, JS::HandleObject obj, const char* property, nit: let's rename the parameters
Comment on attachment 8970584 [details] Bug 1452552 - Add test coverage for platform-dependent scalars. https://reviewboard.mozilla.org/r/239330/#review245092 ::: toolkit/components/telemetry/Scalars.yaml:1820 (Diff revision 1) > notification_emails: > - telemetry-client-dev@mozilla.com > record_in_processes: > - 'all_childs' > > + desktop_only: Let's add a single non-keyed desktop scalar with no "platform" property and check that we correctly record to it on desktop/fennec.
Comment on attachment 8970585 [details] Bug 1452552 - Add "platforms" property to events. https://reviewboard.mozilla.org/r/239332/#review245094
Attachment #8970585 - Flags: review?(alessio.placitelli) → review+
Comment on attachment 8970586 [details] Bug 1452552 - Add test coverage for platform-dependent events. https://reviewboard.mozilla.org/r/239334/#review245096 ::: toolkit/components/telemetry/tests/unit/test_TelemetryEvents.js:691 (Diff revision 1) > }); > + > +add_task({ > + skip_if: () => gIsAndroid > +}, > +async function test_platformSpecificEvents() { As for the scalars, let's also test that it works correctly when the platform property is missing.
Comment on attachment 8970587 [details] Bug 1452552 - Add "platforms" property to histograms. https://reviewboard.mozilla.org/r/239336/#review245108 ::: toolkit/components/telemetry/TelemetryHistogram.cpp:611 (Diff revision 1) > bool canRecordDataset = CanRecordDataset(gHistogramInfos[id].dataset, > internal_CanRecordBase(), > internal_CanRecordExtended()); > // If `histogram` is a non-parent-process histogram, then recording-enabled > // has been checked in its owner process. > if (!canRecordDataset || Any reason why you didn't add the check [here](https://searchfox.org/mozilla-central/rev/8f06c1b9a080b84435a2906e420fe102e1ed780b/toolkit/components/telemetry/TelemetryHistogram.cpp#1939,1960) as well?
Comment on attachment 8970588 [details] Bug 1452552 - Add test coverage for platform-dependent histograms. https://reviewboard.mozilla.org/r/239338/#review245112 ::: toolkit/components/telemetry/Histograms.json:7504 (Diff revision 1) > "high": 10000, > "n_buckets": 10, > "bug_numbers": [1335343], > "description": "a testing histogram; not meant to be touched" > }, > + "TELEMETRY_TEST_DESKTOP_ONLY": { As with the other probes, add one test histogram without the platform property :)
Attachment #8970582 - Attachment is obsolete: true
Attachment #8970582 - Flags: review?(alessio.placitelli)
Comment on attachment 8970583 [details] Bug 1452552 - Add "platforms" property to scalars. https://reviewboard.mozilla.org/r/239328/#review245322 ::: toolkit/components/telemetry/TelemetryCommon.h:35 (Diff revision 1) > > +enum class Platform : uint32_t { > + Firefox = (1 << 0), > + Fennec = (1 << 1), > + Geckoview = (1 << 2), > +}; That would make it easier to enable it from the yaml files. Adding it.
Addressed your comments. I also added the additional test as you said. However, I'm currently a bit uncomfortable with the tests with regard to GeckoView. Currently we can only check if the xpcshell tests run on Android or not. They do run in a fennec-like environment. With the default of `firefox, fennec` for the platform property, all existing tests continue to work. But if the tests are run in a geckoview-like environment they will start to fail. How do we solve this going forward? Are those `xpcshell-tests in geckoview-like environment` currently done or planned?
Comment on attachment 8970587 [details] Bug 1452552 - Add "platforms" property to histograms. https://reviewboard.mozilla.org/r/239336/#review245630
Attachment #8970587 - Flags: review?(alessio.placitelli) → review+
Comment on attachment 8970584 [details] Bug 1452552 - Add test coverage for platform-dependent scalars. https://reviewboard.mozilla.org/r/239330/#review246148 ::: toolkit/components/telemetry/Scalars.yaml:1902 (Diff revision 3) > + - 'all' > + platforms: > + - fennec > + - geckoview > + > + nit: let's remove one empty blank line here
Attachment #8970584 - Flags: review?(alessio.placitelli) → review+
Comment on attachment 8970586 [details] Bug 1452552 - Add test coverage for platform-dependent events. https://reviewboard.mozilla.org/r/239334/#review246150
Attachment #8970586 - Flags: review?(alessio.placitelli) → review+
Comment on attachment 8970588 [details] Bug 1452552 - Add test coverage for platform-dependent histograms. https://reviewboard.mozilla.org/r/239338/#review246152
Attachment #8970588 - Flags: review?(alessio.placitelli) → review+
Comment on attachment 8971585 [details] Bug 1452552 - Test geckoview-only scalar probe in geckoview-like mode. https://reviewboard.mozilla.org/r/240026/#review246156 ::: toolkit/components/telemetry/tests/unit/test_TelemetryScalars.js:865 (Diff revision 1) > + > + Assert.ok(!(DEFAULT_PLATFORM_SCALAR in scalars), "The default platforms scalar must contain the right value"); > + Assert.ok(!(DESKTOP_ONLY_SCALAR in scalars), "The desktop-only scalar must not be persisted."); > + > + // Reset to original environment > + Services.prefs.setBoolPref("toolkit.telemetry.isGeckoViewMode", false); Let's use `Services.prefs.reset` here.
Attachment #8971585 - Flags: review?(alessio.placitelli) → review+
Comment on attachment 8971584 [details] Bug 1452552 - Add a method to reset the platform during runtime on Android. https://reviewboard.mozilla.org/r/240024/#review246164 ::: toolkit/components/telemetry/Telemetry.cpp:511 (Diff revision 1) > + // Initially set the current platform. > + SetCurrentPlatform(); > + > +#if defined(MOZ_WIDGET_ANDROID) > + // If on Android, add an observer to determine changes in geckoview mode > + Preferences::AddStrongObserver(this, "toolkit.telemetry.isGeckoViewMode"); Instead of doing this, let's just expose a test-only method on nsITelemetry that resets the current platform. Please clearly mark it as test-only in the comments, make it a no-op (or better, return NS_ERROR_FAILURE) on Desktop.
Attachment #8971584 - Flags: review?(alessio.placitelli) → review-
Comment on attachment 8971584 [details] Bug 1452552 - Add a method to reset the platform during runtime on Android. https://reviewboard.mozilla.org/r/240024/#review246346 ::: toolkit/components/telemetry/Telemetry.cpp:505 (Diff revision 2) > // called before we get to this point. > MOZ_ASSERT(TelemetryHistogram::GlobalStateHasBeenInitialized()); > + > +#if defined(MOZ_WIDGET_ANDROID) > + // Initially set the current platform on Android. > + ResetPlatform(); As stated in the docstrings, `ResetPlatform` is test-only. Instead of calling this, let's call `SetCurrentPlatform()` before this line [here](https://searchfox.org/mozilla-central/rev/08df4e6e11284186d477d7e5b0ae48483ecc979c/toolkit/components/telemetry/Telemetry.cpp#1266). We don't need compile-time guards: SetCurrentPlatform it's basically no-op on Desktop. ::: toolkit/components/telemetry/nsITelemetry.idl:576 (Diff revision 2) > + > + /** > + * Reset the current platform. This is intended to be only used in Android tests. > + * It will fail on Desktop. > + */ > + void resetPlatform(); nit: `resetCurrentPlatform` for consistency with the names of the other methods?
Attachment #8971584 - Flags: review?(alessio.placitelli) → review-
Comment on attachment 8971584 [details] Bug 1452552 - Add a method to reset the platform during runtime on Android. https://reviewboard.mozilla.org/r/240024/#review246364 Let's great now, thank you! Be mindful of the code-freeze for this changeset: we might want to wait until merge-day :( But nothing prevents us from pushing to treeherder for trying ;)
Attachment #8971584 - Flags: review?(alessio.placitelli) → review+
Was this updated to our recent naming discussions? We talked about that "platform" is an overloaded term that is easily misunderstood. We found that we should probably use "product" (for this bugs property) and "operating system" (additionally, later) instead, as those are clear.
Flags: needinfo?(jrediger)
Talking through this with Alessio about last weeks conversations, "products" currently seems the best name here. Lets move forward with that naming now and then land it.
Flags: needinfo?(jrediger)
Assignee: jrediger → alessio.placitelli
Attachment #8970583 - Attachment is patch: true
Attachment #8970583 - Attachment mime type: text/x-review-board-request → text/plain
Attachment #8970583 - Attachment is obsolete: true
Attachment #8970583 - Attachment is patch: false
Attachment #8970583 - Attachment mime type: text/plain → text/x-review-board-request
Attachment #8970584 - Attachment is obsolete: true
Attachment #8970585 - Attachment is obsolete: true
Attachment #8970586 - Attachment is obsolete: true
Attachment #8970587 - Attachment is obsolete: true
Attachment #8970588 - Attachment is obsolete: true
Attachment #8971584 - Attachment is obsolete: true
Attachment #8971585 - Attachment is obsolete: true
Comment on attachment 8973715 [details] Bug 1452552 - Add "products" property to events. https://reviewboard.mozilla.org/r/242082/#review247914 Code analysis found 1 defect in this patch: - 1 defect found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: toolkit/components/telemetry/docs/collection/events.rst:270 (Diff revision 1) > > // Let us suppose in the parent process this happens: > Services.telemetry.recordEvent("interaction", "click", "document", "xuldoc"); > Services.telemetry.recordEvent("interaction", "click", "document", "xuldoc-neighbour"); > > // And in each of child proccesses 1 through 4, this happens: Warning: Proccesses ==> processes [codespell]
Summary: Add "platform" properties to measurements → Add "products" properties to measurements
Comment on attachment 8973720 [details] Bug 1452552 - Test geckoview-only scalar probe in geckoview-like mode. https://reviewboard.mozilla.org/r/242092/#review247942
Attachment #8973720 - Flags: review?(alessio.placitelli) → review+
Comment on attachment 8973719 [details] Bug 1452552 - Add a method to reset the product during runtime on Android. https://reviewboard.mozilla.org/r/242090/#review247944
Attachment #8973719 - Flags: review?(alessio.placitelli) → review+
Comment on attachment 8973718 [details] Bug 1452552 - Add test coverage for product-dependent histograms. https://reviewboard.mozilla.org/r/242088/#review247946
Attachment #8973718 - Flags: review?(alessio.placitelli) → review+
Comment on attachment 8973717 [details] Bug 1452552 - Add "products" property to histograms. https://reviewboard.mozilla.org/r/242086/#review247948
Attachment #8973717 - Flags: review?(alessio.placitelli) → review+
Comment on attachment 8973713 [details] Bug 1452552 - Add "products" property to scalars. https://reviewboard.mozilla.org/r/242078/#review247950
Attachment #8973713 - Flags: review?(alessio.placitelli) → review+
Comment on attachment 8973714 [details] Bug 1452552 - Add test coverage for product-dependent scalars. https://reviewboard.mozilla.org/r/242080/#review247952
Attachment #8973714 - Flags: review?(alessio.placitelli) → review+
Comment on attachment 8973715 [details] Bug 1452552 - Add "products" property to events. https://reviewboard.mozilla.org/r/242082/#review247954
Attachment #8973715 - Flags: review?(alessio.placitelli) → review+
Comment on attachment 8973716 [details] Bug 1452552 - Add test coverage for product-dependent events. https://reviewboard.mozilla.org/r/242084/#review247956
Attachment #8973716 - Flags: review?(alessio.placitelli) → review+
Pushed by alessio.placitelli@gmail.com: https://hg.mozilla.org/integration/autoland/rev/4cf35dc3b29d Add "products" property to scalars. r=Dexter https://hg.mozilla.org/integration/autoland/rev/348f202fccca Add test coverage for product-dependent scalars. r=Dexter https://hg.mozilla.org/integration/autoland/rev/5f94080d21a0 Add "products" property to events. r=Dexter https://hg.mozilla.org/integration/autoland/rev/77588674e03a Add test coverage for product-dependent events. r=Dexter https://hg.mozilla.org/integration/autoland/rev/cd032e9f08e8 Add "products" property to histograms. r=Dexter https://hg.mozilla.org/integration/autoland/rev/f526292dbfbe Add test coverage for product-dependent histograms. r=Dexter https://hg.mozilla.org/integration/autoland/rev/96160ea2aa64 Add a method to reset the product during runtime on Android. r=Dexter https://hg.mozilla.org/integration/autoland/rev/e0dc79c04031 Test geckoview-only scalar probe in geckoview-like mode. r=Dexter
Blocks: 1460317
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: