Sort and combine the histogram_suffixes_list node in merge_xml

We also need to sort and combine the histogram-suffixes-list node in the
merge_xml. The pretty-print function assumed there is only one
histogram-suffixes-list node and it dropped all histogram-suffixes nodes
in the split histogram_suffixes_list.xml file which resulted in missing
all histograms that were generated from that xml. Added unit tests to
make sure this problem won't happen again.

This cl also renames histogram_suffixes.xml to
histogram_suffixes_list.xml to be consistent with histograms.xml naming
convention.

Bug: b:164097807
Change-Id: I67144c5726acd5aa96ce1a1f597c7bf88e7eda6f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2375936
Commit-Queue: Weilun Shi <sweilun@chromium.org>
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Auto-Submit: Weilun Shi <sweilun@chromium.org>
Cr-Commit-Position: refs/heads/master@{#801615}

tools/metrics/histograms/histogram_paths.py

@@ -17,7 +17,8 @@ def _FindHistogramsXmlFiles():
   file_list = []
   for dirName, _, fileList in os.walk(PATH_TO_HISTOGRAMS_XML_DIR):
     for filename in fileList:
-      if filename == 'histograms.xml' or filename == 'histogram_suffixes.xml':
+      if (filename == 'histograms.xml'
+          or filename == 'histogram_suffixes_list.xml'):
         # Compute the relative path of the histograms xml file.
         file_path = os.path.relpath(
           os.path.join(dirName, filename), PATH_TO_HISTOGRAMS_XML_DIR)
@@ -54,10 +55,12 @@ ALL_XMLS = [path_util.GetInputFile(f) for f in ALL_XMLS_RELATIVE]
 ALL_TEST_XMLS_RELATIVE = [
     'tools/metrics/histograms/test_data/enums.xml',
     'tools/metrics/histograms/test_data/histograms.xml',
+    'tools/metrics/histograms/test_data/histogram_suffixes_list.xml',
     'tools/metrics/histograms/test_data/ukm.xml',
 ]
 ALL_TEST_XMLS = [path_util.GetInputFile(f) for f in ALL_TEST_XMLS_RELATIVE]
-TEST_ENUMS_XML, TEST_HISTOGRAMS_XML, TEST_UKM_XML = ALL_TEST_XMLS
+(TEST_ENUMS_XML, TEST_HISTOGRAMS_XML, TEST_SUFFIXES_XML,
+ TEST_UKM_XML) = ALL_TEST_XMLS
 
 # The path to the `histogram_index` file.
 HISTOGRAMS_INDEX = path_util.GetInputFile(

tools/metrics/histograms/histograms_index.txt

 tools/metrics/histograms/histograms.xml
 tools/metrics/histograms/histograms_xml/Fingerprint/histograms.xml
 tools/metrics/histograms/histograms_xml/UMA/histograms.xml
-tools/metrics/histograms/histograms_xml/histogram_suffixes.xml
+tools/metrics/histograms/histograms_xml/histogram_suffixes_list.xml
\ No newline at end of file

tools/metrics/histograms/histograms_xml/histogram_suffixes.xml → tools/metrics/histograms/histograms_xml/histogram_suffixes_list.xml

@@ -5,8 +5,8 @@ found in the LICENSE file.
 -->
 
 <!--
-This file is used to generate a comprehensive list of Chrome histograms along
-with a detailed description for each histogram.
+This file is used to specify a list of histogram suffixes for existing
+histograms to create their histogram families.
 
 For best practices on writing histogram descriptions, see
 https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histograms/README.md
@@ -14,10 +14,6 @@ https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histogra
 For brief details on how to modify this file to add your description, see
 https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histograms/one-pager.md
 
-Please pretty-print and validate your edits by running the pretty_print.py
-and validate_format.py scripts in the same directory as this file before
-uploading your change for review.
-
 Please send CLs to chromium-metrics-reviews@google.com rather than to specific
 individuals. These CLs will be automatically reassigned to a reviewer within
 about 5 minutes. This approach helps the metrics team to load-balance incoming

tools/metrics/histograms/merge_xml.py

@@ -59,24 +59,32 @@ def GetEnumsNodes(doc, trees):
 
 
 def CombineHistogramsSorted(doc, trees):
-  """Sorts <histogram> nodes by name and returns a single <histograms> node.
+  """Sorts histograms related nodes by name and returns the combined nodes.
+
+  This function sorts nodes including <histogram>, <variant> and
+  <histogram_suffix>. Then it returns one <histograms> that contains the
+  sorted <histogram> and <variant> nodes and the other <histogram_suffixes_list>
+  node containing all <histogram_suffixes> nodes.
 
   Args:
     doc: The document to create the node in.
     trees: A list of DOM trees.
 
   Returns:
-    A list containing a single <histograms> node.
+    A list containing the combined <histograms> node and the combined
+    <histogram_suffix_list> node.
   """
+  # Create the combined <histograms> tag.
   combined_histograms = doc.createElement('histograms')
 
+  def SortByLowerCaseName(node):
+    return node.getAttribute('name').lower()
+
   variants_nodes = GetElementsByTagName(trees, 'variants', depth=3)
-  sorted_variants = sorted(variants_nodes,
-                           key=lambda node: node.getAttribute('name').lower())
+  sorted_variants = sorted(variants_nodes, key=SortByLowerCaseName)
 
   histogram_nodes = GetElementsByTagName(trees, 'histogram', depth=3)
-  sorted_histograms = sorted(histogram_nodes,
-                             key=lambda node: node.getAttribute('name').lower())
+  sorted_histograms = sorted(histogram_nodes, key=SortByLowerCaseName)
 
   for variants in sorted_variants:
     # Use unsafe version of `appendChild` function here because the safe one
@@ -89,10 +97,23 @@ def CombineHistogramsSorted(doc, trees):
     xml.dom.minidom._append_child(combined_histograms, variants)
 
   for histogram in sorted_histograms:
-    # See above comment.
     xml.dom.minidom._append_child(combined_histograms, histogram)
 
-  return [combined_histograms]
+  # Create the combined <histogram_suffixes_list> tag.
+  combined_histogram_suffixes_list = doc.createElement(
+      'histogram_suffixes_list')
+
+  histogram_suffixes_nodes = GetElementsByTagName(trees,
+                                                  'histogram_suffixes',
+                                                  depth=3)
+  sorted_histogram_suffixes = sorted(histogram_suffixes_nodes,
+                                     key=SortByLowerCaseName)
+
+  for histogram_suffixes in sorted_histogram_suffixes:
+    xml.dom.minidom._append_child(combined_histogram_suffixes_list,
+                                  histogram_suffixes)
+
+  return [combined_histograms, combined_histogram_suffixes_list]
 
 
 def MakeNodeWithChildren(doc, tag, children):
@@ -129,9 +150,9 @@ def MergeTrees(trees):
           # This can result in the merged document having multiple <enums> and
           # similar sections, but scripts ignore these anyway.
           GetEnumsNodes(doc, trees) +
-          # Sort the histograms by name and return a single <histograms> node.
-          CombineHistogramsSorted(doc, trees) +
-          GetElementsByTagName(trees, 'histogram_suffixes_list')))
+          # Sort the <histogram> and <histogram_suffixes> nodes by name and
+          # return the combined nodes.
+          CombineHistogramsSorted(doc, trees)))
   # After using the unsafe version of appendChild, we see a regression when
   # pretty-printing the merged |doc|. This might because the unsafe appendChild
   # doesn't build indexes for later lookup. And thus, we need to convert the

tools/metrics/histograms/merge_xml_test.py

@@ -12,8 +12,10 @@ class MergeXmlTest(unittest.TestCase):
 
   def testMergeFiles(self):
     """Checks that enums.xml and histograms.xml can merge successfully."""
-    merged = merge_xml.PrettyPrintMergedFiles(
-        [histogram_paths.TEST_ENUMS_XML, histogram_paths.TEST_HISTOGRAMS_XML])
+    merged = merge_xml.PrettyPrintMergedFiles([
+        histogram_paths.TEST_ENUMS_XML, histogram_paths.TEST_HISTOGRAMS_XML,
+        histogram_paths.TEST_SUFFIXES_XML
+    ])
     # If ukm.xml is not provided, there is no need to populate the
     # UkmEventNameHash enum.
     expected_merged_xml = """
@@ -84,6 +86,12 @@ class MergeXmlTest(unittest.TestCase):
 
 <histogram_suffixes_list>
 
+<histogram_suffixes name="Test.EnumHistogramSuffixes" separator="."
+    ordering="prefix,2">
+  <suffix name="TestEnumSuffix" label="The enum histogram_suffixes"/>
+  <affected-histogram name="Test.EnumHistogram"/>
+</histogram_suffixes>
+
 <histogram_suffixes name="Test.HistogramSuffixes" separator=".">
   <suffix name="TestSuffix" label="A histogram_suffixes"/>
   <affected-histogram name="Test.Histogram"/>
@@ -177,6 +185,12 @@ class MergeXmlTest(unittest.TestCase):
 
 <histogram_suffixes_list>
 
+<histogram_suffixes name="Test.EnumHistogramSuffixes" separator="."
+    ordering="prefix,2">
+  <suffix name="TestEnumSuffix" label="The enum histogram_suffixes"/>
+  <affected-histogram name="Test.EnumHistogram"/>
+</histogram_suffixes>
+
 <histogram_suffixes name="Test.HistogramSuffixes" separator=".">
   <suffix name="TestSuffix" label="A histogram_suffixes"/>
   <affected-histogram name="Test.Histogram"/>

tools/metrics/histograms/test_data/histogram_suffixes_list.xml

+<histogram-configuration>
+
+<histogram_suffixes_list>
+
+<histogram_suffixes name="Test.EnumHistogramSuffixes" separator="."
+    ordering="prefix,2">
+  <suffix name="TestEnumSuffix" label="The enum histogram_suffixes"/>
+  <affected-histogram name="Test.EnumHistogram"/>
+</histogram_suffixes>
+
+</histogram_suffixes_list>
+
+</histogram-configuration>
\ No newline at end of file