[Passwords Import] Add metrics for notes import

Bug: 1383938
Change-Id: Ic6ad8e3283f70c98cf2458f687e2fd8a0d1fec11
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4255325
Reviewed-by: Mohamed Amir Yosef <mamir@chromium.org>
Commit-Queue: Andrii Natiahlyi <natiahlyi@google.com>
Cr-Commit-Position: refs/heads/main@{#1107382}

components/password_manager/core/browser/import/password_importer.cc

@@ -332,6 +332,11 @@ void PasswordImporter::ConsumePasswords(
     return;
   }
 
+  size_t notes_per_file_count = 0;
+  size_t notes_duplicates_per_file_count = 0;
+  size_t notes_substrings_per_file_count = 0;
+  size_t notes_concatenations_per_file_count = 0;
+
   // TODO(crbug/1325290): Either move to earlier point or update histogram.
   base::Time start_time = base::Time::Now();
   std::vector<password_manager::CredentialUIEntry> credentials;
@@ -353,9 +358,8 @@ void PasswordImporter::ConsumePasswords(
       return false;
     }
 
-    // TODO(crbug.com/1383938): Add metric for total number of credential with a
-    // note found in the file. Increment a variable here, emit to the histogram
-    // after the loop.
+    notes_per_file_count++;
+
     auto forms = presenter_->GetCorrespondingPasswordForms(credential);
     if (forms.empty()) {
       // No matching local credentials.
@@ -364,16 +368,12 @@ void PasswordImporter::ConsumePasswords(
 
     auto local_credential = CredentialUIEntry(forms);
     if (local_credential.note == imported_note) {
-      // Duplicate is not considered to be a conflict.
-      // TODO(crbug.com/1383938): Add metric – note resolved – duplicate.
+      notes_duplicates_per_file_count++;
       return false;
     }
 
     if (local_credential.note.find(imported_note) != std::u16string::npos) {
-      // Don't proceed with the concatenation if the local note contains the
-      // imported note as a substring.
-      // TODO(crbug.com/1383938): Add metric – note resolved – imported note is
-      // a substring of local note.
+      notes_substrings_per_file_count++;
       return false;
     }
 
@@ -382,9 +382,6 @@ void PasswordImporter::ConsumePasswords(
       // Notes concatenation size should not exceed 1000 characters.
       results.failed_imports.push_back(CreateFailedImportEntry(
           credential, ImportEntry::Status::LONG_CONCATENATED_NOTE));
-      // TODO(crbug.com/1383938): Add metric – note resolved – notes
-      // concatenation is too long. Error reported. Imported credential doesn't
-      // require further processing.
       return true;
     }
 
@@ -399,7 +396,7 @@ void PasswordImporter::ConsumePasswords(
     // Otherwise, accumulate credentials that need to be edited and ideally do
     // updates as a bulk operation.
     presenter_->EditSavedCredentials(local_credential, updated_credential);
-    // TODO(crbug.com/1383938): Add metric – note resolved – notes concatenated.
+    notes_concatenations_per_file_count++;
     results.number_imported++;
 
     // Matching local credentials were updated with notes concatenation.
@@ -430,6 +427,18 @@ void PasswordImporter::ConsumePasswords(
     }
   }
 
+  base::UmaHistogramCounts1000(
+      "PasswordManager.Import.PerFile.Notes.TotalCount", notes_per_file_count);
+  base::UmaHistogramCounts1000(
+      "PasswordManager.Import.PerFile.Notes.Concatenations",
+      notes_concatenations_per_file_count);
+  base::UmaHistogramCounts1000(
+      "PasswordManager.Import.PerFile.Notes.Duplicates",
+      notes_duplicates_per_file_count);
+  base::UmaHistogramCounts1000(
+      "PasswordManager.Import.PerFile.Notes.Substrings",
+      notes_substrings_per_file_count);
+
   // Pass `credentials` along with import results `results` to the callback
   // too since they are necessary to report which imports did actually fail.
   // (e.g. which url, username ...etc). Pass the import results (`results`)

components/password_manager/core/browser/import/password_importer_unittest.cc

@@ -189,6 +189,9 @@ TEST_F(PasswordImporterTest, CSVImportWithNote) {
             base::WriteFile(input_path, kTestCSVInput, strlen(kTestCSVInput)));
   ASSERT_NO_FATAL_FAILURE(StartImportAndWaitForCompletion(input_path));
 
+  histogram_tester.ExpectUniqueSample(
+      "PasswordManager.Import.PerFile.Notes.TotalCount", 1, 1);
+
   password_manager::ImportResults results = GetImportResults();
 
   EXPECT_EQ(1u, results.number_imported);
@@ -366,6 +369,9 @@ TEST_F(PasswordImporterTest, ExactMatchWithConflictingNotesValidConcatenation) {
                             kTestCSVInput.length()));
   ASSERT_NO_FATAL_FAILURE(StartImportAndWaitForCompletion(input_path));
 
+  histogram_tester.ExpectUniqueSample(
+      "PasswordManager.Import.PerFile.Notes.Concatenations", 1, 1);
+
   const password_manager::ImportResults& results = GetImportResults();
 
   ASSERT_EQ(0u, results.failed_imports.size());
@@ -405,6 +411,9 @@ TEST_F(PasswordImporterTest, ExactMatchImportedNoteIsSubstingOfLocalNote) {
                             kTestCSVInput.length()));
   ASSERT_NO_FATAL_FAILURE(StartImportAndWaitForCompletion(input_path));
 
+  histogram_tester.ExpectUniqueSample(
+      "PasswordManager.Import.PerFile.Notes.Substrings", 1, 1);
+
   const password_manager::ImportResults& results = GetImportResults();
 
   ASSERT_EQ(0u, results.failed_imports.size());
@@ -414,6 +423,8 @@ TEST_F(PasswordImporterTest, ExactMatchImportedNoteIsSubstingOfLocalNote) {
 }
 
 TEST_F(PasswordImporterTest, CSVImportExactMatchProfileStore) {
+  base::test::ScopedFeatureList feature_list{syncer::kPasswordNotesWithBackup};
+
   constexpr char kTestCSVInput[] =
       "Url,Username,Password,Comment\n"
       "https://"
@@ -444,6 +455,8 @@ TEST_F(PasswordImporterTest, CSVImportExactMatchProfileStore) {
       "PasswordManager.ImportedPasswordsPerUserInCSV", 1, 1);
   histogram_tester.ExpectUniqueSample(
       "PasswordManager.Import.PerFile.Duplicates", 1, 1);
+  histogram_tester.ExpectUniqueSample(
+      "PasswordManager.Import.PerFile.Notes.Duplicates", 1, 1);
 
   const password_manager::ImportResults& results = GetImportResults();
 

tools/metrics/histograms/metadata/password/histograms.xml

@@ -1711,6 +1711,31 @@ chromium-metrics-reviews@google.com.
   </token>
 </histogram>
 
+<histogram name="PasswordManager.Import.PerFile.Notes.{Type}" units="units"
+    expires_after="2023-09-15">
+  <owner>eliaskh@chromium.org</owner>
+  <owner>natiahlyi@google.com</owner>
+  <summary>
+    Tracks the number of {Type} per one imported CSV file with Google Password
+    Manager in Settings UI. Recorded during the passwords import process
+    triggered from settings, after parsing the input file.
+  </summary>
+  <token key="Type">
+    <variant name="Concatenations"
+        summary="imported notes that were concatenated with local notes of
+                 the same credential"/>
+    <variant name="Duplicates"
+        summary="imported notes that are duplicates of local notes of the
+                 same credential"/>
+    <variant name="Substrings"
+        summary="imported notes that are substrings of local notes of the
+                 same credential"/>
+    <variant name="TotalCount"
+        summary="valid notes (note's length is not greater than 1000
+                 characters)"/>
+  </token>
+</histogram>
+
 <histogram name="PasswordManager.Import.PerFile.{ErrorType}" units="units"
     expires_after="2023-09-15">
   <owner>eliaskh@chromium.org</owner>