Added validate checks for distributed histograms

Upon presubmit, will perform the following checks if histograms.xml,
obsolete_histograms.xml or enums.xml is changed.

- Validate prefix and pretty-print if histograms.xml is changed.
- Pretty-print if enums.xml is changed.
- Validate histograms contained are all obsolete if obsolete_histograms.xml
is changed.

Amended relevant files to take in relative path to a distributed histograms
directory as system argument.

Bug: 692201
Change-Id: I1797c0b65b53462cbb73820d8416a28651d1c65e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2359714
Commit-Queue: Weilun Shi <sweilun@chromium.org>
Auto-Submit: Weilun Shi <sweilun@chromium.org>
Reviewed-by: Steven Holte <holte@chromium.org>
Cr-Commit-Position: refs/heads/master@{#800378}

tools/metrics/common/presubmit_util.py

@@ -27,7 +27,7 @@ def DoPresubmit(argv,
 
   Args:
     argv: command line arguments
-    original_filename: The filename to read from.
+    original_filename: The path to the file to read from.
     backup_filename: When pretty printing, move the old file contents here.
     prettyFn: A function which takes the original xml content and produces
         pretty printed xml.

tools/metrics/histograms/PRESUBMIT.py

@@ -8,37 +8,112 @@ for more details on the presubmit API built into depot_tools.
 """
 
 
-def ValidationNeeded(input_api):
-  """Check if validation of histograms.xml files are required."""
-  for f in input_api.AffectedTextFiles():
-    p = f.AbsoluteLocalPath()
-    if (input_api.basename(p) in {'histograms.xml', 'enums.xml'} and
-        input_api.os_path.dirname(p) == input_api.PresubmitLocalPath()):
-      return True
+def GetPrettyPrintErrors(input_api, output_api, cwd, rel_path, results):
+  """Runs pretty-print command for specified file."""
+  exit_code = input_api.subprocess.call(
+      [input_api.python_executable, 'pretty_print.py', rel_path, '--presubmit'],
+      cwd=cwd)
+
+  if exit_code != 0:
+    error_msg = (
+        '%s is not formatted correctly,  please run pretty_print.py %s to fix.'
+        % (rel_path, rel_path))
+    results.append(output_api.PresubmitError(error_msg))
+
+
+def GetPrefixErrors(input_api, output_api, cwd, rel_path, results):
+  """Validates histogram prefixes in specified file."""
+  exit_code = input_api.subprocess.call(
+      [input_api.python_executable, 'validate_prefix.py', rel_path], cwd=cwd)
+
+  if exit_code != 0:
+    error_msg = ('%s contains histogram(s) with disallowed prefix, please run '
+                 'validate_prefix.py %s to fix.' % (rel_path, rel_path))
+    results.append(output_api.PresubmitError(error_msg))
+
+
+def GetObsoleteXmlErrors(input_api, output_api, cwd, results):
+  """Validates all histograms in the file are obsolete."""
+  exit_code = input_api.subprocess.call(
+      [input_api.python_executable, 'validate_obsolete_histograms.py'], cwd=cwd)
+
+  if exit_code != 0:
+    error_msg = (
+        'histograms_xml/obsolete_histograms.xml contains non-obsolete '
+        'histograms, please run validate_obsolete_histograms.py to fix.')
+    results.append(output_api.PresubmitError(error_msg))
+
+
+def GetValidateFormatErrors(input_api, output_api, cwd, results):
+  """Validates the format of merged histograms and enums."""
+  exit_code = input_api.subprocess.call(
+      [input_api.python_executable, 'validate_format.py'], cwd=cwd)
+
+  if exit_code != 0:
+    error_msg = (
+        'Histograms are not well-formatted; please run %s/validate_format.py '
+        'and fix the reported errors.' % cwd)
+    results.append(output_api.PresubmitError(error_msg))
+
+
+def ValidateSingleFile(input_api, output_api, file_obj, cwd, results):
+  """Does corresponding validations if histograms.xml or enums.xml is changed.
+
+  Args:
+    input_api: An input_api instance that contains information about changes.
+    output_api: An output_api instance to create results of the PRESUBMIT check.
+    file_obj: A file object of one of the changed files.
+    cwd: Path to current working directory.
+    results: The returned variable which is a list of output_api results.
+
+  Returns:
+    A boolean that True if a histograms.xml or enums.xml file is changed.
+  """
+  p = file_obj.AbsoluteLocalPath()
+  # Only do PRESUBMIT checks when |p| is under |cwd|.
+  if input_api.os_path.commonprefix([p, cwd]) != cwd:
+    return False
+  filepath = input_api.os_path.relpath(p, cwd)
+
+  # If the changed file is obsolete_histograms.xml, validate all histograms
+  # inside are obsolete.
+  if 'obsolete_histograms.xml' in filepath:
+    GetObsoleteXmlErrors(input_api, output_api, cwd, results)
+    # Return false here because we don't need to validate format if users only
+    # change obsolete_histograms.xml.
+    return False
+
+  # If the changed file is histograms.xml, pretty-print and validate prefix it.
+  elif 'histograms.xml' in filepath:
+    GetPrettyPrintErrors(input_api, output_api, cwd, filepath, results)
+    GetPrefixErrors(input_api, output_api, cwd, filepath, results)
+    return True
+
+  # If the changed file is enums.xml, pretty-print it.
+  elif 'enums.xml' in filepath:
+    GetPrettyPrintErrors(input_api, output_api, cwd, filepath, results)
+    return True
+
   return False
 
 
 def CheckChange(input_api, output_api):
   """Checks that histograms.xml is pretty-printed and well-formatted."""
   results = []
-  if ValidationNeeded(input_api):
-    cwd = input_api.PresubmitLocalPath()
-
-    exit_code = input_api.subprocess.call(
-        [input_api.python_executable, 'pretty_print.py', '--presubmit',
-         '--non-interactive'],
-        cwd=cwd)
-    if exit_code != 0:
-      results.append(output_api.PresubmitError(
-          'histograms.xml is not formatted correctly; please run '
-          'git cl format %s to fix.' % cwd))
-
-    exit_code = input_api.subprocess.call(
-        [input_api.python_executable, 'validate_format.py'], cwd=cwd)
-    if exit_code != 0:
-      results.append(output_api.PresubmitError(
-          'histograms.xml is not well formatted; run %s/validate_format.py '
-          'and fix the reported errors.' % cwd))
+  cwd = input_api.PresubmitLocalPath()
+  xml_changed = False
+
+  # Only for changed files, do corresponding checks if the file is
+  # histograms.xml, enums.xml or obsolete_histograms.xml.
+  for file_obj in input_api.AffectedTextFiles():
+    is_changed = ValidateSingleFile(
+        input_api, output_api, file_obj, cwd, results)
+    xml_changed = xml_changed or is_changed
+
+  # Run validate_format.py which checks for merge errors, if changed
+  # files contain histograms.xml or enums.xml.
+  if xml_changed:
+    GetValidateFormatErrors(input_api, output_api, cwd, results)
 
   return results
 

tools/metrics/histograms/histogram_paths.py

@@ -38,6 +38,10 @@ OBSOLETE_XML_RELATIVE = ('tools/metrics/histograms/histograms_xml/'
 ALL_XMLS_RELATIVE = [ENUMS_XML_RELATIVE, OBSOLETE_XML_RELATIVE
                      ] + HISTOGRAMS_XMLS_RELATIVE
 
+HISTOGRAMS_PREFIX_LIST = [
+    os.path.basename(os.path.dirname(f)) for f in HISTOGRAMS_XMLS_RELATIVE
+]
+
 ENUMS_XML = path_util.GetInputFile(ENUMS_XML_RELATIVE)
 UKM_XML = path_util.GetInputFile('tools/metrics/ukm/ukm.xml')
 HISTOGRAMS_XMLS = [path_util.GetInputFile(f) for f in HISTOGRAMS_XMLS_RELATIVE]

tools/metrics/histograms/pretty_print.py

@@ -14,6 +14,7 @@ and wrapping text nodes, so we implement our own full custom XML pretty-printer.
 
 from __future__ import with_statement
 
+import argparse
 import logging
 import os
 import shutil
@@ -170,12 +171,47 @@ def PrettyPrintEnums(raw_xml):
   return top_level_content + formatted_xml
 
 def main():
-  status1 = presubmit_util.DoPresubmit(
-      sys.argv, 'enums.xml', 'enums.before.pretty-print.xml', PrettyPrintEnums)
-  status2 = presubmit_util.DoPresubmit(sys.argv, 'histograms.xml',
-                                       'histograms.before.pretty-print.xml',
-                                       PrettyPrintHistograms)
-  sys.exit(status1 or status2)
+  """Pretty-prints the histograms or enums xml file at given relative path.
+
+  Args:
+    filepath: The relative path to xml file.
+    --non-interactive: (Optional) Does not print log info messages and does not
+        prompt user to accept the diff.
+    --presubmit: (Optional) Simply prints a message if the input is not
+        formatted correctly instead of modifying the file.
+    --diff: (Optional) Prints diff to stdout rather than modifying the file.
+
+  Example usage:
+    pretty_print.py histograms_xml/Fingerprint/histograms.xml
+    pretty_print.py enums.xml
+  """
+  parser = argparse.ArgumentParser()
+  parser.add_argument('filepath', help="relative path to XML file")
+  # The following optional flags are used by common/presubmit_util.py
+  parser.add_argument('--non-interactive', action="store_true")
+  parser.add_argument('--presubmit', action="store_true")
+  parser.add_argument('--diff', action="store_true")
+  args = parser.parse_args()
+
+  status = 0
+  if 'enums.xml' in args.filepath:
+    status = presubmit_util.DoPresubmit(sys.argv, 'enums.xml',
+                                        'enums.before.pretty-print.xml',
+                                        PrettyPrintEnums)
+
+  elif 'histograms' in args.filepath:
+    # Specify the individual directory of histograms.xml or
+    # obsolete_histograms.xml.
+    status = presubmit_util.DoPresubmit(
+        sys.argv,
+        args.filepath,
+        # The backup filename should be
+        # 'path/to/histograms.before.pretty-print.xml'.
+        '.before.pretty-print.'.join(args.filepath.rsplit('.', 1)),
+        PrettyPrintHistograms)
+
+  sys.exit(status)
+
 
 if __name__ == '__main__':
   main()

tools/metrics/histograms/split_xml.py

@@ -13,6 +13,8 @@ import sys
 from xml.dom import minidom
 
 import histogram_configuration_model
+import histogram_paths
+
 
 # The default name of the histograms XML file.
 HISTOGRAMS_XML = 'histograms.xml'
@@ -95,7 +97,7 @@ def _CreateXMLFile(comments, parent_node_string, nodes, output_dir, filename):
     output_file.write(pretty_xml_string)
 
 
-def GetCamelName(histogram_node, depth=0):
+def _GetCamelName(histogram_node, depth=0):
   """Returns the first camelcase name part of the histogram node.
 
   Args:
@@ -137,6 +139,16 @@ def GetCamelName(histogram_node, depth=0):
     return name_part[0:start_index]
 
 
+def GetDirForNode(node):
+  """Returns the correct directory that the given |node| should be placed in."""
+  camel_name = _GetCamelName(node)
+  # Check if the directory of its prefix exists. Return the |camel_name| if the
+  # folder exists. Otherwise, this |node| should be placed in 'others' folder.
+  if camel_name in histogram_paths.HISTOGRAMS_PREFIX_LIST:
+    return camel_name
+  return 'others'
+
+
 def _AddHistogramsToDict(histogram_nodes, depth):
   """Adds histogram nodes to the corresponding keys of a dictionary.
 
@@ -150,7 +162,7 @@ def _AddHistogramsToDict(histogram_nodes, depth):
   """
   document_dict = {}
   for histogram in histogram_nodes:
-    name_part = GetCamelName(histogram, depth)
+    name_part = _GetCamelName(histogram, depth)
     if name_part not in document_dict:
       document_dict[name_part] = []
     document_dict[name_part].append(histogram)

tools/metrics/histograms/validate_format.py

@@ -8,9 +8,6 @@
 import os
 import sys
 
-sys.path.append(os.path.join(os.path.dirname(__file__), '..', 'common'))
-import path_util
-
 import extract_histograms
 import histogram_paths
 import merge_xml
@@ -22,4 +19,3 @@ def main():
 
 if __name__ == '__main__':
   main()
-

tools/metrics/histograms/validate_obsolete_histograms.py

+# Copyright 2020 The Chromium Authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+"""Verifies that all the histograms in obsolete histograms XML are obsolete."""
+
+import logging
+import os
+import sys
+import xml.dom.minidom
+
+import extract_histograms
+import histogram_paths
+import split_xml
+
+
+def ValidateObsoleteXml():
+  """Checks that all the histograms in the obsolete file are obsolete.
+
+  Returns:
+    True if at least a histogram is not obsolete, False otherwise.
+  """
+  has_obsolete_error = False
+  tree = xml.dom.minidom.parse(histogram_paths.OBSOLETE_XML)
+
+  for node in extract_histograms.IterElementsWithTag(tree, 'histogram', 3):
+    obsolete_tag_nodelist = node.getElementsByTagName('obsolete')
+    if len(obsolete_tag_nodelist) > 0:
+      continue
+
+    has_obsolete_error = True
+    # If histogram is not obsolete, find out the directory that it should be
+    # placed in.
+    correct_dir = split_xml.GetDirForNode(node)
+    histogram_name = node.getAttribute('name')
+
+    logging.error(
+        'Histogram of name %s is not obsolete, please move it to the '
+        'histograms_xml/%s directory.', histogram_name, correct_dir)
+
+  return has_obsolete_error
+
+
+if __name__ == '__main__':
+  sys.exit(ValidateObsoleteXml())

tools/metrics/histograms/validate_prefix.py

+# Copyright 2020 The Chromium Authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+"""Checks that the histograms and variants at given xml have correct prefix."""
+
+import logging
+import os
+import sys
+import xml.dom.minidom
+
+import extract_histograms
+import split_xml
+
+
+def ValidatePrefixInFile(xml_path):
+  """Validates that all <histogram> and <variants> are put in the correct file.
+
+  Args:
+    xml_path: The path to the histograms.xml file.
+
+  Returns:
+    A boolean that is True if at least a histogram has incorrect prefix, False
+        otherwise.
+  """
+  prefix = os.path.basename(os.path.dirname(xml_path))
+  has_prefix_error = False
+  tree = xml.dom.minidom.parse(xml_path)
+
+  for node in extract_histograms.IterElementsWithTag(tree, 'variants', 3):
+    correct_dir = split_xml.GetDirForNode(node)
+    if correct_dir != prefix:
+      variants_name = node.getAttribute('name')
+      logging.error(
+          'Variants of name %s is not placed in the correct directory, '
+          'please remove it from the histograms_xml/%s directory '
+          'and place it in the histograms_xml/%s directory.', variants_name,
+          prefix, correct_dir)
+      has_prefix_error = True
+
+  for node in extract_histograms.IterElementsWithTag(tree, 'histogram', 3):
+    correct_dir = split_xml.GetDirForNode(node)
+    if correct_dir != prefix:
+      histogram_name = node.getAttribute('name')
+      logging.error(
+          'Histogram of name %s is not placed in the correct directory, '
+          'please remove it from the histograms_xml/%s directory '
+          'and place it in the histograms_xml/%s directory.', histogram_name,
+          prefix, correct_dir)
+      has_prefix_error = True
+
+  return has_prefix_error
+
+
+def main():
+  """Checks that the histograms at given path have prefix that is the dir name.
+
+  Args:
+    sys.argv[1]: The relative path to xml file.
+
+  Example usage:
+    validate_prefix.py histograms_xml/Fingerprint/histograms.xml
+  """
+  if len(sys.argv) != 2:
+    sys.stderr.write('Usage: %s <rel-path-to-xml>' % sys.argv[0])
+    sys.exit(1)
+
+  xml_path = os.path.join(os.getcwd(), sys.argv[1])
+  prefix_error = ValidatePrefixInFile(xml_path)
+
+  sys.exit(prefix_error)
+
+
+if __name__ == '__main__':
+  main()