Specify relative order of nodes in ukm.xml

For each ObjectNodeType, specify alphabetization rules for all its child nodes types, which enforce their order relative to other types when pretty printed (sorting key is checked here: https://cs.chromium.org/chromium/src/tools/metrics/common/pretty_print_xml.py?q=_TransformByAlphabetizing&g=0&l=143). This patch fixes the issue where the pretty print script needs to be run more than once before the XML is sufficiently pretty for presubmit check.


Bug: 801614
Change-Id: I7e9076c8e55d300f346bde07db0b12d4c7533661
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1561610Reviewed-by: Robert Kaplow <rkaplow@chromium.org>
Commit-Queue: Yue Ru Sun <yrsun@chromium.org>
Cr-Commit-Position: refs/heads/master@{#649863}

tools/metrics/ukm/pretty_print.py

@@ -14,7 +14,7 @@ import presubmit_util
 
 def main(argv):
   presubmit_util.DoPresubmitMain(argv, 'ukm.xml', 'ukm.old.xml',
-                                 'pretty_print.py', ukm_model.UpdateXML)
+                                 'pretty_print.py', ukm_model.PrettifyXML)
 
 
 if '__main__' == __name__:

tools/metrics/ukm/ukm_model.py

@@ -14,7 +14,11 @@ _OBSOLETE_TYPE = models.TextNodeType('obsolete')
 _OWNER_TYPE = models.TextNodeType('owner', single_line=True)
 _SUMMARY_TYPE = models.TextNodeType('summary')
 
-_LOWERCASE_NAME_FN = lambda n: n.get('name').lower()
+# A key for sorting XML nodes by the value of |attribute|.
+_LOWERCASE_FN = lambda attribute: (lambda node: node.get(attribute).lower())
+# A constant function as the sorting key for nodes whose orderings should be
+# kept as given in the XML file within their parent node.
+_KEEP_ORDER = lambda node: 1
 
 _ENUMERATION_TYPE = models.ObjectNodeType(
     'enumeration',
@@ -42,23 +46,28 @@ _STATISTICS_TYPE =  models.ObjectNodeType(
     'statistics',
     attributes=[],
     children=[
-        models.ChildType('quantiles', _QUANTILES_TYPE, multiple=False),
-        models.ChildType('enumeration', _ENUMERATION_TYPE, multiple=False),
+        models.ChildType(_QUANTILES_TYPE.tag, _QUANTILES_TYPE, multiple=False),
+        models.ChildType(
+            _ENUMERATION_TYPE.tag, _ENUMERATION_TYPE, multiple=False),
     ])
 
 _HISTORY_TYPE =  models.ObjectNodeType(
     'history',
     attributes=[],
+    alphabetization=[
+        (_INDEX_TYPE.tag, _LOWERCASE_FN('fields')),
+        (_STATISTICS_TYPE.tag, _KEEP_ORDER),
+    ],
     children=[
-        models.ChildType('index', _INDEX_TYPE, multiple=True),
-        models.ChildType('statistics', _STATISTICS_TYPE, multiple=True),
+        models.ChildType(_INDEX_TYPE.tag, _INDEX_TYPE, multiple=True),
+        models.ChildType(_STATISTICS_TYPE.tag, _STATISTICS_TYPE, multiple=True),
     ])
 
 _AGGREGATION_TYPE =  models.ObjectNodeType(
     'aggregation',
     attributes=[],
     children=[
-        models.ChildType('history', _HISTORY_TYPE, multiple=False),
+        models.ChildType(_HISTORY_TYPE.tag, _HISTORY_TYPE, multiple=False),
     ])
 
 _METRIC_TYPE =  models.ObjectNodeType(
@@ -67,40 +76,52 @@ _METRIC_TYPE =  models.ObjectNodeType(
       ('name', unicode, r'^[A-Za-z0-9_.]+$'),
       ('semantic_type', unicode, None),
     ],
+    alphabetization=[
+        (_OBSOLETE_TYPE.tag, _KEEP_ORDER),
+        (_OWNER_TYPE.tag, _KEEP_ORDER),
+        (_SUMMARY_TYPE.tag, _KEEP_ORDER),
+        (_AGGREGATION_TYPE.tag, _KEEP_ORDER),
+    ],
     children=[
-        models.ChildType('obsolete', _OBSOLETE_TYPE, multiple=False),
-        models.ChildType('owners', _OWNER_TYPE, multiple=True),
-        models.ChildType('summary', _SUMMARY_TYPE, multiple=False),
-        models.ChildType('aggregation', _AGGREGATION_TYPE, multiple=True),
+        models.ChildType(_OBSOLETE_TYPE.tag, _OBSOLETE_TYPE, multiple=False),
+        models.ChildType(_OWNER_TYPE.tag, _OWNER_TYPE, multiple=True),
+        models.ChildType(_SUMMARY_TYPE.tag, _SUMMARY_TYPE, multiple=False),
+        models.ChildType(
+            _AGGREGATION_TYPE.tag, _AGGREGATION_TYPE, multiple=True),
     ])
 
 _EVENT_TYPE =  models.ObjectNodeType(
     'event',
-    alphabetization=[('metric', _LOWERCASE_NAME_FN)],
     attributes=[
       ('name', unicode, r'^[A-Za-z0-9.]+$'),
       ('singular', unicode, r'^(?i)(|true|false)$'),
     ],
+    alphabetization=[
+        (_OBSOLETE_TYPE.tag, _KEEP_ORDER),
+        (_OWNER_TYPE.tag, _KEEP_ORDER),
+        (_SUMMARY_TYPE.tag, _KEEP_ORDER),
+        (_METRIC_TYPE.tag, _LOWERCASE_FN('name')),
+    ],
     extra_newlines=(1, 1, 1),
     children=[
-        models.ChildType('obsolete', _OBSOLETE_TYPE, multiple=False),
-        models.ChildType('owners', _OWNER_TYPE, multiple=True),
-        models.ChildType('summary', _SUMMARY_TYPE, multiple=False),
-        models.ChildType('metrics', _METRIC_TYPE, multiple=True),
+        models.ChildType(_OBSOLETE_TYPE.tag, _OBSOLETE_TYPE, multiple=False),
+        models.ChildType(_OWNER_TYPE.tag, _OWNER_TYPE, multiple=True),
+        models.ChildType(_SUMMARY_TYPE.tag, _SUMMARY_TYPE, multiple=False),
+        models.ChildType(_METRIC_TYPE.tag, _METRIC_TYPE, multiple=True),
     ])
 
 _UKM_CONFIGURATION_TYPE = models.ObjectNodeType(
     'ukm-configuration',
-    alphabetization=[('event', _LOWERCASE_NAME_FN)],
+    alphabetization=[(_EVENT_TYPE.tag, _LOWERCASE_FN('name'))],
     extra_newlines=(2, 1, 1),
     indent=False,
     children=[
-        models.ChildType('events', _EVENT_TYPE, multiple=True),
+        models.ChildType(_EVENT_TYPE.tag, _EVENT_TYPE, multiple=True),
     ])
 
 UKM_XML_TYPE = models.DocumentType(_UKM_CONFIGURATION_TYPE)
 
-def UpdateXML(original_xml):
+def PrettifyXML(original_xml):
   """Parses the original xml and return a pretty printed version.
 
   Args:

tools/metrics/ukm/ukm_model_test.py

@@ -49,6 +49,43 @@ PRETTY_XML = """
 </ukm-configuration>
 """.strip()
 
+UNPRETTIFIED_XML = """
+<!-- Comment1 -->
+<ukm-configuration>
+<event name="Event1">
+<metric name="Metric3"/>
+<metric name="Metric1">
+  <owner>owner2@chromium.org</owner>
+  <summary>
+    Metric1 summary.
+  </summary>
+  <aggregation>
+    <history>
+      <index fields="profile.form_factor"/>
+      <statistics>
+        <quantiles type="std-percentiles"/>
+      </statistics>
+      <index fields="profile.country"/>
+
+    </history>
+  </aggregation>
+</metric>
+        <metric name="Metric2">
+          <aggregation>
+            <history>
+              <statistics><enumeration export="False"/></statistics>
+            </history>
+          </aggregation>
+        </metric>
+
+  <summary>Event1 summary.</summary>
+  <owner>owner@chromium.org</owner>
+  <owner>anotherowner@chromium.org</owner>
+
+</event>
+</ukm-configuration>
+""".strip()
+
 CONFIG_EVENT_NAMES_SORTED = """
 <ukm-configuration>
 
@@ -75,26 +112,30 @@ CONFIG_EVENT_NAMES_UNSORTED = """
 
 class UkmXmlTest(unittest.TestCase):
 
-  def testIsPretty(self):
-    result = ukm_model.UpdateXML(PRETTY_XML)
+  def testPrettify(self):
+    result = ukm_model.PrettifyXML(PRETTY_XML)
+    self.assertMultiLineEqual(PRETTY_XML, result.strip())
+    result = ukm_model.PrettifyXML(UNPRETTIFIED_XML)
     self.assertMultiLineEqual(PRETTY_XML, result.strip())
 
   def testHasBadEventName(self):
     bad_xml = PRETTY_XML.replace('Event1', 'Event:1')
     with self.assertRaises(ValueError) as context:
-      ukm_model.UpdateXML(bad_xml)
+      ukm_model.PrettifyXML(bad_xml)
     self.assertIn('Event:1', str(context.exception))
     self.assertIn('does not match regex', str(context.exception))
 
   def testHasBadMetricName(self):
     bad_xml = PRETTY_XML.replace('Metric1', 'Metric:1')
     with self.assertRaises(ValueError) as context:
-      ukm_model.UpdateXML(bad_xml)
+      ukm_model.PrettifyXML(bad_xml)
     self.assertIn('Metric:1', str(context.exception))
     self.assertIn('does not match regex', str(context.exception))
 
   def testSortByEventName(self):
-    result = ukm_model.UpdateXML(CONFIG_EVENT_NAMES_UNSORTED)
+    result = ukm_model.PrettifyXML(CONFIG_EVENT_NAMES_SORTED)
+    self.assertMultiLineEqual(CONFIG_EVENT_NAMES_SORTED, result.strip())
+    result = ukm_model.PrettifyXML(CONFIG_EVENT_NAMES_UNSORTED)
     self.assertMultiLineEqual(CONFIG_EVENT_NAMES_SORTED, result.strip())
 
 if __name__ == '__main__':