New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 798605 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jan 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug



Sign in to add a comment

Break up the "unknown or other" bucket in FeatureState metric

Project Member Reported by hansberry@chromium.org, Jan 3 2018

Issue description

Labels: M-65
Owner: khorimoto@chromium.org
Status: Started (was: Available)
Project Member

Comment 2 by bugdroid1@chromium.org, Jan 5 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a34786e1db20e381158a697c052c0aab2f85285e

commit a34786e1db20e381158a697c052c0aab2f85285e
Author: Kyle Horimoto <khorimoto@google.com>
Date: Fri Jan 05 22:41:06 2018

[CrOS Tether] Improve the InstantTethering.FeatureState metric.

Previously, nearly half of our reported metrics were in the
OTHER_OR_UNKNOWN category, which negated much of the value of this
metric.

This CL splits that category into two: SUSPENDED and SHUT_DOWN.
  * SUSPENDED is logged when the device has been put to sleep (i.e., the
    laptop lid has been closed).
  * SHUT_DOWN is now deprecated. The FeatureState metric was meant to
    provide insight into how the Tether component was being used, but
    SHUT_DOWN does not provide any insights since the Tether component
    is always eventually shut down any time the user logs out,
    regardless of any usage patterns. Thus, without this change, we
    would expect nearly half of all logs to this metric to be for
    SHUT_DOWN, which dilutes the impact of meaningful logs.

Bug:  798605 , 672263
Change-Id: Ife9dd1183cea77120e2b153369be591fe806dec1
Reviewed-on: https://chromium-review.googlesource.com/849252
Reviewed-by: Steven Holte <holte@chromium.org>
Reviewed-by: Ryan Hansberry <hansberry@chromium.org>
Commit-Queue: Kyle Horimoto <khorimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#527412}
[modify] https://crrev.com/a34786e1db20e381158a697c052c0aab2f85285e/chrome/browser/chromeos/tether/tether_service.cc
[modify] https://crrev.com/a34786e1db20e381158a697c052c0aab2f85285e/chrome/browser/chromeos/tether/tether_service.h
[modify] https://crrev.com/a34786e1db20e381158a697c052c0aab2f85285e/chrome/browser/chromeos/tether/tether_service_unittest.cc
[modify] https://crrev.com/a34786e1db20e381158a697c052c0aab2f85285e/tools/metrics/histograms/enums.xml

Status: Fixed (was: Started)
Project Member

Comment 4 by bugdroid1@chromium.org, Jan 8 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4c741abe37a9f67f01be917a5ca36f6c788d5e0d

commit 4c741abe37a9f67f01be917a5ca36f6c788d5e0d
Author: Alice Boxhall <aboxhall@chromium.org>
Date: Mon Jan 08 04:00:02 2018

Revert "[CrOS Tether] Improve the InstantTethering.FeatureState metric."

This reverts commit a34786e1db20e381158a697c052c0aab2f85285e.

Reason for revert: Seems to have caused a test failure: https://logs.chromium.org/v/?s=chromium%2Fbb%2Fchromium.memory%2FLinux_Chromium_OS_ASan_LSan_Tests__1_%2F25561%2F%2B%2Frecipes%2Fsteps%2Funit_tests%2F0%2Flogs%2FTetherServiceTest.TestFeatureFlagDisabled%2F0

First failing build: https://uberchromegw.corp.google.com/i/chromium.memory/builders/Linux%20Chromium%20OS%20ASan%20LSan%20Tests%20%281%29/builds/25561

Original change's description:
> [CrOS Tether] Improve the InstantTethering.FeatureState metric.
> 
> Previously, nearly half of our reported metrics were in the
> OTHER_OR_UNKNOWN category, which negated much of the value of this
> metric.
> 
> This CL splits that category into two: SUSPENDED and SHUT_DOWN.
>   * SUSPENDED is logged when the device has been put to sleep (i.e., the
>     laptop lid has been closed).
>   * SHUT_DOWN is now deprecated. The FeatureState metric was meant to
>     provide insight into how the Tether component was being used, but
>     SHUT_DOWN does not provide any insights since the Tether component
>     is always eventually shut down any time the user logs out,
>     regardless of any usage patterns. Thus, without this change, we
>     would expect nearly half of all logs to this metric to be for
>     SHUT_DOWN, which dilutes the impact of meaningful logs.
> 
> Bug:  798605 , 672263
> Change-Id: Ife9dd1183cea77120e2b153369be591fe806dec1
> Reviewed-on: https://chromium-review.googlesource.com/849252
> Reviewed-by: Steven Holte <holte@chromium.org>
> Reviewed-by: Ryan Hansberry <hansberry@chromium.org>
> Commit-Queue: Kyle Horimoto <khorimoto@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#527412}

