Skip to content

Commit

Permalink
[Human presence] Consolidated metrics constants.
Browse files Browse the repository at this point in the history 
Human presence metrics constants are used from multiple files spread
across the ash codebase. This CL brings the constants all together into
one header that is referenced from each file.

Bug: b:233679833
Change-Id: I4ceb1dc55ce09656723ebf3d79f4719581e48d16
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3822066
Reviewed-by: Alex Newcomer <newcomer@chromium.org>
Commit-Queue: Michael Martis <martis@chromium.org>
Reviewed-by: Guoxing Zhao <charleszhao@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1034954}
  • Loading branch information
@martis-chromium
martis-chromium authored and Chromium LUCI CQ committed Aug 15, 2022
1 parent 9d7b376 commit 91be349
Show file tree
Hide file tree
Showing 7 changed files with 135 additions and 90 deletions.
1 change: 1 addition & 0 deletions ash/BUILD.gn
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1191,6 +1191,7 @@ component("ash") {
"system/holding_space/recent_files_bubble.h",
"system/holding_space/screen_captures_section.cc",
"system/holding_space/screen_captures_section.h",
"system/human_presence/human_presence_metrics.h",
"system/human_presence/human_presence_orientation_controller.cc",
"system/human_presence/human_presence_orientation_controller.h",
"system/human_presence/lock_on_leave_controller.cc",
Expand Down
51 changes: 51 additions & 0 deletions ash/system/human_presence/human_presence_metrics.h
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
// Copyright 2022 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.

#ifndef ASH_SYSTEM_HUMAN_PRESENCE_HUMAN_PRESENCE_METRICS_H_
#define ASH_SYSTEM_HUMAN_PRESENCE_HUMAN_PRESENCE_METRICS_H_

#include "base/time/time.h"

namespace ash {

// Use two namespaces to keep constant names legible.
namespace snooping_protection_metrics {

constexpr char kEnabledHistogramName[] =
"ChromeOS.HPS.SnoopingProtection.Enabled";
constexpr char kPositiveDurationHistogramName[] =
"ChromeOS.HPS.SnoopingProtection.Positive.Duration";
constexpr char kNegativeDurationHistogramName[] =
"ChromeOS.HPS.SnoopingProtection.Negative.Duration";
constexpr char kFlakeyHistogramName[] =
"ChromeOS.HPS.SnoopingProtection.FlakeyDetection";
constexpr char kNotificationSuppressionEnabledHistogramName[] =
"ChromeOS.HPS.SnoopingProtectionNotificationSuppression.Enabled";

// Number of buckets to log SnoopingProtection present result.
constexpr int kDurationNumBuckets = 100;

// Minimum value for the SnoopingProtection.Positive.Duration and
// SnoopingProtection.Negative.Duration.
constexpr base::TimeDelta kDurationMin = base::Seconds(1);

// Maximum value for SnoopingProtection.Positive.Duration; Longer than 1 hour is
// considered as 1 hour.
constexpr base::TimeDelta kPositiveDurationMax = base::Hours(1);

// Maximum value for SnoopingProtection.Negative.Duration; Longer than 1 day is
// considered as 1 day.
constexpr base::TimeDelta kNegativeDurationMax = base::Hours(24);

} // namespace snooping_protection_metrics

namespace quick_dim_metrics {

constexpr char kEnabledHistogramName[] = "ChromeOS.HPS.QuickDim.Enabled";

} // namespace quick_dim_metrics

} // namespace ash

#endif // ASH_SYSTEM_HUMAN_PRESENCE_HUMAN_PRESENCE_METRICS_H_
35 changes: 12 additions & 23 deletions ash/system/human_presence/snooping_protection_controller.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
#include "ash/public/cpp/session/session_observer.h"
#include "ash/session/session_controller_impl.h"
#include "ash/shell.h"
#include "ash/system/human_presence/human_presence_metrics.h"
#include "ash/system/human_presence/snooping_protection_notification_blocker.h"
#include "base/bind.h"
#include "base/callback.h"
Expand All @@ -29,19 +30,8 @@
#include "ui/message_center/message_center.h"

namespace ash {
namespace {
// Number of buckets to log SnoopingProtection present result.
constexpr int kSnoopingProtectionDurationNumBucket = 100;
// Minimum value for the SnoopingProtection.Positive.Duration and
// SnoopingProtection.Negative.Duration.
constexpr base::TimeDelta kSnoopingProtectionDurationMin = base::Seconds(1);
// Maximum value for SnoopingProtection.Positive.Duration; Longer than 1 hour is
// considered as 1 hour.
constexpr base::TimeDelta kSnoopingProtectionPositiveMax = base::Hours(1);
// Maximum value for SnoopingProtection.Negative.Duration; Longer than 1 day is
// considered as 1 day.
constexpr base::TimeDelta kSnoopingProtectionNegativeMax = base::Hours(24);
} // namespace

namespace metrics = ash::snooping_protection_metrics;

SnoopingProtectionController::SnoopingProtectionController()
: notification_blocker_(
Expand Down Expand Up @@ -351,8 +341,7 @@ void SnoopingProtectionController::UpdatePrefState() {

ReconfigureService(&new_state);
UpdateSnooperStatus(new_state);
base::UmaHistogramBoolean("ChromeOS.HPS.SnoopingProtection.Enabled",
pref_enabled);
base::UmaHistogramBoolean(metrics::kEnabledHistogramName, pref_enabled);
}

void SnoopingProtectionController::OnMinWindowExpired() {
Expand All @@ -374,15 +363,15 @@ void SnoopingProtectionController::LogPresenceWindow(bool was_present) {
last_presence_report_time_ = now;

if (was_present) {
base::UmaHistogramCustomTimes(
"ChromeOS.HPS.SnoopingProtection.Positive.Duration",
time_since_last_report, kSnoopingProtectionDurationMin,
kSnoopingProtectionPositiveMax, kSnoopingProtectionDurationNumBucket);
base::UmaHistogramCustomTimes(metrics::kPositiveDurationHistogramName,
time_since_last_report, metrics::kDurationMin,
metrics::kPositiveDurationMax,
metrics::kDurationNumBuckets);
} else {
base::UmaHistogramCustomTimes(
"ChromeOS.HPS.SnoopingProtection.Negative.Duration",
time_since_last_report, kSnoopingProtectionDurationMin,
kSnoopingProtectionNegativeMax, kSnoopingProtectionDurationNumBucket);
base::UmaHistogramCustomTimes(metrics::kNegativeDurationHistogramName,
time_since_last_report, metrics::kDurationMin,
metrics::kNegativeDurationMax,
metrics::kDurationNumBuckets);
}
}

Expand Down
110 changes: 53 additions & 57 deletions ash/system/human_presence/snooping_protection_controller_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
#include "ash/session/session_controller_impl.h"
#include "ash/session/test_session_controller_client.h"
#include "ash/shell.h"
#include "ash/system/human_presence/human_presence_metrics.h"
#include "ash/test/ash_test_base.h"
#include "base/test/metrics/histogram_tester.h"
#include "base/test/scoped_command_line.h"
Expand All @@ -27,23 +28,14 @@
namespace ash {
namespace {

namespace metrics = ash::snooping_protection_metrics;

// The minimum positive window length will be in the range of a few seconds.
// Here we define two windows that will surely be shorter and longer resp. than
// the positive window length.
constexpr base::TimeDelta kShortTime = base::Milliseconds(30);
constexpr base::TimeDelta kLongTime = base::Seconds(30);

// UMA metric names.
// TODO(b/233679833): Move these metrics names into a common file.
constexpr char kSnoopingProtectionEnabledName[] =
"ChromeOS.HPS.SnoopingProtection.Enabled";
constexpr char kSnoopingProtectionNegativeName[] =
"ChromeOS.HPS.SnoopingProtection.Negative.Duration";
constexpr char kSnoopingProtectionPositiveName[] =
"ChromeOS.HPS.SnoopingProtection.Positive.Duration";
constexpr char kSnoopingProtectionFlakeyDetectionName[] =
"ChromeOS.HPS.SnoopingProtection.FlakeyDetection";

// A fixture that provides access to a fake daemon and an instance of the
// controller hooked up to the test environment.
class SnoopingProtectionControllerTestBase : public NoSessionAshTestBase {
Expand Down Expand Up @@ -509,15 +501,15 @@ TEST_F(SnoopingProtectionControllerTestMetrics, EnableDisablePref) {
base::HistogramTester tester;

SimulateLogin();
tester.ExpectTotalCount(kSnoopingProtectionEnabledName, 0);
tester.ExpectTotalCount(metrics::kEnabledHistogramName, 0);

SetEnabledPref(true);
tester.ExpectBucketCount(kSnoopingProtectionEnabledName, 1, 1);
tester.ExpectTotalCount(kSnoopingProtectionEnabledName, 1);
tester.ExpectBucketCount(metrics::kEnabledHistogramName, 1, 1);
tester.ExpectTotalCount(metrics::kEnabledHistogramName, 1);

SetEnabledPref(false);
tester.ExpectBucketCount(kSnoopingProtectionEnabledName, 0, 1);
tester.ExpectTotalCount(kSnoopingProtectionEnabledName, 2);
tester.ExpectBucketCount(metrics::kEnabledHistogramName, 0, 1);
tester.ExpectTotalCount(metrics::kEnabledHistogramName, 2);
}

TEST_F(SnoopingProtectionControllerTestMetrics, Duration) {
Expand All @@ -530,32 +522,34 @@ TEST_F(SnoopingProtectionControllerTestMetrics, Duration) {
// The first HpsNotifyChanged will not log anything.
state.set_value(hps::HpsResult::POSITIVE);
controller_->OnHpsNotifyChanged(state);
tester.ExpectTotalCount(kSnoopingProtectionPositiveName, 0);
tester.ExpectTotalCount(kSnoopingProtectionNegativeName, 0);
tester.ExpectTotalCount(metrics::kPositiveDurationHistogramName, 0);
tester.ExpectTotalCount(metrics::kNegativeDurationHistogramName, 0);

task_environment()->FastForwardBy(kLongTime);

// Send UNKNOWN will log a kSnoopingProtectionPositiveName event.
// Send UNKNOWN will log a positive duration event.
state.set_value(hps::HpsResult::UNKNOWN);
controller_->OnHpsNotifyChanged(state);
tester.ExpectTimeBucketCount(kSnoopingProtectionPositiveName, kLongTime, 1);
tester.ExpectTotalCount(kSnoopingProtectionPositiveName, 1);
tester.ExpectTotalCount(kSnoopingProtectionNegativeName, 0);
tester.ExpectTimeBucketCount(metrics::kPositiveDurationHistogramName,
kLongTime, 1);
tester.ExpectTotalCount(metrics::kPositiveDurationHistogramName, 1);
tester.ExpectTotalCount(metrics::kNegativeDurationHistogramName, 0);

task_environment()->FastForwardBy(kLongTime);

// Send NEGATIVE a second time will not log anything.
state.set_value(hps::HpsResult::NEGATIVE);
controller_->OnHpsNotifyChanged(state);
tester.ExpectTotalCount(kSnoopingProtectionPositiveName, 1);
tester.ExpectTotalCount(kSnoopingProtectionNegativeName, 0);
tester.ExpectTotalCount(metrics::kPositiveDurationHistogramName, 1);
tester.ExpectTotalCount(metrics::kNegativeDurationHistogramName, 0);

// Send POSITIVE will log a kSnoopingProtectionNegativeName event.
// Send POSITIVE will log a negative duration event.
state.set_value(hps::HpsResult::POSITIVE);
controller_->OnHpsNotifyChanged(state);
tester.ExpectTimeBucketCount(kSnoopingProtectionNegativeName, kLongTime, 1);
tester.ExpectTotalCount(kSnoopingProtectionPositiveName, 1);
tester.ExpectTotalCount(kSnoopingProtectionNegativeName, 1);
tester.ExpectTimeBucketCount(metrics::kNegativeDurationHistogramName,
kLongTime, 1);
tester.ExpectTotalCount(metrics::kPositiveDurationHistogramName, 1);
tester.ExpectTotalCount(metrics::kNegativeDurationHistogramName, 1);
}

TEST_F(SnoopingProtectionControllerTestMetrics, ShutDownTest) {
Expand All @@ -568,31 +562,33 @@ TEST_F(SnoopingProtectionControllerTestMetrics, ShutDownTest) {
// The first HpsNotifyChanged will not log anything.
state.set_value(hps::HpsResult::POSITIVE);
controller_->OnHpsNotifyChanged(state);
tester.ExpectTotalCount(kSnoopingProtectionPositiveName, 0);
tester.ExpectTotalCount(kSnoopingProtectionNegativeName, 0);
tester.ExpectTotalCount(metrics::kPositiveDurationHistogramName, 0);
tester.ExpectTotalCount(metrics::kNegativeDurationHistogramName, 0);

task_environment()->FastForwardBy(kLongTime);

dbus_client_->Shutdown();
tester.ExpectTimeBucketCount(kSnoopingProtectionPositiveName, kLongTime, 1);
tester.ExpectTotalCount(kSnoopingProtectionPositiveName, 1);
tester.ExpectTotalCount(kSnoopingProtectionNegativeName, 0);
tester.ExpectTimeBucketCount(metrics::kPositiveDurationHistogramName,
kLongTime, 1);
tester.ExpectTotalCount(metrics::kPositiveDurationHistogramName, 1);
tester.ExpectTotalCount(metrics::kNegativeDurationHistogramName, 0);
dbus_client_->Restart();

// Send NEGATIVE will not log anything because of the shutdown.
state.set_value(hps::HpsResult::NEGATIVE);
controller_->OnHpsNotifyChanged(state);
tester.ExpectTotalCount(kSnoopingProtectionPositiveName, 1);
tester.ExpectTotalCount(kSnoopingProtectionNegativeName, 0);
tester.ExpectTotalCount(metrics::kPositiveDurationHistogramName, 1);
tester.ExpectTotalCount(metrics::kNegativeDurationHistogramName, 0);

task_environment()->FastForwardBy(kLongTime);

// Send POSITIVE will log a kSnoopingProtectionNegativeName event.
// Send POSITIVE will log a negative duration event.
state.set_value(hps::HpsResult::POSITIVE);
controller_->OnHpsNotifyChanged(state);
tester.ExpectTimeBucketCount(kSnoopingProtectionNegativeName, kLongTime, 1);
tester.ExpectTotalCount(kSnoopingProtectionPositiveName, 1);
tester.ExpectTotalCount(kSnoopingProtectionNegativeName, 1);
tester.ExpectTimeBucketCount(metrics::kNegativeDurationHistogramName,
kLongTime, 1);
tester.ExpectTotalCount(metrics::kPositiveDurationHistogramName, 1);
tester.ExpectTotalCount(metrics::kNegativeDurationHistogramName, 1);
}

TEST_F(SnoopingProtectionControllerTestMetrics, FlakeyDetection) {
Expand All @@ -605,46 +601,46 @@ TEST_F(SnoopingProtectionControllerTestMetrics, FlakeyDetection) {
// The first HpsNotifyChanged will not log anything.
state.set_value(hps::HpsResult::POSITIVE);
controller_->OnHpsNotifyChanged(state);
tester.ExpectTotalCount(kSnoopingProtectionPositiveName, 0);
tester.ExpectTotalCount(kSnoopingProtectionNegativeName, 0);
tester.ExpectTotalCount(kSnoopingProtectionFlakeyDetectionName, 0);
tester.ExpectTotalCount(metrics::kPositiveDurationHistogramName, 0);
tester.ExpectTotalCount(metrics::kNegativeDurationHistogramName, 0);
tester.ExpectTotalCount(metrics::kFlakeyHistogramName, 0);
EXPECT_TRUE(controller_->SnooperPresent());

task_environment()->FastForwardBy(kShortTime);
// Send NEGATIVE after a short period of time will log a flakey detection.
state.set_value(hps::HpsResult::NEGATIVE);
controller_->OnHpsNotifyChanged(state);
tester.ExpectTotalCount(kSnoopingProtectionPositiveName, 0);
tester.ExpectTotalCount(kSnoopingProtectionNegativeName, 0);
tester.ExpectTotalCount(kSnoopingProtectionFlakeyDetectionName, 1);
tester.ExpectTotalCount(metrics::kPositiveDurationHistogramName, 0);
tester.ExpectTotalCount(metrics::kNegativeDurationHistogramName, 0);
tester.ExpectTotalCount(metrics::kFlakeyHistogramName, 1);
EXPECT_TRUE(controller_->SnooperPresent());

task_environment()->FastForwardBy(kShortTime);
// Send NEGATIVE again after a short period of time will log another flakey
// detection.
state.set_value(hps::HpsResult::NEGATIVE);
controller_->OnHpsNotifyChanged(state);
tester.ExpectTotalCount(kSnoopingProtectionPositiveName, 0);
tester.ExpectTotalCount(kSnoopingProtectionNegativeName, 0);
tester.ExpectTotalCount(kSnoopingProtectionFlakeyDetectionName, 2);
tester.ExpectTotalCount(metrics::kPositiveDurationHistogramName, 0);
tester.ExpectTotalCount(metrics::kNegativeDurationHistogramName, 0);
tester.ExpectTotalCount(metrics::kFlakeyHistogramName, 2);
EXPECT_TRUE(controller_->SnooperPresent());

task_environment()->FastForwardBy(kLongTime);
state.set_value(hps::HpsResult::NEGATIVE);
controller_->OnHpsNotifyChanged(state);
tester.ExpectTotalCount(kSnoopingProtectionPositiveName, 1);
tester.ExpectTotalCount(kSnoopingProtectionNegativeName, 0);
tester.ExpectTotalCount(kSnoopingProtectionFlakeyDetectionName, 2);
tester.ExpectTotalCount(metrics::kPositiveDurationHistogramName, 1);
tester.ExpectTotalCount(metrics::kNegativeDurationHistogramName, 0);
tester.ExpectTotalCount(metrics::kFlakeyHistogramName, 2);
EXPECT_FALSE(controller_->SnooperPresent());

// Send POSITIVE after a short period of time will NOT log a flakey detection
// for now.
task_environment()->FastForwardBy(kShortTime);
state.set_value(hps::HpsResult::POSITIVE);
controller_->OnHpsNotifyChanged(state);
tester.ExpectTotalCount(kSnoopingProtectionPositiveName, 1);
tester.ExpectTotalCount(kSnoopingProtectionNegativeName, 1);
tester.ExpectTotalCount(kSnoopingProtectionFlakeyDetectionName, 2);
tester.ExpectTotalCount(metrics::kPositiveDurationHistogramName, 1);
tester.ExpectTotalCount(metrics::kNegativeDurationHistogramName, 1);
tester.ExpectTotalCount(metrics::kFlakeyHistogramName, 2);
EXPECT_TRUE(controller_->SnooperPresent());
}

Expand All @@ -659,22 +655,22 @@ TEST_F(SnoopingProtectionControllerTestMetrics,
// The first HpsNotifyChanged will not log anything.
state.set_value(hps::HpsResult::POSITIVE);
controller_->OnHpsNotifyChanged(state);
tester.ExpectTotalCount(kSnoopingProtectionFlakeyDetectionName, 0);
tester.ExpectTotalCount(metrics::kFlakeyHistogramName, 0);

// Changing Orientation will disable HpsNotify and make the Present state to
// be false.
task_environment()->FastForwardBy(kShortTime);
controller_->OnOrientationChanged(/*suitable_for_human_presence=*/false);
controller_->OnOrientationChanged(/*suitable_for_human_presence=*/true);
tester.ExpectTotalCount(kSnoopingProtectionFlakeyDetectionName, 0);
tester.ExpectTotalCount(metrics::kFlakeyHistogramName, 0);
EXPECT_FALSE(controller_->SnooperPresent());

// Send NEGATIVE after a short period of time will log a flakey detection
// under this specific situation because the OrientationChange already put the
// controller_->SnooperPresent state into false.
state.set_value(hps::HpsResult::NEGATIVE);
controller_->OnHpsNotifyChanged(state);
tester.ExpectTotalCount(kSnoopingProtectionFlakeyDetectionName, 0);
tester.ExpectTotalCount(metrics::kFlakeyHistogramName, 0);
}

} // namespace
Expand Down
6 changes: 4 additions & 2 deletions ash/system/human_presence/snooping_protection_notification_blocker.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
#include "ash/session/session_controller_impl.h"
#include "ash/shell.h"
#include "ash/strings/grit/ash_strings.h"
#include "ash/system/human_presence/human_presence_metrics.h"
#include "ash/system/human_presence/snooping_protection_controller.h"
#include "ash/system/human_presence/snooping_protection_notification_blocker_internal.h"
#include "ash/system/model/system_tray_model.h"
Expand All @@ -39,6 +40,8 @@ namespace ash {

namespace {

namespace metrics = ash::snooping_protection_metrics;

constexpr char kNotifierId[] = "hps-notify";

// Returns the capitalized version of an improper-noun notifier title, or the
Expand Down Expand Up @@ -138,8 +141,7 @@ void SnoopingProtectionNotificationBlocker::OnBlockingPrefChanged() {
const bool pref_enabled = pref_change_registrar_->prefs()->GetBoolean(
prefs::kSnoopingProtectionNotificationSuppressionEnabled);
base::UmaHistogramBoolean(
"ChromeOS.HPS.SnoopingProtectionNotificationSuppression.Enabled",
pref_enabled);
metrics::kNotificationSuppressionEnabledHistogramName, pref_enabled);

OnBlockingActiveChanged();
}
Expand Down

0 comments on commit 91be349

Please sign in to comment.