Change the marker schema to accept a description and get rid of the notion of static fields (#5385)

In a marker schema, the `data` array can currently hold both "dynamic"
and "static" fields. Dynamic fields get their value from each marker,
static fields have a single fixed value.

In the years since the marker schema has been available, the only thing
we've used "static" fields for is a "description" field.

So I think we should just have an actual description field which is
separate from schema.data. And we should remove generic "static" fields.
This simplifies the format, and it also simplifies our code because we
can iterate over the fields without having to check whether a field is a
static or a dynamic field - Flow was requiring extra verbose checks in
this case because the two field types don't have a shared discriminator
property.

This change does make the schema a little less expressive, but nobody
was making use of the extra expressivity.

Also, at some point we'll want to move to a more compact representation
of markers in the profile JSON by not repeating the field keys on every
marker. One way to do this would be to store marker field values in an
array, where each array value maps to the field at the corresponding
index. This representation makes more sense if the fields array only
contains fields that actually get their value from the marker.

This PR only changes the processed format. The Gecko profile format
remains unchanged. We can change it in a follow-up.

On the Gecko side, we currently have an old and [a new
way](https://bugzilla.mozilla.org/show_bug.cgi?id=1869835) to declare
marker schemas. The new way got rid of static fields too and just
accepts [a
description](https://searchfox.org/mozilla-central/rev/f256e50e068136275c1a2aff827aeddc92a75e07/mozglue/baseprofiler/public/BaseProfilerMarkersPrerequisites.h#1089-1091)
(but still outputs the description as a static field).

Author: mstange

docs-developer/CHANGELOG-formats.md

@@ -6,6 +6,16 @@ Note that this is not an exhaustive list. Processed profile format upgraders can
 
 ## Processed profile format
 
+### Version 55
+
+Changes to the `MarkerSchema` type which is used for the elements of the array at `profile.meta.markerSchema`:
+
+- A new `description` field was added. This field is optional.
+- The `data` property was renamed to `fields`.
+- Every field must have a `key` and a `format` property now. There are no static fields any more.
+
+Concretely, this means that if you have a `{ "label": "Description", value: "..." }` entry in your marker schema's `data` array, this entry needs to be removed and the description needs to be put into the `description` field instead, and the `data` property needs to be renamed to `fields`. If you have any other static fields, i.e. fields with `label` and `value` properties rather than `key` and `format` properties, then they need to be removed without replacement.
+
 ### Version 54
 
 The `implementation` column was removed from the frameTable. Modern profiles from Firefox use subcategories to represent the information about the JIT type of a JS frame.

src/app-logic/constants.js

@@ -14,7 +14,7 @@ export const GECKO_PROFILE_VERSION = 31;
 // The current version of the "processed" profile format.
 // Please don't forget to update the processed profile format changelog in
 // `docs-developer/CHANGELOG-formats.md`.
-export const PROCESSED_PROFILE_VERSION = 54;
+export const PROCESSED_PROFILE_VERSION = 55;
 
 // The following are the margin sizes for the left and right of the timeline. Independent
 // components need to share these values.

src/components/tooltip/Marker.js

@@ -241,52 +241,43 @@ class MarkerTooltipContents extends React.PureComponent<Props> {
       // Add the details for the markers based on their Marker schema.
       const schema = getSchemaFromMarker(markerSchemaByName, marker.data);
       if (schema) {
-        for (const schemaData of schema.data) {
-          if (schemaData.hidden) {
-            // Do not include the data field if it's marked as hidden.
+        for (const field of schema.fields) {
+          if (field.hidden) {
+            // Do not include hidden fields.
             continue;
           }
 
-          // Check for a schema that is looking up and formatting a value from
-          // the payload.
-          if (schemaData.value === undefined) {
-            const { key, label, format } = schemaData;
-            if (key in data) {
-              const value = data[key];
-
-              // Don't add undefined values, as values are optional.
-              if (value !== undefined && value !== null) {
-                details.push(
-                  <TooltipDetail
-                    key={schema.name + '-' + key}
-                    label={label || key}
-                  >
-                    {formatMarkupFromMarkerSchema(
-                      schema.name,
-                      format,
-                      value,
-                      thread.stringTable,
-                      threadIdToNameMap,
-                      processIdToNameMap
-                    )}
-                  </TooltipDetail>
-                );
-              }
-            }
-          }
+          const { key, label, format } = field;
 
-          // Do a check to see if there is no key. This means this is a simple
-          // label that is applied to every marker of this type, with no data
-          // lookup. For some reason Flow as not able to refine this.
-          if (schemaData.key === undefined) {
-            const { label, value } = schemaData;
-            const key = label + '-' + value;
-            details.push(
-              <TooltipDetail key={key} label={label}>
-                <div className="tooltipDetailsDescription">{value}</div>
-              </TooltipDetail>
-            );
+          const value = data[key];
+          if (value === undefined || value === null) {
+            // This marker doesn't have a value for this field. Values are optional.
+            continue;
           }
+
+          details.push(
+            <TooltipDetail key={schema.name + '-' + key} label={label || key}>
+              {formatMarkupFromMarkerSchema(
+                schema.name,
+                format,
+                value,
+                thread.stringTable,
+                threadIdToNameMap,
+                processIdToNameMap
+              )}
+            </TooltipDetail>
+          );
+        }
+
+        if (schema.description) {
+          const key = schema.name + '-description';
+          details.push(
+            <TooltipDetail key={key} label="Description">
+              <div className="tooltipDetailsDescription">
+                {schema.description}
+              </div>
+            </TooltipDetail>
+          );
         }
       }
 

src/profile-logic/import/chrome.js

@@ -939,7 +939,7 @@ function extractMarkers(
       tooltipLabel: '{marker.data.type2} - EventDispatch',
       tableLabel: '{marker.data.type2}',
       display: ['marker-chart', 'marker-table', 'timeline-overview'],
-      data: [
+      fields: [
         {
           // In the original chrome profile, the key is `type`, but we rename it
           // so that it doesn't clash with our internal `type` property.

src/profile-logic/marker-data.js

@@ -1434,36 +1434,29 @@ export function sanitizeFromMarkerSchema(
   markerSchema: MarkerSchema,
   markerPayload: MarkerPayload
 ): MarkerPayload {
-  for (const propertyDescription of markerSchema.data) {
-    if (
-      propertyDescription.key !== undefined &&
-      propertyDescription.format !== undefined
-    ) {
-      const key = propertyDescription.key;
-      const format = propertyDescription.format;
-      if (!(key in markerPayload)) {
-        continue;
-      }
+  for (const { key, format } of markerSchema.fields) {
+    if (!(key in markerPayload)) {
+      continue;
+    }
 
-      // We're typing the result of the sanitization with `any` because Flow
-      // doesn't like much our enormous enum of non-exact objects that's used as
-      // MarkerPayload type, and this code is too generic for Flow in this context.
-      if (format === 'url') {
-        markerPayload = ({
-          ...markerPayload,
-          [key]: removeURLs(markerPayload[key]),
-        }: any);
-      } else if (format === 'file-path') {
-        markerPayload = ({
-          ...markerPayload,
-          [key]: removeFilePath(markerPayload[key]),
-        }: any);
-      } else if (format === 'sanitized-string') {
-        markerPayload = ({
-          ...markerPayload,
-          [key]: '<sanitized>',
-        }: any);
-      }
+    // We're typing the result of the sanitization with `any` because Flow
+    // doesn't like much our enormous enum of non-exact objects that's used as
+    // MarkerPayload type, and this code is too generic for Flow in this context.
+    if (format === 'url') {
+      markerPayload = ({
+        ...markerPayload,
+        [key]: removeURLs(markerPayload[key]),
+      }: any);
+    } else if (format === 'file-path') {
+      markerPayload = ({
+        ...markerPayload,
+        [key]: removeFilePath(markerPayload[key]),
+      }: any);
+    } else if (format === 'sanitized-string') {
+      markerPayload = ({
+        ...markerPayload,
+        [key]: '<sanitized>',
+      }: any);
     }
   }
 

src/profile-logic/marker-schema.js

@@ -39,16 +39,12 @@ export const markerSchemaFrontEndOnly: MarkerSchema[] = [
     display: ['marker-table', 'marker-chart'],
     tooltipLabel: 'Jank – event processing delay',
     tableLabel: 'Event processing delay',
-    data: [
-      {
-        label: 'Description',
-        value: oneLine`
-          Jank markers show when the main event loop of a thread has been busy. It is
-          a good indicator that there may be some kind of performance problem that
-          is worth investigating.
-        `,
-      },
-    ],
+    fields: [],
+    description: oneLine`
+      Jank markers show when the main event loop of a thread has been busy. It is
+      a good indicator that there may be some kind of performance problem that
+      is worth investigating.
+    `,
   },
   // Note, these can also come from Gecko, but since we have lots of special handling
   // for IPC, the Gecko ones get overwritten by this definition.
@@ -58,17 +54,12 @@ export const markerSchemaFrontEndOnly: MarkerSchema[] = [
     tableLabel: '{marker.data.messageType} — {marker.data.niceDirection}',
     chartLabel: '{marker.data.messageType}',
     display: ['marker-chart', 'marker-table', 'timeline-ipc'],
-    data: [
+    fields: [
       { key: 'messageType', label: 'Type', format: 'string', searchable: true },
       { key: 'sync', label: 'Sync', format: 'string' },
       { key: 'sendThreadName', label: 'From', format: 'string' },
       { key: 'recvThreadName', label: 'To', format: 'string' },
-      {
-        key: 'otherPid',
-        label: 'Other Pid',
-        format: 'pid',
-        searchable: true,
-      },
+      { key: 'otherPid', label: 'Other Pid', format: 'pid', searchable: true },
     ],
   },
   {
@@ -78,7 +69,7 @@ export const markerSchemaFrontEndOnly: MarkerSchema[] = [
     name: 'Network',
     display: ['marker-table', 'marker-chart'],
     chartLabel: '{marker.data.URI}',
-    data: [
+    fields: [
       {
         format: 'string',
         key: 'contentType',
@@ -212,11 +203,9 @@ export function parseLabel(
       // Handle:                                      ^^^^^^^^^^^^^^^^
 
       let format = null;
-      for (const rule of markerSchema.data) {
-        // The rule.value === undefined line is odd mainly because Flow was having trouble
-        // refining the type.
-        if (rule.value === undefined && rule.key === payloadKey) {
-          format = rule.format;
+      for (const field of markerSchema.fields) {
+        if (field.key === payloadKey) {
+          format = field.format;
           break;
         }
       }
@@ -634,19 +623,19 @@ export function markerPayloadMatchesSearch(
   }
 
   // Check if searchable fields match the search regular expression.
-  for (const payloadField of markerSchema.data) {
+  for (const payloadField of markerSchema.fields) {
     if (payloadField.searchable) {
       let value = data[payloadField.key];
+      if (value === undefined || value === null) {
+        // The value is missing, but this is OK, values are optional.
+        continue;
+      }
+
       if (
         payloadField.format === 'unique-string' ||
         payloadField.format === 'flow-id' ||
         payloadField.format === 'terminating-flow-id'
       ) {
-        if (value === undefined) {
-          // The value is missing, but this is OK, values are optional.
-          continue;
-        }
-
         if (typeof value !== 'number') {
           console.warn(
             `In marker ${marker.name}, the key ${payloadField.key} has an invalid value "${value}" as a unique string, it isn't a number.`
@@ -663,11 +652,7 @@ export function markerPayloadMatchesSearch(
         value = stringTable.getString(value);
       }
 
-      if (value === undefined || value === null || value === '') {
-        continue;
-      }
-
-      if (testFun(value, payloadField.key)) {
+      if (value !== '' && testFun(value, payloadField.key)) {
         return true;
       }
     }
@@ -691,9 +676,9 @@ export function computeStringIndexMarkerFieldsByDataType(
   stringIndexMarkerFieldsByDataType.set('CompositorScreenshot', ['url']);
 
   for (const schema of markerSchemas) {
-    const { name, data } = schema;
+    const { name, fields } = schema;
     const stringIndexFields = [];
-    for (const field of data) {
+    for (const field of fields) {
       if (field.format === 'unique-string' && field.key) {
         stringIndexFields.push(field.key);
       }