TBR=khorimoto@chromium.org,holte@chromium.org,hansberry@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  798605 , 672263
Change-Id: Icee0309eb693ce5f859beac36e1bb5a2228ba501
Reviewed-on: https://chromium-review.googlesource.com/853594
Reviewed-by: Alice Boxhall <aboxhall@chromium.org>
Commit-Queue: Alice Boxhall <aboxhall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#527564}
[modify] https://crrev.com/4c741abe37a9f67f01be917a5ca36f6c788d5e0d/chrome/browser/chromeos/tether/tether_service.cc
[modify] https://crrev.com/4c741abe37a9f67f01be917a5ca36f6c788d5e0d/chrome/browser/chromeos/tether/tether_service.h
[modify] https://crrev.com/4c741abe37a9f67f01be917a5ca36f6c788d5e0d/chrome/browser/chromeos/tether/tether_service_unittest.cc
[modify] https://crrev.com/4c741abe37a9f67f01be917a5ca36f6c788d5e0d/tools/metrics/histograms/enums.xml

Status: Started (was: Fixed)
Project Member

Comment 6 by bugdroid1@chromium.org, Jan 10 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0c68ee356dff1939be1fddcc30d4bf776d5c1e7c

commit 0c68ee356dff1939be1fddcc30d4bf776d5c1e7c
Author: Kyle Horimoto <khorimoto@google.com>
Date: Wed Jan 10 23:33:56 2018

[CrOS Tether] Improve the InstantTethering.FeatureState metric.

This CL relands a reverted CL.
 Original: https://chromium-review.googlesource.com/c/chromium/src/+/849252
 Revert  : https://chromium-review.googlesource.com/c/chromium/src/+/853594

The original CL is uploaded as patchset 1, and the updated CL is
uploaded as patchset 2. To fix the test, I only check the FeatureState
metric in TearDown() if the service was actually created during the test.


Original CL description is below:

Previously, nearly half of our reported metrics were in the
OTHER_OR_UNKNOWN category, which negated much of the value of this
metric.

This CL splits that category into two: SUSPENDED and SHUT_DOWN.
  * SUSPENDED is logged when the device has been put to sleep (i.e., the
    laptop lid has been closed).
  * SHUT_DOWN is now deprecated. The FeatureState metric was meant to
    provide insight into how the Tether component was being used, but
    SHUT_DOWN does not provide any insights since the Tether component
    is always eventually shut down any time the user logs out,
    regardless of any usage patterns. Thus, without this change, we
    would expect nearly half of all logs to this metric to be for
    SHUT_DOWN, which dilutes the impact of meaningful logs.

Bug:  798605 , 672263
Change-Id: If99a054d815d23319749f84df931743f1c7e55a0
TBR: holte@chromium.org
Reviewed-on: https://chromium-review.googlesource.com/861045
Commit-Queue: Kyle Horimoto <khorimoto@chromium.org>
Reviewed-by: Jeremy Klein <jlklein@chromium.org>
Cr-Commit-Position: refs/heads/master@{#528478}
[modify] https://crrev.com/0c68ee356dff1939be1fddcc30d4bf776d5c1e7c/chrome/browser/chromeos/tether/tether_service.cc
[modify] https://crrev.com/0c68ee356dff1939be1fddcc30d4bf776d5c1e7c/chrome/browser/chromeos/tether/tether_service.h
[modify] https://crrev.com/0c68ee356dff1939be1fddcc30d4bf776d5c1e7c/chrome/browser/chromeos/tether/tether_service_unittest.cc
[modify] https://crrev.com/0c68ee356dff1939be1fddcc30d4bf776d5c1e7c/tools/metrics/histograms/enums.xml

Status: Fixed (was: Started)

Sign in to add a comment