Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2966903002:
Specialize Superfish interstitial metrics (Closed)
DescriptionSpecialize Superfish interstitial metrics
Instead of recording interstitial metrics under "interstitial.ssl_*.*", for the
Superfish interstitial, record UMA under "interstitial.superfish.*".
BUG=736110
Review-Url: https://codereview.chromium.org/2966903002
Cr-Commit-Position: refs/heads/master@{#483874}
Committed: https://chromium.googlesource.com/chromium/src/+/b6ce7fd591fa6f6fc2ac031bb3e059344c6d17cb
Patch Set 1 #
Total comments: 2
Patch Set 2 : unify tests #
Total comments: 2
Patch Set 3 : string -> const char[] #
Messages
Total messages: 21 (9 generated)
The CQ bit was checked by estark@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
estark@chromium.org changed reviewers: + meacer@chromium.org
meacer, PTAL? This just separates interstitial metrics for superfish. Thanks!
On 2017/06/30 22:08:57, estark wrote: > meacer, PTAL? This just separates interstitial metrics for superfish. Thanks! I thought superfish interstitial wasn't bypassable, and there aren't any actions on the page. If so, would it make sense to add this as a bin to interstitial.ssl_error_handler instead? I have to confess I'm a bit confused by the multiple histograms (interstitial.ssl_error_handler.superfish in the previous CL and interstitial.superfish.* in this CL)
On 2017/06/30 22:21:41, meacer wrote: > On 2017/06/30 22:08:57, estark wrote: > > meacer, PTAL? This just separates interstitial metrics for superfish. Thanks! > > I thought superfish interstitial wasn't bypassable, and there aren't any actions > on the page. If so, would it make sense to add this as a bin to > interstitial.ssl_error_handler instead? Well, looks like that's what we already have, but under interstitial.ssl_error_handler.superfish. But my question about interactions still stands :) > > I have to confess I'm a bit confused by the multiple histograms > (interstitial.ssl_error_handler.superfish in the previous CL and > interstitial.superfish.* in this CL)
On 2017/06/30 22:21:41, meacer wrote: > On 2017/06/30 22:08:57, estark wrote: > > meacer, PTAL? This just separates interstitial metrics for superfish. Thanks! > > I thought superfish interstitial wasn't bypassable, and there aren't any actions > on the page. If so, would it make sense to add this as a bin to > interstitial.ssl_error_handler instead? > > I have to confess I'm a bit confused by the multiple histograms > (interstitial.ssl_error_handler.superfish in the previous CL and > interstitial.superfish.* in this CL) interstitial.ssl_error_handler.superfish is just where the error was classified as Superfish. What I'm most interested in from the interstitial.superfish.* metrics is the SHOW_LEARN_MORE (clicks on the Help Center link) and the extended reporting enable/disable. I can split out separate metrics for those, but it seemed convenient to just use separate histograms from normal SSL interstitials for everything.
On 2017/06/30 22:25:42, estark wrote: > On 2017/06/30 22:21:41, meacer wrote: > > On 2017/06/30 22:08:57, estark wrote: > > > meacer, PTAL? This just separates interstitial metrics for superfish. > Thanks! > > > > I thought superfish interstitial wasn't bypassable, and there aren't any > actions > > on the page. If so, would it make sense to add this as a bin to > > interstitial.ssl_error_handler instead? > > > > I have to confess I'm a bit confused by the multiple histograms > > (interstitial.ssl_error_handler.superfish in the previous CL and > > interstitial.superfish.* in this CL) > > interstitial.ssl_error_handler.superfish is just where the error was classified > as Superfish. Er, sorry, that was confusing phrasing. I originally added interstitial.ssl_error_handler.superfish to count how often certificate errors happened for Superfish chains. We could potentially deprecate it now if we add these new interstitial.superfish.* ones. What I'm most interested in from the interstitial.superfish.* > metrics is the SHOW_LEARN_MORE (clicks on the Help Center link) and the extended > reporting enable/disable. I can split out separate metrics for those, but it > seemed convenient to just use separate histograms from normal SSL interstitials > for everything.
On 2017/06/30 22:33:44, estark wrote: > On 2017/06/30 22:25:42, estark wrote: > > On 2017/06/30 22:21:41, meacer wrote: > > > On 2017/06/30 22:08:57, estark wrote: > > > > meacer, PTAL? This just separates interstitial metrics for superfish. > > Thanks! > > > > > > I thought superfish interstitial wasn't bypassable, and there aren't any > > actions > > > on the page. If so, would it make sense to add this as a bin to > > > interstitial.ssl_error_handler instead? > > > > > > I have to confess I'm a bit confused by the multiple histograms > > > (interstitial.ssl_error_handler.superfish in the previous CL and > > > interstitial.superfish.* in this CL) > > > > interstitial.ssl_error_handler.superfish is just where the error was > classified > > as Superfish. > > Er, sorry, that was confusing phrasing. I originally added > interstitial.ssl_error_handler.superfish to count how often certificate errors > happened for Superfish chains. We could potentially deprecate it now if we add > these new interstitial.superfish.* ones. > > What I'm most interested in from the interstitial.superfish.* > > metrics is the SHOW_LEARN_MORE (clicks on the Help Center link) and the > extended > > reporting enable/disable. I can split out separate metrics for those, but it > > seemed convenient to just use separate histograms from normal SSL > interstitials > > for everything. OK, sg. I thought there wasn't any interaction to be made on the interstitial but looks like there still is some.
https://codereview.chromium.org/2966903002/diff/1/chrome/browser/ssl/ssl_brow... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/2966903002/diff/1/chrome/browser/ssl/ssl_brow... chrome/browser/ssl/ssl_browser_tests.cc:5008: IN_PROC_BROWSER_TEST_F(SuperfishSSLUITest, SuperfishInterstitialUMA) { This test subsumes the one above (SuperfishSSLUITest.SuperfishInterstitial). Perhaps remove it, or just add the metrics check to the end of it?
The CQ bit was checked by estark@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2966903002/diff/1/chrome/browser/ssl/ssl_brow... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/2966903002/diff/1/chrome/browser/ssl/ssl_brow... chrome/browser/ssl/ssl_browser_tests.cc:5008: IN_PROC_BROWSER_TEST_F(SuperfishSSLUITest, SuperfishInterstitialUMA) { On 2017/06/30 22:40:28, meacer wrote: > This test subsumes the one above (SuperfishSSLUITest.SuperfishInterstitial). > Perhaps remove it, or just add the metrics check to the end of it? Done, just added the metrics checks to the original one.
Lgtm https://codereview.chromium.org/2966903002/diff/20001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/2966903002/diff/20001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_browser_tests.cc:4984: const std::string decision_histogram = "interstitial.superfish.decision"; nit: const char kDecisionHistogram[] instead? Same for interaction_histogram.
Thanks for the quick review! https://codereview.chromium.org/2966903002/diff/20001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/2966903002/diff/20001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_browser_tests.cc:4984: const std::string decision_histogram = "interstitial.superfish.decision"; On 2017/06/30 22:53:29, meacer wrote: > nit: const char kDecisionHistogram[] instead? Same for interaction_histogram. Done.
The CQ bit was checked by estark@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from meacer@chromium.org Link to the patchset: https://codereview.chromium.org/2966903002/#ps40001 (title: "string -> const char[]")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1498864849630890,
"parent_rev": "5f69f39126a7475f3f19ca8344894afb9c40c889", "commit_rev":
"b6ce7fd591fa6f6fc2ac031bb3e059344c6d17cb"}
Message was sent while issue was closed.
Description was changed from ========== Specialize Superfish interstitial metrics Instead of recording interstitial metrics under "interstitial.ssl_*.*", for the Superfish interstitial, record UMA under "interstitial.superfish.*". BUG=736110 ========== to ========== Specialize Superfish interstitial metrics Instead of recording interstitial metrics under "interstitial.ssl_*.*", for the Superfish interstitial, record UMA under "interstitial.superfish.*". BUG=736110 Review-Url: https://codereview.chromium.org/2966903002 Cr-Commit-Position: refs/heads/master@{#483874} Committed: https://chromium.googlesource.com/chromium/src/+/b6ce7fd591fa6f6fc2ac031bb3e0... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/b6ce7fd591fa6f6fc2ac031bb3e0... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
