[TTS] Add aggregation of CTR metrics to the CS component.

components/contextual_search/browser/ctr_aggregator.cc

+// Copyright 2016 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.
+
+#include "components/contextual_search/browser/ctr_aggregator.h"
+
+#include <unordered_map>
+
+#include "base/lazy_instance.h"
+#include "base/time/time.h"
+
+namespace {
+
+// Keys for ChromePreferenceManager storage of first and last week written.
+const char kFirstWeekWrittenKey[] = "contextual_search_first_week";
+const char kLastWeekWrittenKey[] = "contextual_search_last_week";
+// Number of weeks of data to record.
+const int kNumWeeksToRecord = 5;
+// Prefixes for ChromePreferenceManager storage keyed by week.
+const char kClicksWeekKeyPrefix[] = "contextual_search_clicks_week_";
+const char kImpressionsWeekKeyPrefix[] = "contextual_search_impressions_week_";
+
+const double kSecondsPerWeek =
+    base::Time::kMicrosecondsPerWeek / base::Time::kMicrosecondsPerSecond;
+// Used for validation in debug build.
+const int kReasonableMinWeek = 2000;

[Theresa, 2016/08/26 23:32:08]

Why 2000?

[Donn Denman, 2016/08/29 22:58:06]

Updated the comment: // Used for validation in debug build.  Week numbers are >
2300 as of year 2016.

[Theresa, 2016/08/29 23:57:34]

Acknowledged.

+
+}  // namespace
+
+namespace contextual_search {
+
+CTRAggregator::CTRAggregator(NativeIntStorage& storage) : storage_(storage) {
+  base::Time now = base::Time::NowFromSystemTime();
+  double now_in_seconds = now.ToDoubleT();
+  week_number_ = now_in_seconds / kSecondsPerWeek;
+  DCHECK(week_number_ >= kReasonableMinWeek);
+}
+
+CTRAggregator::CTRAggregator(NativeIntStorage& storage, int week_number)
+    : storage_(storage), week_number_(week_number) {
+  storage_.WriteInt(kFirstWeekWrittenKey, week_number_);

[Theresa, 2016/08/26 23:32:08]

Does this line need to get included in the 1-param constructor as well?

[Donn Denman, 2016/08/29 22:58:05]

It's problematic to put it into the constructor since that can cause a write,
which calls back into the Java code.  So I decided to just make sure each public
accessor checks.  I've updated the comment on that method to describe this.

It's also no longer needed in this test constructor, so I removed it.

+}
+
+CTRAggregator::~CTRAggregator() {}
+
+void CTRAggregator::RecordImpression(bool did_click) {
+  UpdateStorageToCurrentWeek();
+  Increment(GetWeekImpressionsKey(week_number_));
+  if (did_click)
+    Increment(GetWeekClicksKey(week_number_));
+}
+
+bool CTRAggregator::HasPreviousWeekData() {
+  EnsureFirstWeekRecorded();
+  int first_week_written = storage_.ReadInt(kFirstWeekWrittenKey);
+  return (week_number_ - first_week_written >= 1);
+}
+
+int CTRAggregator::GetPreviousWeekImpressions() {
+  UpdateStorageToCurrentWeek();
+  std::string key_impressions = GetWeekImpressionsKey(week_number_ - 1);
+  return storage_.ReadInt(key_impressions);
+}
+
+float CTRAggregator::GetPreviousWeekCTR() {
+  if (!HasPreviousWeekData())
+    return NAN;
+
+  UpdateStorageToCurrentWeek();
+  int clicks = GetPreviousWeekClicks();
+  int impressions = GetPreviousWeekImpressions();
+  if (impressions == 0)
+    return 0.0;
+  return (float)clicks / impressions;
+}
+
+bool CTRAggregator::HasPrevious28DayData() {
+  EnsureFirstWeekRecorded();
+  int first_week_written = storage_.ReadInt(kFirstWeekWrittenKey);
+  return (week_number_ - first_week_written >= 4);

[Theresa, 2016/08/26 23:32:08]

use > kNumWeeksToRecord instead of 4?

[Donn Denman, 2016/08/29 22:58:06]

Done: made a const kNumWeeksNeededFor28DayData = 4 (since using 5 won't work).
:-)

[Theresa, 2016/08/29 23:57:34]

Done.

[Theresa, 2016/08/29 23:57:57]

Err.. Acknowledged.

+}
+
+float CTRAggregator::GetPrevious28DayCTR() {
+  if (!HasPrevious28DayData())
+    return NAN;
+
+  UpdateStorageToCurrentWeek();
+  int clicks = GetPrevious28DayClicks();
+  int impressions = GetPrevious28DayImpressions();
+  if (impressions == 0)
+    return 0.0;
+  return (float)clicks / impressions;
+}
+
+int CTRAggregator::GetPrevious28DayImpressions() {
+  UpdateStorageToCurrentWeek();
+  int impressions = 0;
+  for (int week = 1; week <= 4; week++) {

[Theresa, 2016/08/26 23:32:08]

week < kNumWeeksToRecord?

[Donn Denman, 2016/08/29 22:58:06]

Done: seems better to use the new kNumWeeksNeededFor28DayData symbol.

+    std::string key_impressions = GetWeekImpressionsKey(week_number_ - week);
+    impressions += storage_.ReadInt(key_impressions);
+  }
+  return impressions;
+}
+
+// private
+
+int CTRAggregator::GetStorageIndex(int which_week) {

[Theresa, 2016/08/26 23:32:08]

Is this called anywhere other than GetWeekKey? If not, maybe we should collapse
those two methods?

[Donn Denman, 2016/08/29 22:58:05]

Done.

+  DCHECK(which_week > kReasonableMinWeek);
+  return which_week % kNumWeeksToRecord;
+}
+
+std::string CTRAggregator::GetWeekKey(int which_week) {
+  return std::to_string(GetStorageIndex(which_week));
+}
+
+std::string CTRAggregator::GetWeekImpressionsKey(int which_week) {
+  return kImpressionsWeekKeyPrefix + GetWeekKey(which_week);
+}
+
+std::string CTRAggregator::GetWeekClicksKey(int which_week) {
+  return kClicksWeekKeyPrefix + GetWeekKey(which_week);
+}
+
+int CTRAggregator::GetPreviousWeekClicks() {
+  std::string key_clicks = GetWeekClicksKey(week_number_ - 1);
+  return storage_.ReadInt(key_clicks);
+}
+
+int CTRAggregator::GetPrevious28DayClicks() {
+  int clicks = 0;
+  for (int week = 1; week <= 4; week++) {
+    std::string key_clicks = GetWeekClicksKey(week_number_ - week);
+    clicks += storage_.ReadInt(key_clicks);
+  }
+  return clicks;
+}
+
+void CTRAggregator::Increment(std::string key) {
+  int cur_value = storage_.ReadInt(key);
+  cur_value++;
+  storage_.WriteInt(key, cur_value);
+}
+
+void CTRAggregator::EnsureFirstWeekRecorded() {
+  int first_week_written = storage_.ReadInt(kFirstWeekWrittenKey);
+  if (first_week_written == 0)
+    storage_.WriteInt(kFirstWeekWrittenKey, week_number_);
+}
+
+void CTRAggregator::UpdateStorageToCurrentWeek() {
+  // If still on the same week we're done!
+  int last_week_written = storage_.ReadInt(kLastWeekWrittenKey);
+  if (last_week_written == week_number_)
+    return;
+
+  EnsureFirstWeekRecorded();
+  storage_.WriteInt(kLastWeekWrittenKey, week_number_);
+  if (last_week_written == 0)
+    return;
+
+  // Moved to a new week.  Since we recycle storage we must clear the new week
+  // and all that we may have skipped since our last write.
+  int weeks_to_clear =
+      std::min(week_number_ - last_week_written, kNumWeeksToRecord);
+  int week = week_number_;
+  while (weeks_to_clear > 0) {
+    storage_.WriteInt(GetWeekImpressionsKey(week), 0);
+    storage_.WriteInt(GetWeekClicksKey(week), 0);
+    week--;
+    weeks_to_clear--;
+  }
+}
+
+// Storage
+
+typedef std::unordered_map<std::string, int> IntMap;
+static base::LazyInstance<IntMap> s_storage_cache_ = LAZY_INSTANCE_INITIALIZER;
+
+int CTRAggregator::ReadInt(std::string storage_key) {
+  if (s_storage_cache_.Get().count(storage_key) != 0)
+    return s_storage_cache_.Get().at(storage_key);
+
+  int value = storage_.ReadInt(storage_key);
+  s_storage_cache_.Get().emplace(storage_key, value);
+  return value;
+}
+
+void CTRAggregator::WriteInt(std::string storage_key, int value) {

[Theresa, 2016/08/26 23:32:08]

What's calling this?

[Donn Denman, 2016/08/29 22:58:06]

Oops -- thanks for catching this!  
I added caching at the end, and forgot to call it.  Doh!
Tested this time, and found a flaw with overwriting values, and fixed.

+  storage_.WriteInt(storage_key, value);
+  s_storage_cache_.Get().emplace(storage_key, value);
+}
+
+// Testing only
+
+void CTRAggregator::IncrementWeek(int weeks) {
+  week_number_ += weeks;
+}
+
+}  // namespace contextual_search