Issue metadata
Sign in to add a comment
|
ReportSender's success callback should send response code |
||||||||||||||||||||||||
Issue descriptionThere might be cases where SafeBrowsing extended reporting server can send a 404 or other HTTP error code. Currently these call the success callback since there is no net error. Provide a response_code parameter in the success callback so that the caller can determine what to do with it (log UMA etc). Putting this under cert analysis component, for lack of a better one.
,
Apr 27 2017
There is a follow up item: Change cert reporter's histogram to record response codes.
,
May 6 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/19c49d0f04233c3a406d8171eddbe6209362ade5 commit 19c49d0f04233c3a406d8171eddbe6209362ade5 Author: meacer <meacer@chromium.org> Date: Sat May 06 19:21:41 2017 Add metrics for certificate report uploads When sending certificate reports, we suspect that some content filters, firewalls and captive portals delay the response indefinitely without sending back any response. Certificate uploads rely on a response to be received in order to decide whether to retry uploading the report, so this breaks those assumptions. This CL adds a histogram to understand how often this happens. In particular, a difference between submitted reports and the sum of failed and successful reports will indicate that some reports simply get lost. The histogram also provides insight about how often queued reports are dropped, which will help us fine tune the queue size and report time-to-live. A question that came up while discussing this histogram was whether it would be useful since UMA uploads can also be blocked in a similar way. The answer seems to be yes, as UMA retries metrics uploads with a different frequency, and it persists metrics uploads whereas cert reports are not persisted. BUG= 682933 Review-Url: https://codereview.chromium.org/2862453002 Cr-Commit-Position: refs/heads/master@{#469883} [modify] https://crrev.com/19c49d0f04233c3a406d8171eddbe6209362ade5/chrome/browser/safe_browsing/certificate_reporting_service.cc [modify] https://crrev.com/19c49d0f04233c3a406d8171eddbe6209362ade5/chrome/browser/safe_browsing/certificate_reporting_service.h [modify] https://crrev.com/19c49d0f04233c3a406d8171eddbe6209362ade5/chrome/browser/safe_browsing/certificate_reporting_service_browsertest.cc [modify] https://crrev.com/19c49d0f04233c3a406d8171eddbe6209362ade5/chrome/browser/safe_browsing/certificate_reporting_service_test_utils.cc [modify] https://crrev.com/19c49d0f04233c3a406d8171eddbe6209362ade5/chrome/browser/safe_browsing/certificate_reporting_service_test_utils.h [modify] https://crrev.com/19c49d0f04233c3a406d8171eddbe6209362ade5/chrome/browser/safe_browsing/certificate_reporting_service_unittest.cc [modify] https://crrev.com/19c49d0f04233c3a406d8171eddbe6209362ade5/tools/metrics/histograms/enums.xml [modify] https://crrev.com/19c49d0f04233c3a406d8171eddbe6209362ade5/tools/metrics/histograms/histograms.xml
,
May 7 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e57c36727b93afd024eb70f36994ba4c28f88446 commit e57c36727b93afd024eb70f36994ba4c28f88446 Author: avi <avi@chromium.org> Date: Sun May 07 16:54:04 2017 Revert "Add metrics for certificate report uploads" This reverts commit 19c49d0f04233c3a406d8171eddbe6209362ade5. BUG= 682933 TBR=meacer@chromium.org NOTRY=true This fails on multiple platforms. https://build.chromium.org/p/chromium.win/builders/Win7%20%2832%29%20Tests/builds/19591 https://build.chromium.org/p/chromium.memory/builders/Linux%20MSan%20Tests/builds/626 [ RUN ] CertificateReportingServiceBrowserTest.DontSendOldReports Xlib: extension "RANDR" missing on display ":99". [17271:17271:0506/143918.081251:WARNING:audio_manager.cc(293)] Multiple instances of AudioManager detected [17271:17271:0506/143918.081331:WARNING:audio_manager.cc(254)] Multiple instances of AudioManager detected [17271:17271:0506/143918.348153:WARNING:password_store_factory.cc(249)] Using basic (unencrypted) store for password storage. See https://chromium.googlesource.com/chromium/src/+/master/docs/linux_password_storage.md for more information about password storage options. [17271:17271:0506/143919.432896:WARNING:navigator_impl.cc(269)] Discarding message during interstitial. [17271:17271:0506/143919.692333:ERROR:navigation_entry_screenshot_manager.cc(134)] Invalid entry with unique id: 1 [17271:17271:0506/143923.020192:WARNING:navigator_impl.cc(269)] Discarding message during interstitial. [17271:17271:0506/143923.219670:ERROR:navigation_entry_screenshot_manager.cc(134)] Invalid entry with unique id: 5 [17271:17271:0506/143926.470176:WARNING:navigator_impl.cc(269)] Discarding message during interstitial. ../../base/test/histogram_tester.cc:155: Failure Value of: actual_count Actual: 2 Expected: expected_count Which is: 3 Histogram "SSL.CertificateErrorReportEvent" does not have the right number of samples (3) in the expected bucket (3). It has (2). ../../base/test/histogram_tester.cc:170: Failure Value of: actual_count Actual: 12 Expected: expected_count Which is: 13 Histogram "SSL.CertificateErrorReportEvent" does not have the right total number of samples (13). It has (12). [ FAILED ] CertificateReportingServiceBrowserTest.DontSendOldReports, where TypeParam = and GetParam() = (10536 ms) Review-Url: https://codereview.chromium.org/2861323002 Cr-Commit-Position: refs/heads/master@{#469900} [modify] https://crrev.com/e57c36727b93afd024eb70f36994ba4c28f88446/chrome/browser/safe_browsing/certificate_reporting_service.cc [modify] https://crrev.com/e57c36727b93afd024eb70f36994ba4c28f88446/chrome/browser/safe_browsing/certificate_reporting_service.h [modify] https://crrev.com/e57c36727b93afd024eb70f36994ba4c28f88446/chrome/browser/safe_browsing/certificate_reporting_service_browsertest.cc [modify] https://crrev.com/e57c36727b93afd024eb70f36994ba4c28f88446/chrome/browser/safe_browsing/certificate_reporting_service_test_utils.cc [modify] https://crrev.com/e57c36727b93afd024eb70f36994ba4c28f88446/chrome/browser/safe_browsing/certificate_reporting_service_test_utils.h [modify] https://crrev.com/e57c36727b93afd024eb70f36994ba4c28f88446/chrome/browser/safe_browsing/certificate_reporting_service_unittest.cc [modify] https://crrev.com/e57c36727b93afd024eb70f36994ba4c28f88446/tools/metrics/histograms/enums.xml [modify] https://crrev.com/e57c36727b93afd024eb70f36994ba4c28f88446/tools/metrics/histograms/histograms.xml
,
Nov 10 2017
,
Nov 22 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/422819099093c29b50c33498e873724b9be8d3c9 commit 422819099093c29b50c33498e873724b9be8d3c9 Author: Mustafa Emre Acer <meacer@chromium.org> Date: Wed Nov 22 23:10:20 2017 Reland of Add metrics for certificate report uploads One of the tests changed by this CL was flaky (CertificateReportingServiceBrowserTest. DontSendOldReports). This is now fixed, so relanding the CL. Original CL: https://codereview.chromium.org/2862453002 When sending certificate reports, we suspect that some content filters, firewalls and captive portals delay the response indefinitely without sending back any response. Certificate uploads rely on a response to be received in order to decide whether to retry uploading the report, so this breaks those assumptions. This CL adds a histogram to understand how often this happens. In particular, a difference between submitted reports and the sum of failed and successful reports will indicate that some reports simply get lost. The histogram also provides insight about how often queued reports are dropped, which will help us fine tune the queue size and report time-to-live. A question that came up while discussing this histogram was whether it would be useful since UMA uploads can also be blocked in a similar way. The answer seems to be yes, as UMA retries metrics uploads with a different frequency, and it persists metrics uploads whereas cert reports are not persisted. Bug: 682933 Change-Id: I09616eaf04854157ddeff8d20d39e18afc705967 Reviewed-on: https://chromium-review.googlesource.com/782642 Commit-Queue: Mustafa Emre Acer <meacer@chromium.org> Reviewed-by: Ilya Sherman <isherman@chromium.org> Reviewed-by: Emily Stark <estark@chromium.org> Reviewed-by: Jialiu Lin <jialiul@chromium.org> Cr-Commit-Position: refs/heads/master@{#518789} [modify] https://crrev.com/422819099093c29b50c33498e873724b9be8d3c9/chrome/browser/safe_browsing/certificate_reporting_service.cc [modify] https://crrev.com/422819099093c29b50c33498e873724b9be8d3c9/chrome/browser/safe_browsing/certificate_reporting_service.h [modify] https://crrev.com/422819099093c29b50c33498e873724b9be8d3c9/chrome/browser/safe_browsing/certificate_reporting_service_browsertest.cc [modify] https://crrev.com/422819099093c29b50c33498e873724b9be8d3c9/chrome/browser/safe_browsing/certificate_reporting_service_test_utils.cc [modify] https://crrev.com/422819099093c29b50c33498e873724b9be8d3c9/chrome/browser/safe_browsing/certificate_reporting_service_test_utils.h [modify] https://crrev.com/422819099093c29b50c33498e873724b9be8d3c9/chrome/browser/safe_browsing/certificate_reporting_service_unittest.cc [modify] https://crrev.com/422819099093c29b50c33498e873724b9be8d3c9/tools/metrics/histograms/enums.xml [modify] https://crrev.com/422819099093c29b50c33498e873724b9be8d3c9/tools/metrics/histograms/histograms.xml
,
Nov 22 2017
,
Mar 9 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8220c880a589ae11da608a05c25ea854f9314320 commit 8220c880a589ae11da608a05c25ea854f9314320 Author: Mustafa Emre Acer <meacer@chromium.org> Date: Fri Mar 09 00:39:54 2018 Fix histogram entries for certificate reports. The histograms were added in https://crrev.googlesource.com/c/782642 but the histograms.xml and enums.xml didn't match the actual histogram and enum names. This CL fixes that. Bug: 682933 Change-Id: Id0447f7e16032dae16d45781de07bacbd96a2ad6 Reviewed-on: https://chromium-review.googlesource.com/956343 Reviewed-by: Ilya Sherman <isherman@chromium.org> Commit-Queue: Ilya Sherman <isherman@chromium.org> Cr-Commit-Position: refs/heads/master@{#541960} [modify] https://crrev.com/8220c880a589ae11da608a05c25ea854f9314320/tools/metrics/histograms/enums.xml [modify] https://crrev.com/8220c880a589ae11da608a05c25ea854f9314320/tools/metrics/histograms/histograms.xml |
|||||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||||
Comment 1 by bugdroid1@chromium.org
, Apr 27 2017