diff options
Diffstat (limited to 'chromium/components/metrics')
56 files changed, 1383 insertions, 620 deletions
diff --git a/chromium/components/metrics/BUILD.gn b/chromium/components/metrics/BUILD.gn index d57fc5d53c8..bdf7ea2bbc0 100644 --- a/chromium/components/metrics/BUILD.gn +++ b/chromium/components/metrics/BUILD.gn @@ -124,6 +124,7 @@ jumbo_static_library("metrics") { "ukm_demographic_metrics_provider.h", "unsent_log_store.cc", "unsent_log_store.h", + "unsent_log_store_metrics.cc", "unsent_log_store_metrics.h", "unsent_log_store_metrics_impl.cc", "unsent_log_store_metrics_impl.h", @@ -214,14 +215,15 @@ static_library("component_metrics") { } if (!is_ios) { - static_library("gpu") { + static_library("content") { sources = [ - "gpu/gpu_metrics_provider.cc", - "gpu/gpu_metrics_provider.h", - "gpu/rendering_perf_metrics_provider.cc", - "gpu/rendering_perf_metrics_provider.h", + "content/gpu_metrics_provider.cc", + "content/gpu_metrics_provider.h", + "content/rendering_perf_metrics_provider.cc", + "content/rendering_perf_metrics_provider.h", + "content/subprocess_metrics_provider.cc", + "content/subprocess_metrics_provider.h", ] - public_deps = [ ":metrics" ] deps = [ "//base", @@ -251,8 +253,6 @@ jumbo_static_library("net") { "net/net_metrics_log_uploader.h", "net/network_metrics_provider.cc", "net/network_metrics_provider.h", - "net/wifi_access_point_info_provider.cc", - "net/wifi_access_point_info_provider.h", ] public_deps = [ ":metrics" ] @@ -269,14 +269,6 @@ jumbo_static_library("net") { "//third_party/zlib/google:compression_utils", "//url", ] - - if (is_chromeos) { - sources += [ - "net/wifi_access_point_info_provider_chromeos.cc", - "net/wifi_access_point_info_provider_chromeos.h", - ] - deps += [ "//chromeos/network" ] - } } static_library("ui") { @@ -442,6 +434,7 @@ source_set("unit_tests") { "file_metrics_provider_unittest.cc", "histogram_encoder_unittest.cc", "library_support/histogram_manager_unittest.cc", + "log_decoder_unittest.cc", "machine_id_provider_nonwin_unittest.cc", "metrics_log_manager_unittest.cc", "metrics_log_store_unittest.cc", @@ -457,6 +450,7 @@ source_set("unit_tests") { "stability_metrics_helper_unittest.cc", "stability_metrics_provider_unittest.cc", "ui/screen_info_metrics_provider_unittest.cc", + "unsent_log_store_metrics_impl_unittest.cc", "unsent_log_store_unittest.cc", ] @@ -513,14 +507,13 @@ source_set("unit_tests") { # these tests. if (is_ios) { sources -= [ "child_call_stack_profile_collector_unittest.cc" ] - } - - if (is_ios) { deps += [ "//ios/web/public/test" ] - } - - if (!is_ios) { - deps += [ "//content/test:test_support" ] + } else { + sources += [ "content/subprocess_metrics_provider_unittest.cc" ] + deps += [ + ":content", + "//content/test:test_support", + ] } } diff --git a/chromium/components/metrics/DEPS b/chromium/components/metrics/DEPS index c854e57a7e6..2793e4217f4 100644 --- a/chromium/components/metrics/DEPS +++ b/chromium/components/metrics/DEPS @@ -21,3 +21,9 @@ include_rules = [ "-net", "+url" ] + +specific_include_rules = { + "log_decoder\.cc": [ + "+third_party/protobuf/src/google/protobuf", + ], +} diff --git a/chromium/components/metrics/call_stack_profile_encoding.cc b/chromium/components/metrics/call_stack_profile_encoding.cc index 7ce77b0d61d..72811b7576d 100644 --- a/chromium/components/metrics/call_stack_profile_encoding.cc +++ b/chromium/components/metrics/call_stack_profile_encoding.cc @@ -19,6 +19,8 @@ Process ToExecutionContextProcess(CallStackProfileParams::Process process) { return GPU_PROCESS; case CallStackProfileParams::UTILITY_PROCESS: return UTILITY_PROCESS; + case CallStackProfileParams::NETWORK_SERVICE_PROCESS: + return NETWORK_SERVICE_PROCESS; case CallStackProfileParams::ZYGOTE_PROCESS: return ZYGOTE_PROCESS; case CallStackProfileParams::SANDBOX_HELPER_PROCESS: diff --git a/chromium/components/metrics/call_stack_profile_params.h b/chromium/components/metrics/call_stack_profile_params.h index 3f48e92222b..af19844f12a 100644 --- a/chromium/components/metrics/call_stack_profile_params.h +++ b/chromium/components/metrics/call_stack_profile_params.h @@ -21,7 +21,8 @@ struct CallStackProfileParams { ZYGOTE_PROCESS, SANDBOX_HELPER_PROCESS, PPAPI_PLUGIN_PROCESS, - PPAPI_BROKER_PROCESS + PPAPI_BROKER_PROCESS, + NETWORK_SERVICE_PROCESS, }; // The thread from which the collection occurred. diff --git a/chromium/components/metrics/gpu/DEPS b/chromium/components/metrics/content/DEPS index c2ff8a0af67..c2ff8a0af67 100644 --- a/chromium/components/metrics/gpu/DEPS +++ b/chromium/components/metrics/content/DEPS diff --git a/chromium/components/metrics/gpu/gpu_metrics_provider.cc b/chromium/components/metrics/content/gpu_metrics_provider.cc index a1b1bba39f5..baa1e770e92 100644 --- a/chromium/components/metrics/gpu/gpu_metrics_provider.cc +++ b/chromium/components/metrics/content/gpu_metrics_provider.cc @@ -2,7 +2,7 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. -#include "components/metrics/gpu/gpu_metrics_provider.h" +#include "components/metrics/content/gpu_metrics_provider.h" #include "content/public/browser/gpu_data_manager.h" #include "gpu/config/gpu_info.h" @@ -10,11 +10,9 @@ namespace metrics { -GPUMetricsProvider::GPUMetricsProvider() { -} +GPUMetricsProvider::GPUMetricsProvider() {} -GPUMetricsProvider::~GPUMetricsProvider() { -} +GPUMetricsProvider::~GPUMetricsProvider() {} void GPUMetricsProvider::ProvideSystemProfileMetrics( SystemProfileProto* system_profile_proto) { @@ -24,8 +22,7 @@ void GPUMetricsProvider::ProvideSystemProfileMetrics( const gpu::GPUInfo& gpu_info = content::GpuDataManager::GetInstance()->GetGPUInfo(); const gpu::GPUInfo::GPUDevice& active_gpu = gpu_info.active_gpu(); - SystemProfileProto::Hardware::Graphics* gpu = - hardware->mutable_gpu(); + SystemProfileProto::Hardware::Graphics* gpu = hardware->mutable_gpu(); gpu->set_vendor_id(active_gpu.vendor_id); gpu->set_device_id(active_gpu.device_id); gpu->set_driver_version(active_gpu.driver_version); diff --git a/chromium/components/metrics/gpu/gpu_metrics_provider.h b/chromium/components/metrics/content/gpu_metrics_provider.h index 581c7651ce6..d429be83cf5 100644 --- a/chromium/components/metrics/gpu/gpu_metrics_provider.h +++ b/chromium/components/metrics/content/gpu_metrics_provider.h @@ -2,8 +2,8 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. -#ifndef COMPONENTS_METRICS_GPU_GPU_METRICS_PROVIDER_H_ -#define COMPONENTS_METRICS_GPU_GPU_METRICS_PROVIDER_H_ +#ifndef COMPONENTS_METRICS_CONTENT_GPU_METRICS_PROVIDER_H_ +#define COMPONENTS_METRICS_CONTENT_GPU_METRICS_PROVIDER_H_ #include "base/macros.h" #include "components/metrics/metrics_provider.h" @@ -26,4 +26,4 @@ class GPUMetricsProvider : public MetricsProvider { } // namespace metrics -#endif // COMPONENTS_METRICS_GPU_GPU_METRICS_PROVIDER_H_ +#endif // COMPONENTS_METRICS_CONTENT_GPU_METRICS_PROVIDER_H_ diff --git a/chromium/components/metrics/gpu/rendering_perf_metrics_provider.cc b/chromium/components/metrics/content/rendering_perf_metrics_provider.cc index b45f2f7f8ae..c914f38df01 100644 --- a/chromium/components/metrics/gpu/rendering_perf_metrics_provider.cc +++ b/chromium/components/metrics/content/rendering_perf_metrics_provider.cc @@ -2,7 +2,7 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. -#include "components/metrics/gpu/rendering_perf_metrics_provider.h" +#include "components/metrics/content/rendering_perf_metrics_provider.h" #include "gpu/config/gpu_util.h" diff --git a/chromium/components/metrics/gpu/rendering_perf_metrics_provider.h b/chromium/components/metrics/content/rendering_perf_metrics_provider.h index 02c8b3591d4..90ac73b02c2 100644 --- a/chromium/components/metrics/gpu/rendering_perf_metrics_provider.h +++ b/chromium/components/metrics/content/rendering_perf_metrics_provider.h @@ -2,8 +2,8 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. -#ifndef COMPONENTS_METRICS_GPU_RENDERING_PERF_METRICS_PROVIDER_H_ -#define COMPONENTS_METRICS_GPU_RENDERING_PERF_METRICS_PROVIDER_H_ +#ifndef COMPONENTS_METRICS_CONTENT_RENDERING_PERF_METRICS_PROVIDER_H_ +#define COMPONENTS_METRICS_CONTENT_RENDERING_PERF_METRICS_PROVIDER_H_ #include "base/macros.h" #include "components/metrics/metrics_provider.h" @@ -27,4 +27,4 @@ class RenderingPerfMetricsProvider : public MetricsProvider { } // namespace metrics -#endif // COMPONENTS_METRICS_GPU_RENDERING_PERF_METRICS_PROVIDER_H_ +#endif // COMPONENTS_METRICS_CONTENT_RENDERING_PERF_METRICS_PROVIDER_H_ diff --git a/chromium/components/metrics/content/subprocess_metrics_provider.cc b/chromium/components/metrics/content/subprocess_metrics_provider.cc new file mode 100644 index 00000000000..9af900d3421 --- /dev/null +++ b/chromium/components/metrics/content/subprocess_metrics_provider.cc @@ -0,0 +1,210 @@ +// 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/metrics/content/subprocess_metrics_provider.h" + +#include <utility> + +#include "base/bind.h" +#include "base/logging.h" +#include "base/metrics/histogram_base.h" +#include "base/metrics/histogram_macros.h" +#include "base/metrics/persistent_histogram_allocator.h" +#include "base/metrics/persistent_memory_allocator.h" +#include "components/metrics/metrics_service.h" +#include "content/public/browser/browser_child_process_host.h" +#include "content/public/browser/browser_task_traits.h" +#include "content/public/browser/browser_thread.h" +#include "content/public/browser/child_process_data.h" + +namespace metrics { +namespace { + +// This is used by tests that don't have an easy way to access the global +// instance of this class. +SubprocessMetricsProvider* g_subprocess_metrics_provider_for_testing; + +} // namespace + +SubprocessMetricsProvider::SubprocessMetricsProvider() { + base::StatisticsRecorder::RegisterHistogramProvider( + weak_ptr_factory_.GetWeakPtr()); + content::BrowserChildProcessObserver::Add(this); + g_subprocess_metrics_provider_for_testing = this; +} + +SubprocessMetricsProvider::~SubprocessMetricsProvider() { + // Safe even if this object has never been added as an observer. + content::BrowserChildProcessObserver::Remove(this); + g_subprocess_metrics_provider_for_testing = nullptr; +} + +// static +void SubprocessMetricsProvider::MergeHistogramDeltasForTesting() { + DCHECK(g_subprocess_metrics_provider_for_testing); + g_subprocess_metrics_provider_for_testing->MergeHistogramDeltas(); +} + +void SubprocessMetricsProvider::RegisterSubprocessAllocator( + int id, + std::unique_ptr<base::PersistentHistogramAllocator> allocator) { + DCHECK_CALLED_ON_VALID_THREAD(thread_checker_); + DCHECK(!allocators_by_id_.Lookup(id)); + + // Stop now if this was called without an allocator, typically because + // GetSubprocessHistogramAllocatorOnIOThread exited early and returned + // null. + if (!allocator) + return; + + // Map is "MapOwnPointer" so transfer ownership to it. + allocators_by_id_.AddWithID(std::move(allocator), id); +} + +void SubprocessMetricsProvider::DeregisterSubprocessAllocator(int id) { + DCHECK_CALLED_ON_VALID_THREAD(thread_checker_); + + if (!allocators_by_id_.Lookup(id)) + return; + + // Extract the matching allocator from the list of active ones. It will + // be automatically released when this method exits. + std::unique_ptr<base::PersistentHistogramAllocator> allocator( + allocators_by_id_.Replace(id, nullptr)); + allocators_by_id_.Remove(id); + DCHECK(allocator); + + // Merge the last deltas from the allocator before it is released. + MergeHistogramDeltasFromAllocator(id, allocator.get()); +} + +void SubprocessMetricsProvider::MergeHistogramDeltasFromAllocator( + int id, + base::PersistentHistogramAllocator* allocator) { + DCHECK(allocator); + + int histogram_count = 0; + base::PersistentHistogramAllocator::Iterator hist_iter(allocator); + while (true) { + std::unique_ptr<base::HistogramBase> histogram = hist_iter.GetNext(); + if (!histogram) + break; + allocator->MergeHistogramDeltaToStatisticsRecorder(histogram.get()); + ++histogram_count; + } + + DVLOG(1) << "Reported " << histogram_count << " histograms from subprocess #" + << id; +} + +void SubprocessMetricsProvider::MergeHistogramDeltas() { + DCHECK_CALLED_ON_VALID_THREAD(thread_checker_); + + for (AllocatorByIdMap::iterator iter(&allocators_by_id_); !iter.IsAtEnd(); + iter.Advance()) { + MergeHistogramDeltasFromAllocator(iter.GetCurrentKey(), + iter.GetCurrentValue()); + } +} + +void SubprocessMetricsProvider::BrowserChildProcessHostConnected( + const content::ChildProcessData& data) { + DCHECK_CALLED_ON_VALID_THREAD(thread_checker_); + + // It's necessary to access the BrowserChildProcessHost object that is + // managing the child in order to extract the metrics memory from it. + // Unfortunately, the required lookup can only be performed on the IO + // thread so do the necessary dance. + content::GetIOThreadTaskRunner({})->PostTaskAndReplyWithResult( + FROM_HERE, + base::BindOnce( + &SubprocessMetricsProvider::GetSubprocessHistogramAllocatorOnIOThread, + data.id), + base::BindOnce(&SubprocessMetricsProvider::RegisterSubprocessAllocator, + weak_ptr_factory_.GetWeakPtr(), data.id)); +} + +void SubprocessMetricsProvider::BrowserChildProcessHostDisconnected( + const content::ChildProcessData& data) { + DCHECK_CALLED_ON_VALID_THREAD(thread_checker_); + DeregisterSubprocessAllocator(data.id); +} + +void SubprocessMetricsProvider::BrowserChildProcessCrashed( + const content::ChildProcessData& data, + const content::ChildProcessTerminationInfo& info) { + DCHECK_CALLED_ON_VALID_THREAD(thread_checker_); + DeregisterSubprocessAllocator(data.id); +} + +void SubprocessMetricsProvider::BrowserChildProcessKilled( + const content::ChildProcessData& data, + const content::ChildProcessTerminationInfo& info) { + DCHECK_CALLED_ON_VALID_THREAD(thread_checker_); + DeregisterSubprocessAllocator(data.id); +} + +void SubprocessMetricsProvider::OnRenderProcessHostCreated( + content::RenderProcessHost* host) { + // Sometimes, the same host will cause multiple notifications in tests so + // could possibly do the same in a release build. + if (!scoped_observer_.IsObserving(host)) + scoped_observer_.Add(host); +} + +void SubprocessMetricsProvider::RenderProcessReady( + content::RenderProcessHost* host) { + DCHECK_CALLED_ON_VALID_THREAD(thread_checker_); + + // If the render-process-host passed a persistent-memory-allocator to the + // renderer process, extract it and register it here. + std::unique_ptr<base::PersistentMemoryAllocator> allocator = + host->TakeMetricsAllocator(); + if (allocator) { + RegisterSubprocessAllocator( + host->GetID(), std::make_unique<base::PersistentHistogramAllocator>( + std::move(allocator))); + } +} + +void SubprocessMetricsProvider::RenderProcessExited( + content::RenderProcessHost* host, + const content::ChildProcessTerminationInfo& info) { + DCHECK_CALLED_ON_VALID_THREAD(thread_checker_); + + DeregisterSubprocessAllocator(host->GetID()); +} + +void SubprocessMetricsProvider::RenderProcessHostDestroyed( + content::RenderProcessHost* host) { + DCHECK_CALLED_ON_VALID_THREAD(thread_checker_); + + // It's possible for a Renderer to terminate without RenderProcessExited + // (above) being called so it's necessary to de-register also upon the + // destruction of the host. If both get called, no harm is done. + + DeregisterSubprocessAllocator(host->GetID()); + scoped_observer_.Remove(host); +} + +// static +std::unique_ptr<base::PersistentHistogramAllocator> +SubprocessMetricsProvider::GetSubprocessHistogramAllocatorOnIOThread(int id) { + // See if the new process has a memory allocator and take control of it if so. + // This call can only be made on the browser's IO thread. + content::BrowserChildProcessHost* host = + content::BrowserChildProcessHost::FromID(id); + if (!host) + return nullptr; + + std::unique_ptr<base::PersistentMemoryAllocator> allocator = + host->TakeMetricsAllocator(); + if (!allocator) + return nullptr; + + return std::make_unique<base::PersistentHistogramAllocator>( + std::move(allocator)); +} + +} // namespace metrics diff --git a/chromium/components/metrics/content/subprocess_metrics_provider.h b/chromium/components/metrics/content/subprocess_metrics_provider.h new file mode 100644 index 00000000000..cac37d5ca47 --- /dev/null +++ b/chromium/components/metrics/content/subprocess_metrics_provider.h @@ -0,0 +1,116 @@ +// 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. + +#ifndef COMPONENTS_METRICS_CONTENT_SUBPROCESS_METRICS_PROVIDER_H_ +#define COMPONENTS_METRICS_CONTENT_SUBPROCESS_METRICS_PROVIDER_H_ + +#include <memory> +#include <set> + +#include "base/containers/id_map.h" +#include "base/gtest_prod_util.h" +#include "base/memory/weak_ptr.h" +#include "base/metrics/statistics_recorder.h" +#include "base/scoped_observer.h" +#include "base/threading/thread_checker.h" +#include "components/metrics/metrics_provider.h" +#include "content/public/browser/browser_child_process_observer.h" +#include "content/public/browser/render_process_host.h" +#include "content/public/browser/render_process_host_creation_observer.h" +#include "content/public/browser/render_process_host_observer.h" + +namespace base { +class PersistentHistogramAllocator; +} + +namespace metrics { + +// SubprocessMetricsProvider gathers and merges histograms stored in shared +// memory segments between processes. Merging occurs when a process exits, +// when metrics are being collected for upload, or when something else needs +// combined metrics (such as the chrome://histograms page). +class SubprocessMetricsProvider + : public MetricsProvider, + public base::StatisticsRecorder::HistogramProvider, + public content::BrowserChildProcessObserver, + public content::RenderProcessHostCreationObserver, + public content::RenderProcessHostObserver { + public: + SubprocessMetricsProvider(); + ~SubprocessMetricsProvider() override; + + // Merge histograms for all subprocesses. This is used by tests that don't + // have access to the internal instance of this class. + static void MergeHistogramDeltasForTesting(); + + private: + friend class SubprocessMetricsProviderTest; + + // Indicates subprocess to be monitored with unique id for later reference. + // Metrics reporting will read histograms from it and upload them to UMA. + void RegisterSubprocessAllocator( + int id, + std::unique_ptr<base::PersistentHistogramAllocator> allocator); + + // Indicates that a subprocess has exited and is thus finished with the + // allocator it was using. + void DeregisterSubprocessAllocator(int id); + + // Merge all histograms of a given allocator to the global StatisticsRecorder. + // This is called periodically during UMA metrics collection (if enabled) and + // possibly on-demand for other purposes. + void MergeHistogramDeltasFromAllocator( + int id, + base::PersistentHistogramAllocator* allocator); + + // MetricsProvider: + void MergeHistogramDeltas() override; + + // content::BrowserChildProcessObserver: + void BrowserChildProcessHostConnected( + const content::ChildProcessData& data) override; + void BrowserChildProcessHostDisconnected( + const content::ChildProcessData& data) override; + void BrowserChildProcessCrashed( + const content::ChildProcessData& data, + const content::ChildProcessTerminationInfo& info) override; + void BrowserChildProcessKilled( + const content::ChildProcessData& data, + const content::ChildProcessTerminationInfo& info) override; + + // content::RenderProcessHostCreationObserver: + void OnRenderProcessHostCreated( + content::RenderProcessHost* process_host) override; + + // content::RenderProcessHostObserver: + void RenderProcessReady(content::RenderProcessHost* host) override; + void RenderProcessExited( + content::RenderProcessHost* host, + const content::ChildProcessTerminationInfo& info) override; + void RenderProcessHostDestroyed(content::RenderProcessHost* host) override; + + // Gets a histogram allocator from a subprocess. This must be called on + // the IO thread. + static std::unique_ptr<base::PersistentHistogramAllocator> + GetSubprocessHistogramAllocatorOnIOThread(int id); + + THREAD_CHECKER(thread_checker_); + + // All of the shared-persistent-allocators for known sub-processes. + using AllocatorByIdMap = + base::IDMap<std::unique_ptr<base::PersistentHistogramAllocator>, int>; + AllocatorByIdMap allocators_by_id_; + + // Track all observed render processes to un-observe them on exit. + ScopedObserver<content::RenderProcessHost, content::RenderProcessHostObserver> + scoped_observer_{this}; + + base::WeakPtrFactory<SubprocessMetricsProvider> weak_ptr_factory_{this}; + + DISALLOW_COPY_AND_ASSIGN(SubprocessMetricsProvider); +}; + +} // namespace metrics + +#endif // COMPONENTS_METRICS_CONTENT_SUBPROCESS_METRICS_PROVIDER_H_ diff --git a/chromium/components/metrics/content/subprocess_metrics_provider_unittest.cc b/chromium/components/metrics/content/subprocess_metrics_provider_unittest.cc new file mode 100644 index 00000000000..26a3a6e640e --- /dev/null +++ b/chromium/components/metrics/content/subprocess_metrics_provider_unittest.cc @@ -0,0 +1,161 @@ +// 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/metrics/content/subprocess_metrics_provider.h" + +#include <memory> +#include <string> +#include <vector> + +#include "base/metrics/histogram.h" +#include "base/metrics/histogram_flattener.h" +#include "base/metrics/histogram_snapshot_manager.h" +#include "base/metrics/persistent_histogram_allocator.h" +#include "base/metrics/persistent_memory_allocator.h" +#include "base/metrics/statistics_recorder.h" +#include "content/public/test/browser_task_environment.h" +#include "testing/gmock/include/gmock/gmock.h" +#include "testing/gtest/include/gtest/gtest.h" + +using ::testing::IsEmpty; +using ::testing::UnorderedElementsAre; + +namespace metrics { +namespace { + +const uint32_t TEST_MEMORY_SIZE = 64 << 10; // 64 KiB + +class HistogramFlattenerDeltaRecorder : public base::HistogramFlattener { + public: + HistogramFlattenerDeltaRecorder() {} + + void RecordDelta(const base::HistogramBase& histogram, + const base::HistogramSamples& snapshot) override { + recorded_delta_histogram_names_.push_back(histogram.histogram_name()); + } + + std::vector<std::string> GetRecordedDeltaHistogramNames() { + return recorded_delta_histogram_names_; + } + + private: + std::vector<std::string> recorded_delta_histogram_names_; + + DISALLOW_COPY_AND_ASSIGN(HistogramFlattenerDeltaRecorder); +}; + +} // namespace + +class SubprocessMetricsProviderTest : public testing::Test { + protected: + SubprocessMetricsProviderTest() { + // MergeHistogramDeltas needs to be called beause it uses a histogram + // macro which caches a pointer to a histogram. If not done before setting + // a persistent global allocator, then it would point into memory that + // will go away. + provider_.MergeHistogramDeltas(); + + // Create a dedicated StatisticsRecorder for this test. + test_recorder_ = base::StatisticsRecorder::CreateTemporaryForTesting(); + + // Create a global allocator using a block of memory from the heap. + base::GlobalHistogramAllocator::CreateWithLocalMemory(TEST_MEMORY_SIZE, 0, + ""); + } + + ~SubprocessMetricsProviderTest() override { + base::GlobalHistogramAllocator::ReleaseForTesting(); + } + + SubprocessMetricsProvider* provider() { return &provider_; } + + std::unique_ptr<base::PersistentHistogramAllocator> CreateDuplicateAllocator( + base::PersistentHistogramAllocator* allocator) { + // Just wrap around the data segment in-use by the passed allocator. + return std::make_unique<base::PersistentHistogramAllocator>( + std::make_unique<base::PersistentMemoryAllocator>( + const_cast<void*>(allocator->data()), allocator->length(), 0, 0, + std::string(), false)); + } + + std::vector<std::string> GetSnapshotHistogramNames() { + // Merge the data from the allocator into the StatisticsRecorder. + provider_.MergeHistogramDeltas(); + + // Flatten what is known to see what has changed since the last time. + HistogramFlattenerDeltaRecorder flattener; + base::HistogramSnapshotManager snapshot_manager(&flattener); + // "true" to the begin() includes histograms held in persistent storage. + base::StatisticsRecorder::PrepareDeltas(true, base::Histogram::kNoFlags, + base::Histogram::kNoFlags, + &snapshot_manager); + return flattener.GetRecordedDeltaHistogramNames(); + } + + void EnableRecording() { provider_.OnRecordingEnabled(); } + void DisableRecording() { provider_.OnRecordingDisabled(); } + + void RegisterSubprocessAllocator( + int id, + std::unique_ptr<base::PersistentHistogramAllocator> allocator) { + provider_.RegisterSubprocessAllocator(id, std::move(allocator)); + } + + void DeregisterSubprocessAllocator(int id) { + provider_.DeregisterSubprocessAllocator(id); + } + + private: + // A thread-bundle makes the tests appear on the UI thread, something that is + // checked in methods called from the SubprocessMetricsProvider class under + // test. This must be constructed before the |provider_| field. + content::BrowserTaskEnvironment task_environment_; + + SubprocessMetricsProvider provider_; + std::unique_ptr<base::StatisticsRecorder> test_recorder_; + + DISALLOW_COPY_AND_ASSIGN(SubprocessMetricsProviderTest); +}; + +TEST_F(SubprocessMetricsProviderTest, SnapshotMetrics) { + base::HistogramBase* foo = base::Histogram::FactoryGet("foo", 1, 100, 10, 0); + base::HistogramBase* bar = base::Histogram::FactoryGet("bar", 1, 100, 10, 0); + base::HistogramBase* baz = base::Histogram::FactoryGet("baz", 1, 100, 10, 0); + foo->Add(42); + bar->Add(84); + + // Detach the global allocator but keep it around until this method exits + // so that the memory holding histogram data doesn't get released. Register + // a new allocator that duplicates the global one. + std::unique_ptr<base::GlobalHistogramAllocator> global_allocator( + base::GlobalHistogramAllocator::ReleaseForTesting()); + RegisterSubprocessAllocator(123, + CreateDuplicateAllocator(global_allocator.get())); + + // Recording should find the two histograms created in persistent memory. + EXPECT_THAT(GetSnapshotHistogramNames(), UnorderedElementsAre("foo", "bar")); + + // A second run should have nothing to produce. + EXPECT_THAT(GetSnapshotHistogramNames(), IsEmpty()); + + // Create a new histogram and update existing ones. Should now report 3 items. + baz->Add(1969); + foo->Add(10); + bar->Add(20); + EXPECT_THAT(GetSnapshotHistogramNames(), + UnorderedElementsAre("foo", "bar", "baz")); + + // Ensure that deregistering does a final merge of the data. + foo->Add(10); + bar->Add(20); + DeregisterSubprocessAllocator(123); + EXPECT_THAT(GetSnapshotHistogramNames(), UnorderedElementsAre("foo", "bar")); + + // Further snapshots should be empty even if things have changed. + foo->Add(10); + bar->Add(20); + EXPECT_THAT(GetSnapshotHistogramNames(), IsEmpty()); +} + +} // namespace metrics diff --git a/chromium/components/metrics/daily_event.cc b/chromium/components/metrics/daily_event.cc index befc2eb9b3d..6df1cdd4273 100644 --- a/chromium/components/metrics/daily_event.cc +++ b/chromium/components/metrics/daily_event.cc @@ -6,6 +6,7 @@ #include <utility> +#include "base/logging.h" #include "base/metrics/histogram.h" #include "components/prefs/pref_registry_simple.h" #include "components/prefs/pref_service.h" diff --git a/chromium/components/metrics/field_trials_provider.cc b/chromium/components/metrics/field_trials_provider.cc index b13f131823e..dc202f7a39f 100644 --- a/chromium/components/metrics/field_trials_provider.cc +++ b/chromium/components/metrics/field_trials_provider.cc @@ -48,16 +48,46 @@ void FieldTrialsProvider::ProvideSystemProfileMetrics( void FieldTrialsProvider::ProvideSystemProfileMetricsWithLogCreationTime( base::TimeTicks log_creation_time, metrics::SystemProfileProto* system_profile_proto) { - std::vector<ActiveGroupId> field_trial_ids; + // TODO(crbug/1090497): Maybe call ProvideCurrentSessionData() instead from + // places in which ProvideSystemProfileMetricsWithLogCreationTime() is called, + // e.g. startup_data.cc and background_tracing_metrics_provider.cc. + + log_creation_time_ = log_creation_time; + const std::string& version = variations::GetSeedVersion(); if (!version.empty()) system_profile_proto->set_variations_seed_version(version); - GetFieldTrialIds(&field_trial_ids); - WriteFieldTrials(field_trial_ids, system_profile_proto); + + // TODO(crbug/1090098): Determine whether this can be deleted. + GetAndWriteFieldTrials(system_profile_proto); +} + +void FieldTrialsProvider::ProvideCurrentSessionData( + metrics::ChromeUserMetricsExtension* uma_proto) { + // This function is called from both + // ProvideSystemProfileMetricsWithLogCreationTime() and + // ProvideCurrentSessionData() so that field trials activated in other metrics + // providers are captured. We need both calls because in some scenarios in + // which this class is used, only the former function gets called. + DCHECK(!log_creation_time_.is_null()); + GetAndWriteFieldTrials(uma_proto->mutable_system_profile()); +} + +void FieldTrialsProvider::SetLogCreationTimeForTesting(base::TimeTicks time) { + log_creation_time_ = time; +} + +void FieldTrialsProvider::GetAndWriteFieldTrials( + metrics::SystemProfileProto* system_profile_proto) const { + system_profile_proto->clear_field_trial(); + + std::vector<ActiveGroupId> field_trials; + GetFieldTrialIds(&field_trials); + WriteFieldTrials(field_trials, system_profile_proto); if (registry_) { std::vector<ActiveGroupId> synthetic_trials; - registry_->GetSyntheticFieldTrialsOlderThan(log_creation_time, + registry_->GetSyntheticFieldTrialsOlderThan(log_creation_time_, &synthetic_trials); WriteFieldTrials(synthetic_trials, system_profile_proto); } diff --git a/chromium/components/metrics/field_trials_provider.h b/chromium/components/metrics/field_trials_provider.h index 6d115e5e47a..d2b4e0edcaa 100644 --- a/chromium/components/metrics/field_trials_provider.h +++ b/chromium/components/metrics/field_trials_provider.h @@ -8,6 +8,7 @@ #include "base/strings/string_piece.h" #include "base/time/time.h" #include "components/metrics/metrics_provider.h" +#include "third_party/metrics_proto/chrome_user_metrics_extension.pb.h" // TODO(crbug/507665): Once MetricsProvider/SystemProfileProto are moved into // //services/metrics, then //components/variations can depend on them, and @@ -30,12 +31,26 @@ class FieldTrialsProvider : public metrics::MetricsProvider { void ProvideSystemProfileMetricsWithLogCreationTime( base::TimeTicks log_creation_time, metrics::SystemProfileProto* system_profile_proto) override; + void ProvideCurrentSessionData( + metrics::ChromeUserMetricsExtension* uma_proto) override; + + // Sets |log_creation_time_| to |time|. + void SetLogCreationTimeForTesting(base::TimeTicks time); private: // Overrideable for testing. virtual void GetFieldTrialIds( std::vector<ActiveGroupId>* field_trial_ids) const; + // Gets active FieldTrials and SyntheticFieldTrials and populates + // |system_profile_proto| with them. + void GetAndWriteFieldTrials( + metrics::SystemProfileProto* system_profile_proto) const; + + // The most recent time passed to + // ProvideSystemProfileMetricsWithLogCreationTime(). + base::TimeTicks log_creation_time_; + SyntheticTrialRegistry* registry_; // Suffix used for the field trial names before they are hashed for uploads. diff --git a/chromium/components/metrics/field_trials_provider_unittest.cc b/chromium/components/metrics/field_trials_provider_unittest.cc index 0190615cda4..593568ce0ba 100644 --- a/chromium/components/metrics/field_trials_provider_unittest.cc +++ b/chromium/components/metrics/field_trials_provider_unittest.cc @@ -15,7 +15,12 @@ namespace variations { namespace { const ActiveGroupId kFieldTrialIds[] = {{37, 43}, {13, 47}, {23, 17}}; -const ActiveGroupId kSyntheticTrials[] = {{55, 15}, {66, 16}}; +const ActiveGroupId kSyntheticTrialIds[] = {{55, 15}, {66, 16}}; +const ActiveGroupId kAllTrialIds[] = {{37, 43}, + {13, 47}, + {23, 17}, + {55, 15}, + {66, 16}}; class TestProvider : public FieldTrialsProvider { public: @@ -32,29 +37,15 @@ class TestProvider : public FieldTrialsProvider { } }; -// Check that the values in |system_values| correspond to the test data -// defined at the top of this file. +// Check that the field trials in |system_profile| correspond to |expected|. void CheckFieldTrialsInSystemProfile( - const metrics::SystemProfileProto& system_profile) { - // Verify the right data is present for the field trials. - for (size_t i = 0; i < base::size(kFieldTrialIds); ++i) { + const metrics::SystemProfileProto& system_profile, + const ActiveGroupId* expected) { + for (int i = 0; i < system_profile.field_trial_size(); ++i) { const metrics::SystemProfileProto::FieldTrial& field_trial = system_profile.field_trial(i); - EXPECT_EQ(kFieldTrialIds[i].name, field_trial.name_id()); - EXPECT_EQ(kFieldTrialIds[i].group, field_trial.group_id()); - } -} - -// Check that the values in |system_values| correspond to the test data -// defined at the top of this file. -void CheckSyntheticTrialsInSystemProfile( - const metrics::SystemProfileProto& system_profile) { - // Verify the right data is present for the synthetic trials. - for (size_t i = 0; i < base::size(kSyntheticTrials); ++i) { - const metrics::SystemProfileProto::FieldTrial& field_trial = - system_profile.field_trial(i + base::size(kFieldTrialIds)); - EXPECT_EQ(kSyntheticTrials[i].name, field_trial.name_id()); - EXPECT_EQ(kSyntheticTrials[i].group, field_trial.group_id()); + EXPECT_EQ(expected[i].name, field_trial.name_id()); + EXPECT_EQ(expected[i].group, field_trial.group_id()); } } @@ -68,7 +59,7 @@ class FieldTrialsProviderTest : public ::testing::Test { protected: // Register trials which should get recorded. void RegisterExpectedSyntheticTrials() { - for (const ActiveGroupId& id : kSyntheticTrials) { + for (const ActiveGroupId& id : kSyntheticTrialIds) { registry_.RegisterSyntheticFieldTrial( SyntheticTrialGroup(id.name, id.group)); } @@ -108,10 +99,9 @@ TEST_F(FieldTrialsProviderTest, ProvideSyntheticTrials) { provider.ProvideSystemProfileMetricsWithLogCreationTime(log_creation_time, &proto); - ASSERT_EQ(base::size(kFieldTrialIds) + base::size(kSyntheticTrials), + EXPECT_EQ(base::size(kAllTrialIds), static_cast<size_t>(proto.field_trial_size())); - CheckFieldTrialsInSystemProfile(proto); - CheckSyntheticTrialsInSystemProfile(proto); + CheckFieldTrialsInSystemProfile(proto, kAllTrialIds); } TEST_F(FieldTrialsProviderTest, NoSyntheticTrials) { @@ -121,9 +111,33 @@ TEST_F(FieldTrialsProviderTest, NoSyntheticTrials) { provider.ProvideSystemProfileMetricsWithLogCreationTime(base::TimeTicks(), &proto); - ASSERT_EQ(base::size(kFieldTrialIds), + EXPECT_EQ(base::size(kFieldTrialIds), static_cast<size_t>(proto.field_trial_size())); - CheckFieldTrialsInSystemProfile(proto); + CheckFieldTrialsInSystemProfile(proto, kFieldTrialIds); +} + +TEST_F(FieldTrialsProviderTest, ProvideCurrentSessionData) { + metrics::ChromeUserMetricsExtension uma_log; + uma_log.system_profile(); + + // {1, 1} should not be in the resulting proto as ProvideCurrentSessionData() + // clears existing trials and sets the trials to be those determined by + // GetSyntheticFieldTrialsOlderThan() and GetFieldTrialIds(). + metrics::SystemProfileProto::FieldTrial* trial = + uma_log.mutable_system_profile()->add_field_trial(); + trial->set_name_id(1); + trial->set_group_id(1); + + TestProvider provider(®istry_, base::StringPiece()); + RegisterExpectedSyntheticTrials(); + WaitUntilTimeChanges(base::TimeTicks::Now()); + provider.SetLogCreationTimeForTesting(base::TimeTicks::Now()); + + provider.ProvideCurrentSessionData(&uma_log); + + EXPECT_EQ(base::size(kAllTrialIds), + static_cast<size_t>(uma_log.system_profile().field_trial_size())); + CheckFieldTrialsInSystemProfile(uma_log.system_profile(), kAllTrialIds); } } // namespace variations diff --git a/chromium/components/metrics/file_metrics_provider_unittest.cc b/chromium/components/metrics/file_metrics_provider_unittest.cc index fee1a5472d6..3435393fb00 100644 --- a/chromium/components/metrics/file_metrics_provider_unittest.cc +++ b/chromium/components/metrics/file_metrics_provider_unittest.cc @@ -9,6 +9,7 @@ #include "base/files/file_util.h" #include "base/files/memory_mapped_file.h" #include "base/files/scoped_temp_dir.h" +#include "base/logging.h" #include "base/metrics/histogram.h" #include "base/metrics/histogram_flattener.h" #include "base/metrics/histogram_snapshot_manager.h" diff --git a/chromium/components/metrics/log_decoder.cc b/chromium/components/metrics/log_decoder.cc index 1754c2717df..c92e2a6f2af 100644 --- a/chromium/components/metrics/log_decoder.cc +++ b/chromium/components/metrics/log_decoder.cc @@ -4,6 +4,7 @@ #include "components/metrics/log_decoder.h" +#include "third_party/protobuf/src/google/protobuf/message_lite.h" #include "third_party/zlib/google/compression_utils.h" namespace metrics { @@ -13,4 +14,13 @@ bool DecodeLogData(const std::string& compressed_log_data, return compression::GzipUncompress(compressed_log_data, log_data); } +bool DecodeLogDataToProto(const std::string& compressed_log_data, + google::protobuf::MessageLite* proto) { + std::string log_data; + if (!DecodeLogData(compressed_log_data, &log_data)) + return false; + + return proto->ParseFromString(log_data); +} + } // namespace metrics diff --git a/chromium/components/metrics/log_decoder.h b/chromium/components/metrics/log_decoder.h index c85037f932c..f6a7969642b 100644 --- a/chromium/components/metrics/log_decoder.h +++ b/chromium/components/metrics/log_decoder.h @@ -7,6 +7,14 @@ #include <string> +namespace google { +namespace protobuf { + +class MessageLite; + +} // namespace protobuf +} // namespace google + namespace metrics { // Other modules can call this function instead of directly calling gzip. This @@ -16,6 +24,11 @@ namespace metrics { bool DecodeLogData(const std::string& compressed_log_data, std::string* log_data); +// Decodes |compressed_log_data| and populates |proto| with the decompressed log +// data. Returns true on success and false on failure. +bool DecodeLogDataToProto(const std::string& compressed_log_data, + google::protobuf::MessageLite* proto); + } // namespace metrics #endif // COMPONENTS_METRICS_LOG_DECODER_H_ diff --git a/chromium/components/metrics/log_decoder_unittest.cc b/chromium/components/metrics/log_decoder_unittest.cc new file mode 100644 index 00000000000..2657c85ee4c --- /dev/null +++ b/chromium/components/metrics/log_decoder_unittest.cc @@ -0,0 +1,33 @@ +// Copyright 2020 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/metrics/log_decoder.h" + +#include <string> + +#include "base/macros.h" +#include "testing/gtest/include/gtest/gtest.h" +#include "third_party/metrics_proto/chrome_user_metrics_extension.pb.h" +#include "third_party/zlib/google/compression_utils.h" + +namespace metrics { + +TEST(LogDecoderTest, DecodeLogDataToProto) { + ChromeUserMetricsExtension uma_log1; + uma_log1.mutable_system_profile()->set_application_locale("fr"); + + std::string log_data1; + ASSERT_TRUE(uma_log1.SerializeToString(&log_data1)); + std::string compressed_log_data; + ASSERT_TRUE(compression::GzipCompress(log_data1, &compressed_log_data)); + + ChromeUserMetricsExtension uma_log2; + EXPECT_TRUE(DecodeLogDataToProto(compressed_log_data, &uma_log2)); + + std::string log_data2; + uma_log2.SerializeToString(&log_data2); + EXPECT_EQ(log_data1, log_data2); +} + +} // namespace metrics diff --git a/chromium/components/metrics/log_store.h b/chromium/components/metrics/log_store.h index 4965879402b..744ef67b7f8 100644 --- a/chromium/components/metrics/log_store.h +++ b/chromium/components/metrics/log_store.h @@ -42,6 +42,10 @@ class LogStore { // Discards the staged log. virtual void DiscardStagedLog() = 0; + // Marks the staged log as sent, DiscardStagedLog() shall still be called if + // the staged log needs discarded. + virtual void MarkStagedLogAsSent() = 0; + // Saves any unsent logs to persistent storage. virtual void PersistUnsentLogs() const = 0; diff --git a/chromium/components/metrics/metrics_log.cc b/chromium/components/metrics/metrics_log.cc index c200c8782ec..628bec542ff 100644 --- a/chromium/components/metrics/metrics_log.cc +++ b/chromium/components/metrics/metrics_log.cc @@ -11,6 +11,7 @@ #include "base/build_time.h" #include "base/cpu.h" +#include "base/logging.h" #include "base/metrics/histogram_base.h" #include "base/metrics/histogram_flattener.h" #include "base/metrics/histogram_functions.h" @@ -233,6 +234,7 @@ void MetricsLog::RecordCoreSystemProfile( void MetricsLog::RecordHistogramDelta(const std::string& histogram_name, const base::HistogramSamples& snapshot) { DCHECK(!closed_); + samples_count_ += snapshot.TotalCount(); EncodeHistogramDelta(histogram_name, snapshot, &uma_proto_); } diff --git a/chromium/components/metrics/metrics_log.h b/chromium/components/metrics/metrics_log.h index b4e7cd40bb3..6a265aa0618 100644 --- a/chromium/components/metrics/metrics_log.h +++ b/chromium/components/metrics/metrics_log.h @@ -15,6 +15,7 @@ #include <vector> #include "base/macros.h" +#include "base/metrics/histogram_base.h" #include "base/time/time.h" #include "components/metrics/metrics_service_client.h" #include "third_party/metrics_proto/chrome_user_metrics_extension.pb.h" @@ -165,6 +166,10 @@ class MetricsLog { LogType log_type() const { return log_type_; } + // Returns the number of samples in this log, it is only valid after the + // histogram delta is calculated. + base::HistogramBase::Count samples_count() const { return samples_count_; } + // Exposed for the sake of mocking/accessing in test code. ChromeUserMetricsExtension* UmaProtoForTest() { return &uma_proto_; } @@ -212,6 +217,9 @@ class MetricsLog { // RecordEnvironment() or LoadSavedEnvironmentFromPrefs(). bool has_environment_; + // The number of samples in this log. + base::HistogramBase::Count samples_count_ = 0; + DISALLOW_COPY_AND_ASSIGN(MetricsLog); }; diff --git a/chromium/components/metrics/metrics_log_manager.cc b/chromium/components/metrics/metrics_log_manager.cc index d90aa7b3dc4..28794486a76 100644 --- a/chromium/components/metrics/metrics_log_manager.cc +++ b/chromium/components/metrics/metrics_log_manager.cc @@ -28,8 +28,10 @@ void MetricsLogManager::FinishCurrentLog(MetricsLogStore* log_store) { current_log_->CloseLog(); std::string log_data; current_log_->GetEncodedLog(&log_data); - if (!log_data.empty()) - log_store->StoreLog(log_data, current_log_->log_type()); + if (!log_data.empty()) { + log_store->StoreLog(log_data, current_log_->log_type(), + base::make_optional(current_log_->samples_count())); + } current_log_.reset(); } diff --git a/chromium/components/metrics/metrics_log_store.cc b/chromium/components/metrics/metrics_log_store.cc index 026bcaf0673..9a01caacecf 100644 --- a/chromium/components/metrics/metrics_log_store.cc +++ b/chromium/components/metrics/metrics_log_store.cc @@ -38,6 +38,8 @@ const size_t kStorageByteLimitPerLogType = 300 * 1000; // ~300kB void MetricsLogStore::RegisterPrefs(PrefRegistrySimple* registry) { registry->RegisterListPref(prefs::kMetricsInitialLogs); registry->RegisterListPref(prefs::kMetricsOngoingLogs); + registry->RegisterDictionaryPref(prefs::kMetricsInitialLogsMetadata); + registry->RegisterDictionaryPref(prefs::kMetricsOngoingLogsMetadata); } MetricsLogStore::MetricsLogStore(PrefService* local_state, @@ -47,6 +49,7 @@ MetricsLogStore::MetricsLogStore(PrefService* local_state, initial_log_queue_(std::make_unique<UnsentLogStoreMetricsImpl>(), local_state, prefs::kMetricsInitialLogs, + prefs::kMetricsInitialLogsMetadata, kInitialLogsSaveLimit, kStorageByteLimitPerLogType, 0, @@ -54,6 +57,7 @@ MetricsLogStore::MetricsLogStore(PrefService* local_state, ongoing_log_queue_(std::make_unique<UnsentLogStoreMetricsImpl>(), local_state, prefs::kMetricsOngoingLogs, + prefs::kMetricsOngoingLogsMetadata, kOngoingLogsSaveLimit, kStorageByteLimitPerLogType, max_ongoing_log_size, @@ -67,15 +71,17 @@ void MetricsLogStore::LoadPersistedUnsentLogs() { unsent_logs_loaded_ = true; } -void MetricsLogStore::StoreLog(const std::string& log_data, - MetricsLog::LogType log_type) { +void MetricsLogStore::StoreLog( + const std::string& log_data, + MetricsLog::LogType log_type, + base::Optional<base::HistogramBase::Count> samples_count) { switch (log_type) { case MetricsLog::INITIAL_STABILITY_LOG: - initial_log_queue_.StoreLog(log_data); + initial_log_queue_.StoreLog(log_data, samples_count); break; case MetricsLog::ONGOING_LOG: case MetricsLog::INDEPENDENT_LOG: - ongoing_log_queue_.StoreLog(log_data); + ongoing_log_queue_.StoreLog(log_data, samples_count); break; } } @@ -124,6 +130,14 @@ void MetricsLogStore::DiscardStagedLog() { DCHECK(!has_staged_log()); } +void MetricsLogStore::MarkStagedLogAsSent() { + DCHECK(has_staged_log()); + if (initial_log_queue_.has_staged_log()) + initial_log_queue_.MarkStagedLogAsSent(); + else + ongoing_log_queue_.MarkStagedLogAsSent(); +} + void MetricsLogStore::PersistUnsentLogs() const { DCHECK(unsent_logs_loaded_); if (!unsent_logs_loaded_) diff --git a/chromium/components/metrics/metrics_log_store.h b/chromium/components/metrics/metrics_log_store.h index 361a09c2b2b..0c7550e9953 100644 --- a/chromium/components/metrics/metrics_log_store.h +++ b/chromium/components/metrics/metrics_log_store.h @@ -8,6 +8,8 @@ #include <string> #include "base/macros.h" +#include "base/metrics/histogram_base.h" +#include "base/optional.h" #include "components/metrics/log_store.h" #include "components/metrics/metrics_log.h" #include "components/metrics/unsent_log_store.h" @@ -36,7 +38,9 @@ class MetricsLogStore : public LogStore { static void RegisterPrefs(PrefRegistrySimple* registry); // Saves |log_data| as the given type. - void StoreLog(const std::string& log_data, MetricsLog::LogType log_type); + void StoreLog(const std::string& log_data, + MetricsLog::LogType log_type, + base::Optional<base::HistogramBase::Count> samples_count); // LogStore: bool has_unsent_logs() const override; @@ -46,6 +50,7 @@ class MetricsLogStore : public LogStore { const std::string& staged_log_signature() const override; void StageNextLog() override; void DiscardStagedLog() override; + void MarkStagedLogAsSent() override; void PersistUnsentLogs() const override; void LoadPersistedUnsentLogs() override; diff --git a/chromium/components/metrics/metrics_log_store_unittest.cc b/chromium/components/metrics/metrics_log_store_unittest.cc index bb4da691931..d3c0fee949e 100644 --- a/chromium/components/metrics/metrics_log_store_unittest.cc +++ b/chromium/components/metrics/metrics_log_store_unittest.cc @@ -49,7 +49,7 @@ TEST_F(MetricsLogStoreTest, StandardFlow) { EXPECT_FALSE(log_store.has_staged_log()); EXPECT_FALSE(log_store.has_unsent_logs()); - log_store.StoreLog("a", MetricsLog::ONGOING_LOG); + log_store.StoreLog("a", MetricsLog::ONGOING_LOG, base::nullopt); EXPECT_TRUE(log_store.has_unsent_logs()); EXPECT_FALSE(log_store.has_staged_log()); @@ -69,7 +69,7 @@ TEST_F(MetricsLogStoreTest, StoreAndLoad) { MetricsLogStore log_store(&pref_service_, 0, std::string()); log_store.LoadPersistedUnsentLogs(); EXPECT_FALSE(log_store.has_unsent_logs()); - log_store.StoreLog("a", MetricsLog::ONGOING_LOG); + log_store.StoreLog("a", MetricsLog::ONGOING_LOG, base::nullopt); log_store.PersistUnsentLogs(); EXPECT_EQ(0U, TypeCount(MetricsLog::INITIAL_STABILITY_LOG)); EXPECT_EQ(1U, TypeCount(MetricsLog::ONGOING_LOG)); @@ -82,9 +82,9 @@ TEST_F(MetricsLogStoreTest, StoreAndLoad) { EXPECT_TRUE(log_store.has_unsent_logs()); EXPECT_EQ(0U, TypeCount(MetricsLog::INITIAL_STABILITY_LOG)); EXPECT_EQ(1U, TypeCount(MetricsLog::ONGOING_LOG)); - log_store.StoreLog("x", MetricsLog::INITIAL_STABILITY_LOG); + log_store.StoreLog("x", MetricsLog::INITIAL_STABILITY_LOG, base::nullopt); log_store.StageNextLog(); - log_store.StoreLog("b", MetricsLog::ONGOING_LOG); + log_store.StoreLog("b", MetricsLog::ONGOING_LOG, base::nullopt); EXPECT_TRUE(log_store.has_unsent_logs()); EXPECT_TRUE(log_store.has_staged_log()); @@ -134,7 +134,7 @@ TEST_F(MetricsLogStoreTest, StoreStagedOngoingLog) { // Ensure that types are preserved when storing staged logs. MetricsLogStore log_store(&pref_service_, 0, std::string()); log_store.LoadPersistedUnsentLogs(); - log_store.StoreLog("a", MetricsLog::ONGOING_LOG); + log_store.StoreLog("a", MetricsLog::ONGOING_LOG, base::nullopt); log_store.StageNextLog(); log_store.PersistUnsentLogs(); @@ -146,7 +146,7 @@ TEST_F(MetricsLogStoreTest, StoreStagedInitialLog) { // Ensure that types are preserved when storing staged logs. MetricsLogStore log_store(&pref_service_, 0, std::string()); log_store.LoadPersistedUnsentLogs(); - log_store.StoreLog("b", MetricsLog::INITIAL_STABILITY_LOG); + log_store.StoreLog("b", MetricsLog::INITIAL_STABILITY_LOG, base::nullopt); log_store.StageNextLog(); log_store.PersistUnsentLogs(); @@ -159,8 +159,9 @@ TEST_F(MetricsLogStoreTest, LargeLogDiscarding) { MetricsLogStore log_store(&pref_service_, 1, std::string()); log_store.LoadPersistedUnsentLogs(); - log_store.StoreLog("persisted", MetricsLog::INITIAL_STABILITY_LOG); - log_store.StoreLog("not_persisted", MetricsLog::ONGOING_LOG); + log_store.StoreLog("persisted", MetricsLog::INITIAL_STABILITY_LOG, + base::nullopt); + log_store.StoreLog("not_persisted", MetricsLog::ONGOING_LOG, base::nullopt); // Only the stability log should be written out, due to the threshold. log_store.PersistUnsentLogs(); @@ -174,10 +175,10 @@ TEST_F(MetricsLogStoreTest, DiscardOrder) { MetricsLogStore log_store(&pref_service_, 0, std::string()); log_store.LoadPersistedUnsentLogs(); - log_store.StoreLog("a", MetricsLog::ONGOING_LOG); - log_store.StoreLog("b", MetricsLog::ONGOING_LOG); + log_store.StoreLog("a", MetricsLog::ONGOING_LOG, base::nullopt); + log_store.StoreLog("b", MetricsLog::ONGOING_LOG, base::nullopt); log_store.StageNextLog(); - log_store.StoreLog("c", MetricsLog::INITIAL_STABILITY_LOG); + log_store.StoreLog("c", MetricsLog::INITIAL_STABILITY_LOG, base::nullopt); EXPECT_EQ(2U, log_store.ongoing_log_count()); EXPECT_EQ(1U, log_store.initial_log_count()); // Should discard the ongoing log staged earlier. diff --git a/chromium/components/metrics/metrics_pref_names.cc b/chromium/components/metrics/metrics_pref_names.cc index 63250e13a0b..38dffd3e278 100644 --- a/chromium/components/metrics/metrics_pref_names.cc +++ b/chromium/components/metrics/metrics_pref_names.cc @@ -28,6 +28,12 @@ const char kMetricsDefaultOptIn[] = "user_experience_metrics.default_opt_in"; // count info, etc. const char kMetricsInitialLogs[] = "user_experience_metrics.initial_logs2"; +// An dictionary of information about the unsent initial logs, it was +// recorded when the unsent log is persisted and will be written into the +// metrics at the next browser starts up. +const char kMetricsInitialLogsMetadata[] = + "user_experience_metrics.unsent_log_metadata.initial_logs"; + // Low entropy source values. The new source (with suffix "3") was created // because the old source (with suffix "2") is biased in the wild. Clients which // have an old source still incorporate it into the high entropy source, to @@ -50,6 +56,11 @@ const char kMetricsMachineId[] = "user_experience_metrics.machine_id"; // user activities. const char kMetricsOngoingLogs[] = "user_experience_metrics.ongoing_logs2"; +// An dictionary that is same as kUnsentLogMetkMetricsInitialLogsMetadata, +// but for the ongoing logs. +const char kMetricsOngoingLogsMetadata[] = + "user_experience_metrics.unsent_log_metadata.ongoing_logs"; + // Boolean that indicates a cloned install has been detected and the metrics // client id and low entropy source should be reset. const char kMetricsResetIds[] = "user_experience_metrics.reset_metrics_ids"; diff --git a/chromium/components/metrics/metrics_pref_names.h b/chromium/components/metrics/metrics_pref_names.h index ae1376cb49a..43414a6ca62 100644 --- a/chromium/components/metrics/metrics_pref_names.h +++ b/chromium/components/metrics/metrics_pref_names.h @@ -14,10 +14,12 @@ extern const char kInstallDate[]; extern const char kMetricsClientID[]; extern const char kMetricsDefaultOptIn[]; extern const char kMetricsInitialLogs[]; +extern const char kMetricsInitialLogsMetadata[]; extern const char kMetricsLowEntropySource[]; extern const char kMetricsOldLowEntropySource[]; extern const char kMetricsMachineId[]; extern const char kMetricsOngoingLogs[]; +extern const char kMetricsOngoingLogsMetadata[]; extern const char kMetricsResetIds[]; // For finding out whether metrics and crash reporting is enabled use the diff --git a/chromium/components/metrics/metrics_scheduler.cc b/chromium/components/metrics/metrics_scheduler.cc index 793f6be7d29..4df3c8a34bf 100644 --- a/chromium/components/metrics/metrics_scheduler.cc +++ b/chromium/components/metrics/metrics_scheduler.cc @@ -51,6 +51,10 @@ void MetricsScheduler::TaskDone(base::TimeDelta next_interval) { } void MetricsScheduler::TriggerTask() { + // This can happen in tests which set a very small timer interval. + if (callback_pending_) + return; + callback_pending_ = true; task_callback_.Run(); } diff --git a/chromium/components/metrics/metrics_service.cc b/chromium/components/metrics/metrics_service.cc index 687471a854d..6e3e369a220 100644 --- a/chromium/components/metrics/metrics_service.cc +++ b/chromium/components/metrics/metrics_service.cc @@ -219,7 +219,9 @@ MetricsService::MetricsService(MetricsStateManager* state_manager, test_mode_active_(false), state_(INITIALIZED), idle_since_last_transmission_(false), - session_id_(-1) { + session_id_(-1), + synthetic_trial_registry_( + client->IsExternalExperimentAllowlistEnabled()) { DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); DCHECK(state_manager_); DCHECK(client_); @@ -229,9 +231,6 @@ MetricsService::MetricsService(MetricsStateManager* state_manager, std::make_unique<StabilityMetricsProvider>(local_state_)); RegisterMetricsProvider(state_manager_->GetProvider()); - - RegisterMetricsProvider(std::make_unique<variations::FieldTrialsProvider>( - &synthetic_trial_registry_, base::StringPiece())); } MetricsService::~MetricsService() { @@ -239,6 +238,12 @@ MetricsService::~MetricsService() { } void MetricsService::InitializeMetricsRecordingState() { + // The FieldTrialsProvider should be registered last. This ensures that + // studies whose features are checked when providers add their information to + // the log appear in the active field trials. + RegisterMetricsProvider(std::make_unique<variations::FieldTrialsProvider>( + &synthetic_trial_registry_, base::StringPiece())); + reporting_service_.Initialize(); InitializeMetricsState(); @@ -449,10 +454,6 @@ void MetricsService::ClearSavedStabilityMetrics() { delegating_provider_.ClearSavedStabilityMetrics(); } -void MetricsService::PushExternalLog(const std::string& log) { - log_store()->StoreLog(log, MetricsLog::ONGOING_LOG); -} - bool MetricsService::StageCurrentLogForTest() { CloseCurrentLog(); @@ -539,6 +540,7 @@ void MetricsService::InitializeMetricsState() { local_state_->SetInteger(prefs::kMetricsSessionID, session_id_); // Notify stability metrics providers about the launch. + UMA_HISTOGRAM_BOOLEAN("UMA.MetricsService.Initialize", true); provider.LogLaunch(); provider.CheckLastSessionEndCompleted(); diff --git a/chromium/components/metrics/metrics_service.h b/chromium/components/metrics/metrics_service.h index dab73c84e41..b42bb611318 100644 --- a/chromium/components/metrics/metrics_service.h +++ b/chromium/components/metrics/metrics_service.h @@ -171,9 +171,6 @@ class MetricsService : public base::HistogramFlattener { // Clears the stability metrics that are saved in local state. void ClearSavedStabilityMetrics(); - // Pushes a log that has been generated by an external component. - void PushExternalLog(const std::string& log); - variations::SyntheticTrialRegistry* synthetic_trial_registry() { return &synthetic_trial_registry_; } @@ -186,13 +183,6 @@ class MetricsService : public base::HistogramFlattener { bool StageCurrentLogForTest(); protected: - // Exposed for testing. - // TODO(1034679): migrate these to public FooForTest() methods. - MetricsLogManager* log_manager() { return &log_manager_; } - MetricsLogStore* log_store() { - return reporting_service_.metrics_log_store(); - } - // Sets the persistent system profile. Virtual for tests. virtual void SetPersistentSystemProfile(const std::string& serialized_proto, bool complete); @@ -229,6 +219,11 @@ class MetricsService : public base::HistogramFlattener { UNSET }; + // Gets the LogStore for UMA logs. + MetricsLogStore* log_store() { + return reporting_service_.metrics_log_store(); + } + // Calls into the client to initialize some system profile metrics. void StartInitTask(); @@ -403,6 +398,7 @@ class MetricsService : public base::HistogramFlattener { // exited-cleanly bit in the prefs. static ShutdownCleanliness clean_shutdown_status_; + FRIEND_TEST_ALL_PREFIXES(MetricsServiceTest, ActiveFieldTrialsReported); FRIEND_TEST_ALL_PREFIXES(MetricsServiceTest, IsPluginProcess); FRIEND_TEST_ALL_PREFIXES(::ChromeMetricsServiceClientTest, TestRegisterMetricsServiceProviders); diff --git a/chromium/components/metrics/metrics_service_accessor.cc b/chromium/components/metrics/metrics_service_accessor.cc index bddf51512c1..ca1fb8ca159 100644 --- a/chromium/components/metrics/metrics_service_accessor.cc +++ b/chromium/components/metrics/metrics_service_accessor.cc @@ -47,29 +47,6 @@ bool MetricsServiceAccessor::RegisterSyntheticFieldTrial( } // static -bool MetricsServiceAccessor::RegisterSyntheticMultiGroupFieldTrial( - MetricsService* metrics_service, - base::StringPiece trial_name, - const std::vector<uint32_t>& group_name_hashes) { - if (!metrics_service) - return false; - - metrics_service->synthetic_trial_registry() - ->RegisterSyntheticMultiGroupFieldTrial(variations::HashName(trial_name), - group_name_hashes); - return true; -} - -// static -bool MetricsServiceAccessor::RegisterSyntheticFieldTrialWithNameHash( - MetricsService* metrics_service, - uint32_t trial_name_hash, - base::StringPiece group_name) { - return RegisterSyntheticFieldTrialWithNameAndGroupHash( - metrics_service, trial_name_hash, variations::HashName(group_name)); -} - -// static bool MetricsServiceAccessor::RegisterSyntheticFieldTrialWithNameAndGroupHash( MetricsService* metrics_service, uint32_t trial_name_hash, diff --git a/chromium/components/metrics/metrics_service_accessor.h b/chromium/components/metrics/metrics_service_accessor.h index 3a36278213c..e6b247b9854 100644 --- a/chromium/components/metrics/metrics_service_accessor.h +++ b/chromium/components/metrics/metrics_service_accessor.h @@ -41,23 +41,6 @@ class MetricsServiceAccessor { base::StringPiece trial_name, base::StringPiece group_name); - // Registers a field trial name and set of groups with |metrics_service| (if - // not null), to be used to annotate a UMA report with a particular - // configuration state. Returns true on success. - // See the comment on MetricsService::RegisterSyntheticMultiGroupFieldTrial() - // for details. - static bool RegisterSyntheticMultiGroupFieldTrial( - MetricsService* metrics_service, - base::StringPiece trial_name, - const std::vector<uint32_t>& group_name_hashes); - - // Same as RegisterSyntheticFieldTrial above, but takes in the trial name as a - // hash rather than computing the hash from the string. - static bool RegisterSyntheticFieldTrialWithNameHash( - MetricsService* metrics_service, - uint32_t trial_name_hash, - base::StringPiece group_name); - // Same as RegisterSyntheticFieldTrial above, but takes in the trial and group // names as hashes rather than computing those hashes from the strings. static bool RegisterSyntheticFieldTrialWithNameAndGroupHash( diff --git a/chromium/components/metrics/metrics_service_client.cc b/chromium/components/metrics/metrics_service_client.cc index 7e01ca6a3b6..c838a0c3a18 100644 --- a/chromium/components/metrics/metrics_service_client.cc +++ b/chromium/components/metrics/metrics_service_client.cc @@ -8,6 +8,7 @@ #include <string> #include "base/command_line.h" +#include "base/logging.h" #include "base/strings/string_number_conversions.h" #include "base/strings/string_util.h" #include "components/metrics/metrics_switches.h" @@ -30,18 +31,6 @@ ukm::UkmService* MetricsServiceClient::GetUkmService() { return nullptr; } -bool MetricsServiceClient::IsReportingPolicyManaged() { - return false; -} - -EnableMetricsDefault MetricsServiceClient::GetMetricsReportingDefaultState() { - return EnableMetricsDefault::DEFAULT_UNKNOWN; -} - -bool MetricsServiceClient::IsUMACellularUploadLogicEnabled() { - return false; -} - GURL MetricsServiceClient::GetMetricsServerUrl() { return GURL(kNewMetricsServerUrl); } @@ -73,6 +62,22 @@ bool MetricsServiceClient::ShouldStartUpFastForTesting() const { return false; } +bool MetricsServiceClient::IsReportingPolicyManaged() { + return false; +} + +EnableMetricsDefault MetricsServiceClient::GetMetricsReportingDefaultState() { + return EnableMetricsDefault::DEFAULT_UNKNOWN; +} + +bool MetricsServiceClient::IsUMACellularUploadLogicEnabled() { + return false; +} + +bool MetricsServiceClient::IsExternalExperimentAllowlistEnabled() { + return true; +} + bool MetricsServiceClient::IsUkmAllowedForAllProfiles() { return false; } diff --git a/chromium/components/metrics/metrics_service_client.h b/chromium/components/metrics/metrics_service_client.h index e22f276d843..ea907d78820 100644 --- a/chromium/components/metrics/metrics_service_client.h +++ b/chromium/components/metrics/metrics_service_client.h @@ -130,6 +130,10 @@ class MetricsServiceClient { // Returns whether cellular logic is enabled for metrics reporting. virtual bool IsUMACellularUploadLogicEnabled(); + // Returns whether the allowlist for external experiment ids is enabled. Some + // embedders like WebLayer disable it. For Chrome, it should be enabled. + virtual bool IsExternalExperimentAllowlistEnabled(); + // Returns true iff UKM is allowed for all profiles. // See //components/ukm/observers/ukm_consent_state_observer.h for details. virtual bool IsUkmAllowedForAllProfiles(); diff --git a/chromium/components/metrics/metrics_service_unittest.cc b/chromium/components/metrics/metrics_service_unittest.cc index e0791ddad76..0adbd9da00e 100644 --- a/chromium/components/metrics/metrics_service_unittest.cc +++ b/chromium/components/metrics/metrics_service_unittest.cc @@ -12,6 +12,7 @@ #include "base/bind.h" #include "base/macros.h" +#include "base/metrics/field_trial.h" #include "base/metrics/metrics_hashes.h" #include "base/metrics/statistics_recorder.h" #include "base/metrics/user_metrics.h" @@ -23,6 +24,7 @@ #include "base/threading/thread_task_runner_handle.h" #include "components/metrics/client_info.h" #include "components/metrics/environment_recorder.h" +#include "components/metrics/log_decoder.h" #include "components/metrics/metrics_log.h" #include "components/metrics/metrics_pref_names.h" #include "components/metrics/metrics_state_manager.h" @@ -31,7 +33,10 @@ #include "components/metrics/test/test_metrics_provider.h" #include "components/metrics/test/test_metrics_service_client.h" #include "components/prefs/testing_pref_service.h" +#include "components/variations/active_field_trials.h" #include "testing/gtest/include/gtest/gtest.h" +#include "third_party/metrics_proto/chrome_user_metrics_extension.pb.h" +#include "third_party/metrics_proto/system_profile.pb.h" #include "third_party/zlib/google/compression_utils.h" namespace metrics { @@ -50,16 +55,28 @@ std::unique_ptr<ClientInfo> ReturnNoBackup() { return nullptr; } +// Returns true if |id| is present in |proto|'s collection of FieldTrials. +bool IsFieldTrialPresent(const SystemProfileProto& proto, + const std::string& trial_name, + const std::string& group_name) { + const variations::ActiveGroupId id = + variations::MakeActiveGroupId(trial_name, group_name); + + for (const auto& trial : proto.field_trial()) { + if (trial.name_id() == id.name && trial.group_id() == id.group) + return true; + } + return false; +} + class TestMetricsService : public MetricsService { public: TestMetricsService(MetricsStateManager* state_manager, MetricsServiceClient* client, PrefService* local_state) : MetricsService(state_manager, client, local_state) {} - ~TestMetricsService() override {} + ~TestMetricsService() override = default; - using MetricsService::log_manager; - using MetricsService::log_store; using MetricsService::RecordCurrentEnvironmentHelper; // MetricsService: @@ -170,6 +187,33 @@ class MetricsServiceTest : public testing::Test { DISALLOW_COPY_AND_ASSIGN(MetricsServiceTest); }; +class ExperimentTestMetricsProvider : public TestMetricsProvider { + public: + explicit ExperimentTestMetricsProvider( + base::FieldTrial* profile_metrics_trial, + base::FieldTrial* session_data_trial) + : profile_metrics_trial_(profile_metrics_trial), + session_data_trial_(session_data_trial) {} + + ~ExperimentTestMetricsProvider() override = default; + + void ProvideSystemProfileMetrics( + SystemProfileProto* system_profile_proto) override { + TestMetricsProvider::ProvideSystemProfileMetrics(system_profile_proto); + profile_metrics_trial_->group(); + } + + void ProvideCurrentSessionData( + ChromeUserMetricsExtension* uma_proto) override { + TestMetricsProvider::ProvideCurrentSessionData(uma_proto); + session_data_trial_->group(); + } + + private: + base::FieldTrial* profile_metrics_trial_; + base::FieldTrial* session_data_trial_; +}; + } // namespace TEST_F(MetricsServiceTest, InitialStabilityLogAfterCleanShutDown) { @@ -231,9 +275,9 @@ TEST_F(MetricsServiceTest, InitialStabilityLogAtProviderRequest) { service.InitializeMetricsRecordingState(); // The initial stability log should be generated and persisted in unsent logs. - MetricsLogStore* log_store = service.log_store(); - EXPECT_TRUE(log_store->has_unsent_logs()); - EXPECT_FALSE(log_store->has_staged_log()); + MetricsLogStore* test_log_store = service.LogStoreForTest(); + EXPECT_TRUE(test_log_store->has_unsent_logs()); + EXPECT_FALSE(test_log_store->has_staged_log()); // Ensure that HasPreviousSessionData() is always called on providers, // for consistency, even if other conditions already indicate their presence. @@ -245,15 +289,11 @@ TEST_F(MetricsServiceTest, InitialStabilityLogAtProviderRequest) { EXPECT_TRUE(test_provider->provide_stability_metrics_called()); // Stage the log and retrieve it. - log_store->StageNextLog(); - EXPECT_TRUE(log_store->has_staged_log()); - - std::string uncompressed_log; - EXPECT_TRUE( - compression::GzipUncompress(log_store->staged_log(), &uncompressed_log)); + test_log_store->StageNextLog(); + EXPECT_TRUE(test_log_store->has_staged_log()); ChromeUserMetricsExtension uma_log; - EXPECT_TRUE(uma_log.ParseFromString(uncompressed_log)); + EXPECT_TRUE(DecodeLogDataToProto(test_log_store->staged_log(), &uma_log)); EXPECT_TRUE(uma_log.has_client_id()); EXPECT_TRUE(uma_log.has_session_id()); @@ -298,9 +338,9 @@ TEST_F(MetricsServiceTest, InitialStabilityLogAfterCrash) { service.InitializeMetricsRecordingState(); // The initial stability log should be generated and persisted in unsent logs. - MetricsLogStore* log_store = service.log_store(); - EXPECT_TRUE(log_store->has_unsent_logs()); - EXPECT_FALSE(log_store->has_staged_log()); + MetricsLogStore* test_log_store = service.LogStoreForTest(); + EXPECT_TRUE(test_log_store->has_unsent_logs()); + EXPECT_FALSE(test_log_store->has_staged_log()); // Ensure that HasPreviousSessionData() is always called on providers, // for consistency, even if other conditions already indicate their presence. @@ -312,15 +352,11 @@ TEST_F(MetricsServiceTest, InitialStabilityLogAfterCrash) { EXPECT_TRUE(test_provider->provide_stability_metrics_called()); // Stage the log and retrieve it. - log_store->StageNextLog(); - EXPECT_TRUE(log_store->has_staged_log()); - - std::string uncompressed_log; - EXPECT_TRUE( - compression::GzipUncompress(log_store->staged_log(), &uncompressed_log)); + test_log_store->StageNextLog(); + EXPECT_TRUE(test_log_store->has_staged_log()); ChromeUserMetricsExtension uma_log; - EXPECT_TRUE(uma_log.ParseFromString(uncompressed_log)); + EXPECT_TRUE(DecodeLogDataToProto(test_log_store->staged_log(), &uma_log)); EXPECT_TRUE(uma_log.has_client_id()); EXPECT_TRUE(uma_log.has_session_id()); @@ -363,6 +399,44 @@ TEST_F(MetricsServiceTest, MetricsProvidersInitialized) { EXPECT_TRUE(test_provider->init_called()); } +// Verify that FieldTrials activated by a MetricsProvider are reported by the +// FieldTrialsProvider. +TEST_F(MetricsServiceTest, ActiveFieldTrialsReported) { + EnableMetricsReporting(); + TestMetricsServiceClient client; + TestMetricsService service(GetMetricsStateManager(), &client, + GetLocalState()); + + // Set up FieldTrials. + const std::string trial_name1 = "CoffeeExperiment"; + const std::string group_name1 = "Free"; + base::FieldTrial* trial1 = + base::FieldTrialList::CreateFieldTrial(trial_name1, group_name1); + + const std::string trial_name2 = "DonutExperiment"; + const std::string group_name2 = "MapleBacon"; + base::FieldTrial* trial2 = + base::FieldTrialList::CreateFieldTrial(trial_name2, group_name2); + + service.RegisterMetricsProvider( + std::make_unique<ExperimentTestMetricsProvider>(trial1, trial2)); + + service.InitializeMetricsRecordingState(); + service.Start(); + service.StageCurrentLogForTest(); + + MetricsLogStore* test_log_store = service.LogStoreForTest(); + ChromeUserMetricsExtension uma_log; + EXPECT_TRUE(DecodeLogDataToProto(test_log_store->staged_log(), &uma_log)); + + // Verify that the reported FieldTrial IDs are for the trial set up by this + // test. + EXPECT_TRUE( + IsFieldTrialPresent(uma_log.system_profile(), trial_name1, group_name1)); + EXPECT_TRUE( + IsFieldTrialPresent(uma_log.system_profile(), trial_name2, group_name2)); +} + TEST_F(MetricsServiceTest, SystemProfileDataProvidedOnEnableRecording) { EnableMetricsReporting(); TestMetricsServiceClient client; @@ -427,15 +501,6 @@ TEST_F(MetricsServiceTest, SplitRotation) { task_runner_->RunPendingTasks(); EXPECT_TRUE(client.uploader()->is_uploading()); EXPECT_EQ(1U, task_runner_->NumPendingTasks()); - // Uploader should reschedule when there is another log available. - service.PushExternalLog("Blah"); - client.uploader()->CompleteUpload(200); - EXPECT_FALSE(client.uploader()->is_uploading()); - EXPECT_EQ(2U, task_runner_->NumPendingTasks()); - // Upload should start. - task_runner_->RunPendingTasks(); - EXPECT_TRUE(client.uploader()->is_uploading()); - EXPECT_EQ(1U, task_runner_->NumPendingTasks()); } TEST_F(MetricsServiceTest, LastLiveTimestamp) { diff --git a/chromium/components/metrics/net/net_metrics_log_uploader.cc b/chromium/components/metrics/net/net_metrics_log_uploader.cc index 641e119282f..3c0562bb3c0 100644 --- a/chromium/components/metrics/net/net_metrics_log_uploader.cc +++ b/chromium/components/metrics/net/net_metrics_log_uploader.cc @@ -4,12 +4,17 @@ #include "components/metrics/net/net_metrics_log_uploader.h" +#include <sstream> + #include "base/base64.h" #include "base/bind.h" #include "base/feature_list.h" #include "base/metrics/field_trial_params.h" #include "base/metrics/histogram_macros.h" +#include "base/metrics/statistics_recorder.h" #include "base/rand_util.h" +#include "base/strings/strcat.h" +#include "base/strings/string_number_conversions.h" #include "base/task/post_task.h" #include "base/threading/sequenced_task_runner_handle.h" #include "components/encrypted_messages/encrypted_message.pb.h" @@ -21,7 +26,9 @@ #include "net/url_request/url_fetcher.h" #include "services/network/public/cpp/shared_url_loader_factory.h" #include "services/network/public/cpp/simple_url_loader.h" +#include "third_party/metrics_proto/chrome_user_metrics_extension.pb.h" #include "third_party/metrics_proto/reporting_info.pb.h" +#include "third_party/zlib/google/compression_utils.h" #include "url/gurl.h" namespace { @@ -182,6 +189,52 @@ bool EncryptAndBase64EncodeString(const std::string& plaintext, return true; } +#ifndef NDEBUG +void LogUploadingHistograms(const std::string& compressed_log_data) { + if (!VLOG_IS_ON(2)) + return; + + std::string uncompressed; + if (!compression::GzipUncompress(compressed_log_data, &uncompressed)) { + DVLOG(2) << "failed to uncompress log"; + return; + } + metrics::ChromeUserMetricsExtension proto; + if (!proto.ParseFromString(uncompressed)) { + DVLOG(2) << "failed to parse uncompressed log"; + return; + }; + DVLOG(2) << "Uploading histograms..."; + + const base::StatisticsRecorder::Histograms histograms = + base::StatisticsRecorder::GetHistograms(); + auto get_histogram_name = [&](uint64_t name_hash) -> std::string { + for (base::HistogramBase* histogram : histograms) { + if (histogram->name_hash() == name_hash) + return histogram->histogram_name(); + } + return base::StrCat({"unnamed ", base::NumberToString(name_hash)}); + }; + + for (int i = 0; i < proto.histogram_event_size(); i++) { + const metrics::HistogramEventProto& event = proto.histogram_event(i); + + std::stringstream summary; + summary << " sum=" << event.sum(); + for (int j = 0; j < event.bucket_size(); j++) { + const metrics::HistogramEventProto::Bucket& b = event.bucket(j); + // Empty fields have a specific meaning, see + // third_party/metrics_proto/histogram_event.proto. + summary << " bucket[" + << (b.has_min() ? base::NumberToString(b.min()) : "..") << '-' + << (b.has_max() ? base::NumberToString(b.max()) : "..") << ")=" + << (b.has_count() ? base::NumberToString(b.count()) : "(1)"); + } + DVLOG(2) << get_histogram_name(event.name_hash()) << summary.str(); + } +} +#endif + } // namespace namespace metrics { @@ -243,6 +296,13 @@ void NetMetricsLogUploader::UploadLogToURL( const GURL& url) { DCHECK(!log_hash.empty()); +#ifndef NDEBUG + // For debug builds, you can use -vmodule=net_metrics_log_uploader=2 + // to enable logging of uploaded histograms. You probably also want to use + // --force-enable-metrics-reporting, or metrics reporting may not be enabled. + LogUploadingHistograms(compressed_log_data); +#endif + auto resource_request = std::make_unique<network::ResourceRequest>(); resource_request->url = url; // Drop cookies and auth data. diff --git a/chromium/components/metrics/net/network_metrics_provider.cc b/chromium/components/metrics/net/network_metrics_provider.cc index 1c99f32282c..1f4bfda67a4 100644 --- a/chromium/components/metrics/net/network_metrics_provider.cc +++ b/chromium/components/metrics/net/network_metrics_provider.cc @@ -34,10 +34,6 @@ #include "services/network/public/cpp/network_connection_tracker.h" #endif -#if defined(OS_CHROMEOS) -#include "components/metrics/net/wifi_access_point_info_provider_chromeos.h" -#endif // OS_CHROMEOS - namespace { #if defined(OS_ANDROID) @@ -200,21 +196,6 @@ void NetworkMetricsProvider::ProvideSystemProfileMetrics( wifi_phy_layer_protocol_is_ambiguous_ = false; min_effective_connection_type_ = effective_connection_type_; max_effective_connection_type_ = effective_connection_type_; - - if (!wifi_access_point_info_provider_) { -#if defined(OS_CHROMEOS) - wifi_access_point_info_provider_.reset( - new WifiAccessPointInfoProviderChromeos()); -#else - wifi_access_point_info_provider_.reset( - new WifiAccessPointInfoProvider()); -#endif // OS_CHROMEOS - } - - // Connected wifi access point information. - WifiAccessPointInfoProvider::WifiAccessPointInfo info; - if (wifi_access_point_info_provider_->GetInfo(&info)) - WriteWifiAccessPointProto(info, network); } void NetworkMetricsProvider::OnConnectionChanged( @@ -321,85 +302,6 @@ void NetworkMetricsProvider::OnWifiPHYLayerProtocolResult( wifi_phy_layer_protocol_ = mode; } -void NetworkMetricsProvider::WriteWifiAccessPointProto( - const WifiAccessPointInfoProvider::WifiAccessPointInfo& info, - SystemProfileProto::Network* network_proto) { - DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); - SystemProfileProto::Network::WifiAccessPoint* access_point_info = - network_proto->mutable_access_point_info(); - SystemProfileProto::Network::WifiAccessPoint::SecurityMode security = - SystemProfileProto::Network::WifiAccessPoint::SECURITY_UNKNOWN; - switch (info.security) { - case WifiAccessPointInfoProvider::WIFI_SECURITY_NONE: - security = SystemProfileProto::Network::WifiAccessPoint::SECURITY_NONE; - break; - case WifiAccessPointInfoProvider::WIFI_SECURITY_WPA: - security = SystemProfileProto::Network::WifiAccessPoint::SECURITY_WPA; - break; - case WifiAccessPointInfoProvider::WIFI_SECURITY_WEP: - security = SystemProfileProto::Network::WifiAccessPoint::SECURITY_WEP; - break; - case WifiAccessPointInfoProvider::WIFI_SECURITY_RSN: - security = SystemProfileProto::Network::WifiAccessPoint::SECURITY_RSN; - break; - case WifiAccessPointInfoProvider::WIFI_SECURITY_802_1X: - security = SystemProfileProto::Network::WifiAccessPoint::SECURITY_802_1X; - break; - case WifiAccessPointInfoProvider::WIFI_SECURITY_PSK: - security = SystemProfileProto::Network::WifiAccessPoint::SECURITY_PSK; - break; - case WifiAccessPointInfoProvider::WIFI_SECURITY_UNKNOWN: - security = SystemProfileProto::Network::WifiAccessPoint::SECURITY_UNKNOWN; - break; - } - access_point_info->set_security_mode(security); - - // |bssid| is xx:xx:xx:xx:xx:xx, extract the first three components and - // pack into a uint32_t. - std::string bssid = info.bssid; - if (bssid.size() == 17 && bssid[2] == ':' && bssid[5] == ':' && - bssid[8] == ':' && bssid[11] == ':' && bssid[14] == ':') { - std::string vendor_prefix_str; - uint32_t vendor_prefix; - - base::RemoveChars(bssid.substr(0, 9), ":", &vendor_prefix_str); - DCHECK_EQ(6U, vendor_prefix_str.size()); - if (base::HexStringToUInt(vendor_prefix_str, &vendor_prefix)) - access_point_info->set_vendor_prefix(vendor_prefix); - else - NOTREACHED(); - } - - // Return if vendor information is not provided. - if (info.model_number.empty() && info.model_name.empty() && - info.device_name.empty() && info.oui_list.empty()) - return; - - SystemProfileProto::Network::WifiAccessPoint::VendorInformation* vendor = - access_point_info->mutable_vendor_info(); - if (!info.model_number.empty()) - vendor->set_model_number(info.model_number); - if (!info.model_name.empty()) - vendor->set_model_name(info.model_name); - if (!info.device_name.empty()) - vendor->set_device_name(info.device_name); - - // Return if OUI list is not provided. - if (info.oui_list.empty()) - return; - - // Parse OUI list. - for (const base::StringPiece& oui_str : base::SplitStringPiece( - info.oui_list, " ", base::TRIM_WHITESPACE, base::SPLIT_WANT_ALL)) { - uint32_t oui; - if (base::HexStringToUInt(oui_str, &oui)) { - vendor->add_element_identifier(oui); - } else { - DLOG(WARNING) << "Error when parsing OUI list of the WiFi access point"; - } - } -} - void NetworkMetricsProvider::LogAggregatedMetrics() { DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); base::HistogramBase* error_codes = base::SparseHistogram::FactoryGet( diff --git a/chromium/components/metrics/net/network_metrics_provider.h b/chromium/components/metrics/net/network_metrics_provider.h index 7d8a8682584..9278e319e03 100644 --- a/chromium/components/metrics/net/network_metrics_provider.h +++ b/chromium/components/metrics/net/network_metrics_provider.h @@ -15,7 +15,6 @@ #include "base/sequenced_task_runner.h" #include "base/single_thread_task_runner.h" #include "components/metrics/metrics_provider.h" -#include "components/metrics/net/wifi_access_point_info_provider.h" #include "net/base/network_interfaces.h" #include "net/nqe/effective_connection_type.h" #include "services/network/public/cpp/network_connection_tracker.h" @@ -87,12 +86,6 @@ class NetworkMetricsProvider // net::GetWifiPHYLayerProtocol. void OnWifiPHYLayerProtocolResult(net::WifiPHYLayerProtocol mode); - // Writes info about the wireless access points that this system is - // connected to. - void WriteWifiAccessPointProto( - const WifiAccessPointInfoProvider::WifiAccessPointInfo& info, - SystemProfileProto::Network* network_proto); - // Logs metrics that are functions of other metrics being uploaded. void LogAggregatedMetrics(); @@ -128,9 +121,6 @@ class NetworkMetricsProvider // net::GetWifiPHYLayerProtocol. net::WifiPHYLayerProtocol wifi_phy_layer_protocol_; - // Helper object for retrieving connected wifi access point information. - std::unique_ptr<WifiAccessPointInfoProvider> wifi_access_point_info_provider_; - // These metrics track histogram totals for the Net.ErrorCodesForMainFrame4 // histogram. They are used to compute deltas at upload time. base::HistogramBase::Count total_aborts_; diff --git a/chromium/components/metrics/net/wifi_access_point_info_provider.cc b/chromium/components/metrics/net/wifi_access_point_info_provider.cc deleted file mode 100644 index 21e04b181d2..00000000000 --- a/chromium/components/metrics/net/wifi_access_point_info_provider.cc +++ /dev/null @@ -1,25 +0,0 @@ -// Copyright 2014 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/metrics/net/wifi_access_point_info_provider.h" - -namespace metrics { - -WifiAccessPointInfoProvider::WifiAccessPointInfo::WifiAccessPointInfo() { -} - -WifiAccessPointInfoProvider::WifiAccessPointInfo::~WifiAccessPointInfo() { -} - -WifiAccessPointInfoProvider::WifiAccessPointInfoProvider() { -} - -WifiAccessPointInfoProvider::~WifiAccessPointInfoProvider() { -} - -bool WifiAccessPointInfoProvider::GetInfo(WifiAccessPointInfo *info) { - return false; -} - -} // namespace metrics diff --git a/chromium/components/metrics/net/wifi_access_point_info_provider.h b/chromium/components/metrics/net/wifi_access_point_info_provider.h deleted file mode 100644 index 3a761da1b12..00000000000 --- a/chromium/components/metrics/net/wifi_access_point_info_provider.h +++ /dev/null @@ -1,54 +0,0 @@ -// Copyright 2014 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 COMPONENTS_METRICS_NET_WIFI_ACCESS_POINT_INFO_PROVIDER_H_ -#define COMPONENTS_METRICS_NET_WIFI_ACCESS_POINT_INFO_PROVIDER_H_ - -#include <string> - -#include "base/macros.h" - -namespace metrics { - -// Interface for accessing connected wireless access point information. -class WifiAccessPointInfoProvider { - public: - // Wifi access point security mode definitions. - enum WifiSecurityMode { - WIFI_SECURITY_UNKNOWN = 0, - WIFI_SECURITY_WPA = 1, - WIFI_SECURITY_WEP = 2, - WIFI_SECURITY_RSN = 3, - WIFI_SECURITY_802_1X = 4, - WIFI_SECURITY_PSK = 5, - WIFI_SECURITY_NONE - }; - - // Information of the currently connected wifi access point. - struct WifiAccessPointInfo { - WifiAccessPointInfo(); - ~WifiAccessPointInfo(); - WifiSecurityMode security; - std::string bssid; - std::string model_number; - std::string model_name; - std::string device_name; - std::string oui_list; - }; - - WifiAccessPointInfoProvider(); - virtual ~WifiAccessPointInfoProvider(); - - // Fill in the wifi access point info if device is currently connected to a - // wifi access point. Return true if device is connected to a wifi access - // point, false otherwise. - virtual bool GetInfo(WifiAccessPointInfo *info); - - private: - DISALLOW_COPY_AND_ASSIGN(WifiAccessPointInfoProvider); -}; - -} // namespace metrics - -#endif // COMPONENTS_METRICS_NET_WIFI_ACCESS_POINT_INFO_PROVIDER_H_ diff --git a/chromium/components/metrics/net/wifi_access_point_info_provider_chromeos.cc b/chromium/components/metrics/net/wifi_access_point_info_provider_chromeos.cc deleted file mode 100644 index 91bcfdfa34b..00000000000 --- a/chromium/components/metrics/net/wifi_access_point_info_provider_chromeos.cc +++ /dev/null @@ -1,124 +0,0 @@ -// Copyright 2014 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/metrics/net/wifi_access_point_info_provider_chromeos.h" - -#include <stdint.h> - -#include "base/bind.h" -#include "base/location.h" -#include "base/strings/string_number_conversions.h" -#include "chromeos/network/network_configuration_handler.h" -#include "chromeos/network/network_handler.h" -#include "chromeos/network/network_state.h" -#include "chromeos/network/network_state_handler.h" -#include "chromeos/network/shill_property_util.h" -#include "third_party/cros_system_api/dbus/service_constants.h" - -using chromeos::NetworkHandler; - -namespace metrics { - -WifiAccessPointInfoProviderChromeos::WifiAccessPointInfoProviderChromeos() { - NetworkHandler::Get()->network_state_handler()->AddObserver(this, FROM_HERE); - - // Update initial connection state. - DefaultNetworkChanged( - NetworkHandler::Get()->network_state_handler()->DefaultNetwork()); -} - -WifiAccessPointInfoProviderChromeos::~WifiAccessPointInfoProviderChromeos() { - NetworkHandler::Get()->network_state_handler()->RemoveObserver(this, - FROM_HERE); -} - -bool WifiAccessPointInfoProviderChromeos::GetInfo(WifiAccessPointInfo* info) { - // Wifi access point information is not provided if the BSSID is empty. - // This assumes the BSSID is never empty when access point information exists. - if (wifi_access_point_info_.bssid.empty()) - return false; - - *info = wifi_access_point_info_; - return true; -} - -void WifiAccessPointInfoProviderChromeos::DefaultNetworkChanged( - const chromeos::NetworkState* default_network) { - // Reset access point info to prevent reporting of out-dated data. - wifi_access_point_info_ = WifiAccessPointInfo(); - - // Skip non-wifi connections - if (!default_network || default_network->type() != shill::kTypeWifi) - return; - - // Retrieve access point info for wifi connection. - NetworkHandler::Get()->network_configuration_handler()->GetShillProperties( - default_network->path(), - base::BindRepeating(&WifiAccessPointInfoProviderChromeos::ParseInfo, - AsWeakPtr()), - chromeos::network_handler::ErrorCallback()); -} - -void WifiAccessPointInfoProviderChromeos::ParseInfo( - const std::string &service_path, - const base::DictionaryValue& properties) { - // Skip services that contain "_nomap" in the SSID. - std::string ssid = chromeos::shill_property_util::GetSSIDFromProperties( - properties, false /* verbose_logging */, nullptr); - if (ssid.find("_nomap", 0) != std::string::npos) - return; - - std::string bssid; - if (!properties.GetStringWithoutPathExpansion(shill::kWifiBSsid, &bssid) || - bssid.empty()) - return; - - // Filter out BSSID with local bit set in the first byte. - uint32_t first_octet; - if (!base::HexStringToUInt(bssid.substr(0, 2), &first_octet)) - NOTREACHED(); - if (first_octet & 0x2) - return; - wifi_access_point_info_.bssid = bssid; - - // Parse security info. - std::string security; - properties.GetStringWithoutPathExpansion( - shill::kSecurityProperty, &security); - wifi_access_point_info_.security = WIFI_SECURITY_UNKNOWN; - if (security == shill::kSecurityWpa) - wifi_access_point_info_.security = WIFI_SECURITY_WPA; - else if (security == shill::kSecurityWep) - wifi_access_point_info_.security = WIFI_SECURITY_WEP; - else if (security == shill::kSecurityRsn) - wifi_access_point_info_.security = WIFI_SECURITY_RSN; - else if (security == shill::kSecurity8021x) - wifi_access_point_info_.security = WIFI_SECURITY_802_1X; - else if (security == shill::kSecurityPsk) - wifi_access_point_info_.security = WIFI_SECURITY_PSK; - else if (security == shill::kSecurityNone) - wifi_access_point_info_.security = WIFI_SECURITY_NONE; - - properties.GetStringWithoutPathExpansion( - shill::kWifiBSsid, &wifi_access_point_info_.bssid); - const base::DictionaryValue* vendor_dict = nullptr; - if (!properties.GetDictionaryWithoutPathExpansion( - shill::kWifiVendorInformationProperty, - &vendor_dict)) - return; - - vendor_dict->GetStringWithoutPathExpansion( - shill::kVendorWPSModelNumberProperty, - &wifi_access_point_info_.model_number); - vendor_dict->GetStringWithoutPathExpansion( - shill::kVendorWPSModelNameProperty, - &wifi_access_point_info_.model_name); - vendor_dict->GetStringWithoutPathExpansion( - shill::kVendorWPSDeviceNameProperty, - &wifi_access_point_info_.device_name); - vendor_dict->GetStringWithoutPathExpansion(shill::kVendorOUIListProperty, - &wifi_access_point_info_.oui_list); -} - -} // namespace metrics diff --git a/chromium/components/metrics/net/wifi_access_point_info_provider_chromeos.h b/chromium/components/metrics/net/wifi_access_point_info_provider_chromeos.h deleted file mode 100644 index d3102397741..00000000000 --- a/chromium/components/metrics/net/wifi_access_point_info_provider_chromeos.h +++ /dev/null @@ -1,48 +0,0 @@ -// Copyright 2014 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 COMPONENTS_METRICS_NET_WIFI_ACCESS_POINT_INFO_PROVIDER_CHROMEOS_H_ -#define COMPONENTS_METRICS_NET_WIFI_ACCESS_POINT_INFO_PROVIDER_CHROMEOS_H_ - -#include <string> - -#include "base/macros.h" -#include "base/memory/weak_ptr.h" -#include "base/values.h" -#include "chromeos/network/network_state_handler_observer.h" -#include "components/metrics/net/wifi_access_point_info_provider.h" - -namespace metrics { - -// WifiAccessPointInfoProviderChromeos provides the connected wifi -// acccess point information for chromeos. -class WifiAccessPointInfoProviderChromeos - : public WifiAccessPointInfoProvider, - public chromeos::NetworkStateHandlerObserver, - public base::SupportsWeakPtr<WifiAccessPointInfoProviderChromeos> { - public: - WifiAccessPointInfoProviderChromeos(); - ~WifiAccessPointInfoProviderChromeos() override; - - // WifiAccessPointInfoProvider: - bool GetInfo(WifiAccessPointInfo* info) override; - - // NetworkStateHandlerObserver: - void DefaultNetworkChanged( - const chromeos::NetworkState* default_network) override; - - private: - // Callback from Shill.Service.GetProperties. Parses |properties| to obtain - // the wifi access point information. - void ParseInfo(const std::string& service_path, - const base::DictionaryValue& properties); - - WifiAccessPointInfo wifi_access_point_info_; - - DISALLOW_COPY_AND_ASSIGN(WifiAccessPointInfoProviderChromeos); -}; - -} // namespace metrics - -#endif // COMPONENTS_METRICS_NET_WIFI_ACCESS_POINT_INFO_PROVIDER_CHROMEOS_H_ diff --git a/chromium/components/metrics/reporting_service.cc b/chromium/components/metrics/reporting_service.cc index 4b1f8040d56..777c72f018b 100644 --- a/chromium/components/metrics/reporting_service.cc +++ b/chromium/components/metrics/reporting_service.cc @@ -10,6 +10,7 @@ #include "base/bind.h" #include "base/callback.h" #include "base/command_line.h" +#include "base/logging.h" #include "base/strings/string_number_conversions.h" #include "components/metrics/data_use_tracker.h" #include "components/metrics/log_store.h" @@ -192,6 +193,9 @@ void ReportingService::OnLogUploadComplete(int response_code, } if (upload_succeeded || discard_log) { + if (upload_succeeded) + log_store()->MarkStagedLogAsSent(); + log_store()->DiscardStagedLog(); // Store the updated list to disk now that the removed log is uploaded. log_store()->PersistUnsentLogs(); diff --git a/chromium/components/metrics/reporting_service_unittest.cc b/chromium/components/metrics/reporting_service_unittest.cc index b05815ec351..180b0acc825 100644 --- a/chromium/components/metrics/reporting_service_unittest.cc +++ b/chromium/components/metrics/reporting_service_unittest.cc @@ -56,6 +56,7 @@ class TestLogStore : public LogStore { logs_.pop_front(); staged_log_hash_.clear(); } + void MarkStagedLogAsSent() override {} void PersistUnsentLogs() const override {} void LoadPersistedUnsentLogs() override {} diff --git a/chromium/components/metrics/stability_metrics_helper.cc b/chromium/components/metrics/stability_metrics_helper.cc index a1743301238..375e7ac0d04 100644 --- a/chromium/components/metrics/stability_metrics_helper.cc +++ b/chromium/components/metrics/stability_metrics_helper.cc @@ -12,8 +12,6 @@ #include "base/check.h" #include "base/metrics/histogram_functions.h" #include "base/metrics/histogram_macros.h" -#include "base/metrics/user_metrics.h" -#include "base/notreached.h" #include "base/system/sys_info.h" #include "build/build_config.h" #include "build/buildflag.h" @@ -211,7 +209,6 @@ void StabilityMetricsHelper::BrowserChildProcessCrashed() { } void StabilityMetricsHelper::LogLoadStarted() { - base::RecordAction(base::UserMetricsAction("PageLoad")); IncrementPrefValue(prefs::kStabilityPageLoadCount); IncrementLongPrefsValue(prefs::kUninstallMetricsPageLoadCount); RecordStabilityEvent(StabilityEventType::kPageLoad); diff --git a/chromium/components/metrics/structured/structured_metrics_provider.cc b/chromium/components/metrics/structured/structured_metrics_provider.cc index b91964c8613..10fbd7df8f9 100644 --- a/chromium/components/metrics/structured/structured_metrics_provider.cc +++ b/chromium/components/metrics/structured/structured_metrics_provider.cc @@ -6,6 +6,7 @@ #include <utility> +#include "base/logging.h" #include "base/message_loop/message_loop_current.h" #include "base/strings/string_number_conversions.h" #include "base/values.h" diff --git a/chromium/components/metrics/unsent_log_store.cc b/chromium/components/metrics/unsent_log_store.cc index 70d82f63f03..1951b2d641f 100644 --- a/chromium/components/metrics/unsent_log_store.cc +++ b/chromium/components/metrics/unsent_log_store.cc @@ -4,6 +4,7 @@ #include "components/metrics/unsent_log_store.h" +#include <cmath> #include <memory> #include <string> #include <utility> @@ -28,6 +29,9 @@ const char kLogHashKey[] = "hash"; const char kLogSignatureKey[] = "signature"; const char kLogTimestampKey[] = "timestamp"; const char kLogDataKey[] = "data"; +const char kLogUnsentCountKey[] = "unsent_samples_count"; +const char kLogSentCountKey[] = "sent_samples_count"; +const char kLogPersistedSizeInKbKey[] = "unsent_persisted_size_in_kb"; std::string EncodeToBase64(const std::string& to_convert) { DCHECK(to_convert.data()); @@ -44,15 +48,17 @@ std::string DecodeFromBase64(const std::string& to_convert) { } // namespace -UnsentLogStore::LogInfo::LogInfo() {} -UnsentLogStore::LogInfo::LogInfo( - const UnsentLogStore::LogInfo& other) = default; -UnsentLogStore::LogInfo::~LogInfo() {} - -void UnsentLogStore::LogInfo::Init(UnsentLogStoreMetrics* metrics, - const std::string& log_data, - const std::string& log_timestamp, - const std::string& signing_key) { +UnsentLogStore::LogInfo::LogInfo() = default; +UnsentLogStore::LogInfo::LogInfo(const UnsentLogStore::LogInfo& other) = + default; +UnsentLogStore::LogInfo::~LogInfo() = default; + +void UnsentLogStore::LogInfo::Init( + UnsentLogStoreMetrics* metrics, + const std::string& log_data, + const std::string& log_timestamp, + const std::string& signing_key, + base::Optional<base::HistogramBase::Count> samples_count) { DCHECK(!log_data.empty()); if (!compression::GzipCompress(log_data, &compressed_log_data)) { @@ -75,18 +81,21 @@ void UnsentLogStore::LogInfo::Init(UnsentLogStoreMetrics* metrics, } timestamp = log_timestamp; + this->samples_count = samples_count; } UnsentLogStore::UnsentLogStore(std::unique_ptr<UnsentLogStoreMetrics> metrics, - PrefService* local_state, - const char* pref_name, - size_t min_log_count, - size_t min_log_bytes, - size_t max_log_size, - const std::string& signing_key) + PrefService* local_state, + const char* log_data_pref_name, + const char* metadata_pref_name, + size_t min_log_count, + size_t min_log_bytes, + size_t max_log_size, + const std::string& signing_key) : metrics_(std::move(metrics)), local_state_(local_state), - pref_name_(pref_name), + log_data_pref_name_(log_data_pref_name), + metadata_pref_name_(metadata_pref_name), min_log_count_(min_log_count), min_log_bytes_(min_log_bytes), max_log_size_(max_log_size != 0 ? max_log_size : static_cast<size_t>(-1)), @@ -148,8 +157,15 @@ void UnsentLogStore::DiscardStagedLog() { staged_log_index_ = -1; } +void UnsentLogStore::MarkStagedLogAsSent() { + DCHECK(has_staged_log()); + DCHECK_LT(static_cast<size_t>(staged_log_index_), list_.size()); + if (list_[staged_log_index_].samples_count.has_value()) + total_samples_sent_ += list_[staged_log_index_].samples_count.value(); +} + void UnsentLogStore::PersistUnsentLogs() const { - ListPrefUpdate update(local_state_, pref_name_); + ListPrefUpdate update(local_state_, log_data_pref_name_); // TODO(crbug.com/859477): Verify that the preference has been properly // registered. CHECK(update.Get()); @@ -157,14 +173,17 @@ void UnsentLogStore::PersistUnsentLogs() const { } void UnsentLogStore::LoadPersistedUnsentLogs() { - ReadLogsFromPrefList(*local_state_->GetList(pref_name_)); + ReadLogsFromPrefList(*local_state_->GetList(log_data_pref_name_)); + RecordMetaDataMertics(); } -void UnsentLogStore::StoreLog(const std::string& log_data) { - list_.push_back(LogInfo()); +void UnsentLogStore::StoreLog( + const std::string& log_data, + base::Optional<base::HistogramBase::Count> samples_count) { + list_.emplace_back(); list_.back().Init(metrics_.get(), log_data, base::NumberToString(base::Time::Now().ToTimeT()), - signing_key_); + signing_key_, samples_count); } const std::string& UnsentLogStore::GetLogAtIndex(size_t index) { @@ -173,8 +192,10 @@ const std::string& UnsentLogStore::GetLogAtIndex(size_t index) { return list_[index].compressed_log_data; } -std::string UnsentLogStore::ReplaceLogAtIndex(size_t index, - const std::string& new_log_data) { +std::string UnsentLogStore::ReplaceLogAtIndex( + size_t index, + const std::string& new_log_data, + base::Optional<base::HistogramBase::Count> samples_count) { DCHECK_GE(index, 0U); DCHECK_LT(index, list_.size()); @@ -185,7 +206,8 @@ std::string UnsentLogStore::ReplaceLogAtIndex(size_t index, old_timestamp.swap(list_[index].timestamp); list_[index] = LogInfo(); - list_[index].Init(metrics_.get(), new_log_data, old_timestamp, signing_key_); + list_[index].Init(metrics_.get(), new_log_data, old_timestamp, signing_key_, + samples_count); return old_log_data; } @@ -194,7 +216,11 @@ void UnsentLogStore::Purge() { DiscardStagedLog(); } list_.clear(); - local_state_->ClearPref(pref_name_); + local_state_->ClearPref(log_data_pref_name_); + // The |total_samples_sent_| isn't cleared intentionally because it is still + // meaningful. + if (metadata_pref_name_) + local_state_->ClearPref(metadata_pref_name_); } void UnsentLogStore::ReadLogsFromPrefList(const base::ListValue& list_value) { @@ -257,6 +283,8 @@ void UnsentLogStore::WriteLogsToPrefList(base::ListValue* list_value) const { ++saved_log_count; } int dropped_logs_num = start - 1; + base::HistogramBase::Count unsent_samples_count = 0; + size_t unsent_persisted_size = 0; for (size_t i = start; i < list_.size(); ++i) { size_t log_size = list_[i].compressed_log_data.length(); @@ -273,9 +301,56 @@ void UnsentLogStore::WriteLogsToPrefList(base::ListValue* list_value) const { EncodeToBase64(list_[i].compressed_log_data)); dict_value->SetString(kLogTimestampKey, list_[i].timestamp); list_value->Append(std::move(dict_value)); + + if (list_[i].samples_count.has_value()) { + unsent_samples_count += list_[i].samples_count.value(); + } + unsent_persisted_size += log_size; } if (dropped_logs_num > 0) metrics_->RecordDroppedLogsNum(dropped_logs_num); + + WriteToMetricsPref(unsent_samples_count, total_samples_sent_, + unsent_persisted_size); +} + +void UnsentLogStore::WriteToMetricsPref( + base::HistogramBase::Count unsent_samples_count, + base::HistogramBase::Count sent_samples_count, + size_t unsent_persisted_size) const { + if (metadata_pref_name_ == nullptr) + return; + + DictionaryPrefUpdate update(local_state_, metadata_pref_name_); + base::DictionaryValue* pref_data = update.Get(); + pref_data->SetKey(kLogUnsentCountKey, base::Value(unsent_samples_count)); + pref_data->SetKey(kLogSentCountKey, base::Value(sent_samples_count)); + // Round up to kb. + pref_data->SetKey( + kLogPersistedSizeInKbKey, + base::Value(static_cast<int>(std::ceil(unsent_persisted_size / 1024.0)))); +} + +void UnsentLogStore::RecordMetaDataMertics() { + if (metadata_pref_name_ == nullptr) + return; + + const base::DictionaryValue* value = + local_state_->GetDictionary(metadata_pref_name_); + if (!value) + return; + + auto unsent_samples_count = value->FindIntKey(kLogUnsentCountKey); + auto sent_samples_count = value->FindIntKey(kLogSentCountKey); + auto unsent_persisted_size_in_kb = + value->FindIntKey(kLogPersistedSizeInKbKey); + + if (unsent_samples_count && sent_samples_count && + unsent_persisted_size_in_kb) { + metrics_->RecordLastUnsentLogMetadataMetrics( + unsent_samples_count.value(), sent_samples_count.value(), + unsent_persisted_size_in_kb.value()); + } } } // namespace metrics diff --git a/chromium/components/metrics/unsent_log_store.h b/chromium/components/metrics/unsent_log_store.h index b4f11a65695..596a77b1958 100644 --- a/chromium/components/metrics/unsent_log_store.h +++ b/chromium/components/metrics/unsent_log_store.h @@ -11,8 +11,11 @@ #include <string> #include <vector> +#include "base/gtest_prod_util.h" #include "base/logging.h" #include "base/macros.h" +#include "base/metrics/histogram_base.h" +#include "base/optional.h" #include "base/values.h" #include "components/metrics/log_store.h" @@ -26,10 +29,14 @@ class UnsentLogStoreMetrics; class UnsentLogStore : public LogStore { public: // Constructs an UnsentLogStore that stores data in |local_state| under the - // preference |pref_name|. + // preference |log_data_pref_name|. // Calling code is responsible for ensuring that the lifetime of |local_state| // is longer than the lifetime of UnsentLogStore. // + // The optional |metadata_pref_name| is the preference that is used to store + // the unsent logs info while the unset logs are persisted. That info will be + // recorded as UMA metrics in next browser startup. + // // When saving logs to disk, stores either the first |min_log_count| logs, or // at least |min_log_bytes| bytes of logs, whichever is greater. // @@ -40,12 +47,13 @@ class UnsentLogStore : public LogStore { // data, which will be uploaded with the log and used to validate data // integrity. UnsentLogStore(std::unique_ptr<UnsentLogStoreMetrics> metrics, - PrefService* local_state, - const char* pref_name, - size_t min_log_count, - size_t min_log_bytes, - size_t max_log_size, - const std::string& signing_key); + PrefService* local_state, + const char* log_data_pref_name, + const char* metadata_pref_name, + size_t min_log_count, + size_t min_log_bytes, + size_t max_log_size, + const std::string& signing_key); ~UnsentLogStore(); // LogStore: @@ -56,18 +64,24 @@ class UnsentLogStore : public LogStore { const std::string& staged_log_signature() const override; void StageNextLog() override; void DiscardStagedLog() override; + void MarkStagedLogAsSent() override; void PersistUnsentLogs() const override; void LoadPersistedUnsentLogs() override; - // Adds a log to the list. - void StoreLog(const std::string& log_data); + // Adds a UMA log to the list, |samples_count| is the total number of samples + // in the log (if available). + void StoreLog(const std::string& log_data, + base::Optional<base::HistogramBase::Count> samples_count); // Gets log data at the given index in the list. const std::string& GetLogAtIndex(size_t index); // Replaces the compressed log at |index| in the store with given log data // reusing the same timestamp from the original log, and returns old log data. - std::string ReplaceLogAtIndex(size_t index, const std::string& new_log_data); + std::string ReplaceLogAtIndex( + size_t index, + const std::string& new_log_data, + base::Optional<base::HistogramBase::Count> samples_count); // Deletes all logs, in memory and on disk. void Purge(); @@ -79,12 +93,22 @@ class UnsentLogStore : public LogStore { size_t size() const { return list_.size(); } private: + FRIEND_TEST_ALL_PREFIXES(UnsentLogStoreTest, UnsentLogMetadataMetrics); + // Writes the list to the ListValue. void WriteLogsToPrefList(base::ListValue* list) const; // Reads the list from the ListValue. void ReadLogsFromPrefList(const base::ListValue& list); + // Writes the unsent log info to the |metadata_pref_name_| preference. + void WriteToMetricsPref(base::HistogramBase::Count unsent_samples_count, + base::HistogramBase::Count sent_samples_count, + size_t persisted_size) const; + + // Records the info in |metadata_pref_name_| as UMA metrics. + void RecordMetaDataMertics(); + // An object for recording UMA metrics. std::unique_ptr<UnsentLogStoreMetrics> metrics_; @@ -94,7 +118,11 @@ class UnsentLogStore : public LogStore { PrefService* local_state_; // The name of the preference to serialize logs to/from. - const char* pref_name_; + const char* log_data_pref_name_; + + // The name of the preference to store the unsent logs info, it could be + // nullptr if the metadata isn't desired. + const char* metadata_pref_name_; // We will keep at least this |min_log_count_| logs or |min_log_bytes_| bytes // of logs, whichever is greater, when writing to disk. These apply after @@ -124,7 +152,8 @@ class UnsentLogStore : public LogStore { void Init(UnsentLogStoreMetrics* metrics, const std::string& log_data, const std::string& log_timestamp, - const std::string& signing_key); + const std::string& signing_key, + base::Optional<base::HistogramBase::Count> samples_count); // Compressed log data - a serialized protobuf that's been gzipped. std::string compressed_log_data; @@ -140,6 +169,9 @@ class UnsentLogStore : public LogStore { // The timestamp of when the log was created as a time_t value. std::string timestamp; + + // The total number of samples in this log if applicable. + base::Optional<base::HistogramBase::Count> samples_count; }; // A list of all of the stored logs, stored with SHA1 hashes to check for // corruption while they are stored in memory. @@ -149,6 +181,9 @@ class UnsentLogStore : public LogStore { // staged, the index will be -1. int staged_log_index_; + // The total number of samples that have been sent from this LogStore. + base::HistogramBase::Count total_samples_sent_ = 0; + DISALLOW_COPY_AND_ASSIGN(UnsentLogStore); }; diff --git a/chromium/components/metrics/unsent_log_store_metrics.cc b/chromium/components/metrics/unsent_log_store_metrics.cc new file mode 100644 index 00000000000..426f4fc6b6d --- /dev/null +++ b/chromium/components/metrics/unsent_log_store_metrics.cc @@ -0,0 +1,30 @@ +// Copyright 2020 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/metrics/unsent_log_store_metrics.h" + +namespace metrics { + +// static +const base::Feature UnsentLogStoreMetrics::kRecordLastUnsentLogMetadataMetrics = + {"RecordLastUnsentLogMetadataMetrics", base::FEATURE_DISABLED_BY_DEFAULT}; + +UnsentLogStoreMetrics::UnsentLogStoreMetrics() = default; +UnsentLogStoreMetrics::~UnsentLogStoreMetrics() = default; + +void UnsentLogStoreMetrics::RecordLogReadStatus(LogReadStatus status) {} + +void UnsentLogStoreMetrics::RecordCompressionRatio(size_t compressed_size, + size_t original_size) {} + +void UnsentLogStoreMetrics::RecordDroppedLogSize(size_t size) {} + +void UnsentLogStoreMetrics::RecordDroppedLogsNum(int dropped_logs_num) {} + +void UnsentLogStoreMetrics::RecordLastUnsentLogMetadataMetrics( + int unsent_samples_count, + int sent_samples_count, + int persisted_size_in_kb) {} + +} // namespace metrics diff --git a/chromium/components/metrics/unsent_log_store_metrics.h b/chromium/components/metrics/unsent_log_store_metrics.h index 4e324063ab3..013a0651f2e 100644 --- a/chromium/components/metrics/unsent_log_store_metrics.h +++ b/chromium/components/metrics/unsent_log_store_metrics.h @@ -5,6 +5,7 @@ #ifndef COMPONENTS_METRICS_UNSENT_LOG_STORE_METRICS_H_ #define COMPONENTS_METRICS_UNSENT_LOG_STORE_METRICS_H_ +#include "base/feature_list.h" #include "base/macros.h" #include "components/metrics/unsent_log_store.h" @@ -31,17 +32,25 @@ class UnsentLogStoreMetrics { END_RECALL_STATUS // Number of bins to use to create the histogram. }; - UnsentLogStoreMetrics() {} - virtual ~UnsentLogStoreMetrics() {} + UnsentLogStoreMetrics(); + virtual ~UnsentLogStoreMetrics(); - virtual void RecordLogReadStatus(LogReadStatus status) {} + virtual void RecordLogReadStatus(LogReadStatus status); - virtual void RecordCompressionRatio( - size_t compressed_size, size_t original_size) {} + virtual void RecordCompressionRatio(size_t compressed_size, + size_t original_size); - virtual void RecordDroppedLogSize(size_t size) {} + virtual void RecordDroppedLogSize(size_t size); - virtual void RecordDroppedLogsNum(int dropped_logs_num) {} + virtual void RecordDroppedLogsNum(int dropped_logs_num); + + virtual void RecordLastUnsentLogMetadataMetrics(int unsent_samples_count, + int sent_samples_count, + int persisted_size_in_kb); + + // The feature to record the unsent log info metrics, refer to + // UnsentLogStoreMetricsImpl::RecordLastUnsentLogMetadataMetrics. + static const base::Feature kRecordLastUnsentLogMetadataMetrics; private: DISALLOW_COPY_AND_ASSIGN(UnsentLogStoreMetrics); diff --git a/chromium/components/metrics/unsent_log_store_metrics_impl.cc b/chromium/components/metrics/unsent_log_store_metrics_impl.cc index 7bd7def067a..f533bc4b1c6 100644 --- a/chromium/components/metrics/unsent_log_store_metrics_impl.cc +++ b/chromium/components/metrics/unsent_log_store_metrics_impl.cc @@ -4,19 +4,20 @@ #include "components/metrics/unsent_log_store_metrics_impl.h" -#include "base/metrics/histogram_macros.h" +#include "base/metrics/histogram_functions.h" namespace metrics { void UnsentLogStoreMetricsImpl::RecordLogReadStatus( UnsentLogStoreMetrics::LogReadStatus status) { - UMA_HISTOGRAM_ENUMERATION("PrefService.PersistentLogRecallProtobufs", status, - UnsentLogStoreMetrics::END_RECALL_STATUS); + base::UmaHistogramEnumeration("PrefService.PersistentLogRecallProtobufs", + status, + UnsentLogStoreMetrics::END_RECALL_STATUS); } void UnsentLogStoreMetricsImpl::RecordCompressionRatio( size_t compressed_size, size_t original_size) { - UMA_HISTOGRAM_PERCENTAGE( + base::UmaHistogramPercentage( "UMA.ProtoCompressionRatio", static_cast<int>(100 * compressed_size / original_size)); } @@ -25,7 +26,37 @@ void UnsentLogStoreMetricsImpl::RecordDroppedLogSize(size_t size) { } void UnsentLogStoreMetricsImpl::RecordDroppedLogsNum(int dropped_logs_num) { - UMA_HISTOGRAM_COUNTS_1M("UMA.UnsentLogs.Dropped", dropped_logs_num); + base::UmaHistogramCounts1M("UMA.UnsentLogs.Dropped", dropped_logs_num); +} + +void UnsentLogStoreMetricsImpl::RecordLastUnsentLogMetadataMetrics( + int unsent_samples_count, + int sent_samples_count, + int persisted_size_in_kb) { + if (!base::FeatureList::IsEnabled(kRecordLastUnsentLogMetadataMetrics)) + return; + + if (unsent_samples_count < 0 || sent_samples_count < 0 || + persisted_size_in_kb < 0) { + return; + } + + base::UmaHistogramCounts100000("UMA.UnsentLogs.UnsentCount", + unsent_samples_count); + base::UmaHistogramCounts1M("UMA.UnsentLogs.SentCount", sent_samples_count); + // Sets 10MB as maximum because the total size of logs in each LogStore is up + // to 6MB. + base::UmaHistogramCounts10000("UMA.UnsentLogs.PersistedSizeInKB", + persisted_size_in_kb); + + if (sent_samples_count == 0 && unsent_samples_count == 0) { + base::UmaHistogramPercentage("UMA.UnsentLogs.UnsentPercentage", 0); + } else { + base::UmaHistogramPercentage( + "UMA.UnsentLogs.UnsentPercentage", + 100 * unsent_samples_count / + (unsent_samples_count + sent_samples_count)); + } } } // namespace metrics diff --git a/chromium/components/metrics/unsent_log_store_metrics_impl.h b/chromium/components/metrics/unsent_log_store_metrics_impl.h index 95a15ec40f6..ca548f560f4 100644 --- a/chromium/components/metrics/unsent_log_store_metrics_impl.h +++ b/chromium/components/metrics/unsent_log_store_metrics_impl.h @@ -23,6 +23,9 @@ class UnsentLogStoreMetricsImpl : public UnsentLogStoreMetrics { size_t compressed_size, size_t original_size) override; void RecordDroppedLogSize(size_t size) override; void RecordDroppedLogsNum(int dropped_logs_num) override; + void RecordLastUnsentLogMetadataMetrics(int unsent_samples_count, + int sent_samples_count, + int persisted_size_in_kb) override; private: DISALLOW_COPY_AND_ASSIGN(UnsentLogStoreMetricsImpl); diff --git a/chromium/components/metrics/unsent_log_store_metrics_impl_unittest.cc b/chromium/components/metrics/unsent_log_store_metrics_impl_unittest.cc new file mode 100644 index 00000000000..8027cb6b9c4 --- /dev/null +++ b/chromium/components/metrics/unsent_log_store_metrics_impl_unittest.cc @@ -0,0 +1,82 @@ +// Copyright 2020 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/metrics/unsent_log_store_metrics_impl.h" + +#include "base/test/metrics/histogram_tester.h" +#include "base/test/scoped_feature_list.h" +#include "testing/gtest/include/gtest/gtest.h" + +namespace metrics { + +TEST(UnsentLogStoreMetricsImplTest, RecordLastUnsentLogMetadataMetrics) { + base::test::ScopedFeatureList feature_override; + feature_override.InitAndEnableFeature( + UnsentLogStoreMetrics::kRecordLastUnsentLogMetadataMetrics); + UnsentLogStoreMetricsImpl impl; + base::HistogramTester histogram_tester; + + impl.RecordLastUnsentLogMetadataMetrics(99, 19999, 63); + histogram_tester.ExpectBucketCount("UMA.UnsentLogs.UnsentCount", 99, 1); + histogram_tester.ExpectBucketCount("UMA.UnsentLogs.SentCount", 19999, 1); + histogram_tester.ExpectBucketCount("UMA.UnsentLogs.UnsentPercentage", + 99 * 100 / (99 + 19999), 1); + histogram_tester.ExpectBucketCount("UMA.UnsentLogs.PersistedSizeInKB", 63, 1); +} + +TEST(UnsentLogStoreMetricsImplTest, DisableRecordLastUnsentLogMetadataMetrics) { + UnsentLogStoreMetricsImpl impl; + base::HistogramTester histogram_tester; + + impl.RecordLastUnsentLogMetadataMetrics(99, 19999, 63); + EXPECT_TRUE( + histogram_tester.GetAllSamples("UMA.UnsentLogs.UnsentCount").empty()); + EXPECT_TRUE( + histogram_tester.GetAllSamples("UMA.UnsentLogs.SentCount").empty()); + EXPECT_TRUE(histogram_tester.GetAllSamples("UMA.UnsentLogs.UnsentPercentage") + .empty()); + EXPECT_TRUE(histogram_tester.GetAllSamples("UMA.UnsentLogs.PersistedSizeInKB") + .empty()); +} + +TEST(UnsentLogStoreMetricsImplTest, BothUnsentAndSentZeroSample) { + base::test::ScopedFeatureList feature_override; + feature_override.InitAndEnableFeature( + UnsentLogStoreMetrics::kRecordLastUnsentLogMetadataMetrics); + UnsentLogStoreMetricsImpl impl; + base::HistogramTester histogram_tester; + + impl.RecordLastUnsentLogMetadataMetrics(0, 0, 63); + histogram_tester.ExpectBucketCount("UMA.UnsentLogs.UnsentCount", 0, 1); + histogram_tester.ExpectBucketCount("UMA.UnsentLogs.SentCount", 0, 1); + histogram_tester.ExpectBucketCount("UMA.UnsentLogs.UnsentPercentage", 0, 1); +} + +TEST(UnsentLogStoreMetricsImplTest, ZeroUnsentSample) { + base::test::ScopedFeatureList feature_override; + feature_override.InitAndEnableFeature( + UnsentLogStoreMetrics::kRecordLastUnsentLogMetadataMetrics); + UnsentLogStoreMetricsImpl impl; + base::HistogramTester histogram_tester; + + impl.RecordLastUnsentLogMetadataMetrics(0, 999999, 63); + histogram_tester.ExpectBucketCount("UMA.UnsentLogs.UnsentCount", 0, 1); + histogram_tester.ExpectBucketCount("UMA.UnsentLogs.SentCount", 999999, 1); + histogram_tester.ExpectBucketCount("UMA.UnsentLogs.UnsentPercentage", 0, 1); +} + +TEST(UnsentLogStoreMetricsImplTest, ZeroSentSample) { + base::test::ScopedFeatureList feature_override; + feature_override.InitAndEnableFeature( + UnsentLogStoreMetrics::kRecordLastUnsentLogMetadataMetrics); + UnsentLogStoreMetricsImpl impl; + base::HistogramTester histogram_tester; + + impl.RecordLastUnsentLogMetadataMetrics(999, 0, 63); + histogram_tester.ExpectBucketCount("UMA.UnsentLogs.UnsentCount", 999, 1); + histogram_tester.ExpectBucketCount("UMA.UnsentLogs.SentCount", 0, 1); + histogram_tester.ExpectBucketCount("UMA.UnsentLogs.UnsentPercentage", 100, 1); +} + +} // namespace metrics diff --git a/chromium/components/metrics/unsent_log_store_unittest.cc b/chromium/components/metrics/unsent_log_store_unittest.cc index 02bc9a05973..101991551b8 100644 --- a/chromium/components/metrics/unsent_log_store_unittest.cc +++ b/chromium/components/metrics/unsent_log_store_unittest.cc @@ -23,6 +23,8 @@ namespace metrics { namespace { const char kTestPrefName[] = "TestPref"; +const char kTestMetaDataPrefName[] = "TestMetaDataPref"; + const size_t kLogCountLimit = 3; const size_t kLogByteLimit = 1000; @@ -52,6 +54,7 @@ class UnsentLogStoreTest : public testing::Test { public: UnsentLogStoreTest() { prefs_.registry()->RegisterListPref(kTestPrefName); + prefs_.registry()->RegisterDictionaryPref(kTestMetaDataPrefName); } protected: @@ -61,15 +64,38 @@ class UnsentLogStoreTest : public testing::Test { DISALLOW_COPY_AND_ASSIGN(UnsentLogStoreTest); }; +class TestUnsentLogStoreMetrics : public UnsentLogStoreMetrics { + public: + TestUnsentLogStoreMetrics() = default; + + void RecordLastUnsentLogMetadataMetrics(int unsent_samples_count, + int sent_samples_count, + int persisted_size_in_kb) override { + unsent_samples_count_ = unsent_samples_count; + sent_samples_count_ = sent_samples_count; + persisted_size_in_kb_ = persisted_size_in_kb; + } + + int unsent_samples_count() const { return unsent_samples_count_; } + int sent_samples_count() const { return sent_samples_count_; } + int persisted_size_in_kb() const { return persisted_size_in_kb_; } + + private: + int unsent_samples_count_ = 0; + int sent_samples_count_ = 0; + int persisted_size_in_kb_ = 0; +}; + class TestUnsentLogStore : public UnsentLogStore { public: TestUnsentLogStore(PrefService* service, size_t min_log_bytes) : UnsentLogStore(std::make_unique<UnsentLogStoreMetricsImpl>(), service, kTestPrefName, + nullptr, kLogCountLimit, min_log_bytes, - 0, + /* max_log_size= */ 0, std::string()) {} TestUnsentLogStore(PrefService* service, size_t min_log_bytes, @@ -77,10 +103,22 @@ class TestUnsentLogStore : public UnsentLogStore { : UnsentLogStore(std::make_unique<UnsentLogStoreMetricsImpl>(), service, kTestPrefName, + nullptr, kLogCountLimit, min_log_bytes, - 0, + /* max_log_size = */ 0, signing_key) {} + TestUnsentLogStore(std::unique_ptr<UnsentLogStoreMetrics> metrics, + PrefService* service, + size_t max_log_size) + : UnsentLogStore(std::move(metrics), + service, + kTestPrefName, + kTestMetaDataPrefName, + kLogCountLimit, + /* min_log_bytes= */ 1, + max_log_size, + std::string()) {} // Stages and removes the next log, while testing it's value. void ExpectNextLog(const std::string& expected_log) { @@ -112,7 +150,7 @@ TEST_F(UnsentLogStoreTest, EmptyLogList) { TEST_F(UnsentLogStoreTest, SingleElementLogList) { TestUnsentLogStore unsent_log_store(&prefs_, kLogByteLimit); - unsent_log_store.StoreLog("Hello world!"); + unsent_log_store.StoreLog("Hello world!", base::nullopt); unsent_log_store.PersistUnsentLogs(); TestUnsentLogStore result_unsent_log_store(&prefs_, kLogByteLimit); @@ -139,7 +177,7 @@ TEST_F(UnsentLogStoreTest, LongButTinyLogList) { size_t log_count = kLogCountLimit * 5; for (size_t i = 0; i < log_count; ++i) - unsent_log_store.StoreLog("x"); + unsent_log_store.StoreLog("x", base::nullopt); unsent_log_store.PersistUnsentLogs(); @@ -170,13 +208,13 @@ TEST_F(UnsentLogStoreTest, LongButSmallLogList) { (log_count - 4) * Compress(blank_log).length(); TestUnsentLogStore unsent_log_store(&prefs_, min_log_bytes); - unsent_log_store.StoreLog("one"); - unsent_log_store.StoreLog("two"); - unsent_log_store.StoreLog(first_kept); + unsent_log_store.StoreLog("one", base::nullopt); + unsent_log_store.StoreLog("two", base::nullopt); + unsent_log_store.StoreLog(first_kept, base::nullopt); for (size_t i = unsent_log_store.size(); i < log_count - 1; ++i) { - unsent_log_store.StoreLog(blank_log); + unsent_log_store.StoreLog(blank_log, base::nullopt); } - unsent_log_store.StoreLog(last_kept); + unsent_log_store.StoreLog(last_kept, base::nullopt); unsent_log_store.PersistUnsentLogs(); TestUnsentLogStore result_unsent_log_store(&prefs_, kLogByteLimit); @@ -200,7 +238,7 @@ TEST_F(UnsentLogStoreTest, ShortButLargeLogList) { TestUnsentLogStore unsent_log_store(&prefs_, kLogByteLimit); for (size_t i = 0; i < log_count; ++i) { - unsent_log_store.StoreLog(log_data); + unsent_log_store.StoreLog(log_data, base::nullopt); } unsent_log_store.PersistUnsentLogs(); @@ -225,9 +263,9 @@ TEST_F(UnsentLogStoreTest, LongAndLargeLogList) { std::string log_data = GenerateLogWithMinCompressedSize(log_size); for (size_t i = 0; i < log_count; ++i) { if (i == log_count - kLogCountLimit) - unsent_log_store.StoreLog(target_log); + unsent_log_store.StoreLog(target_log, base::nullopt); else - unsent_log_store.StoreLog(log_data); + unsent_log_store.StoreLog(log_data, base::nullopt); } unsent_log_store.PersistUnsentLogs(); @@ -248,13 +286,13 @@ TEST_F(UnsentLogStoreTest, Staging) { std::string tmp; EXPECT_FALSE(unsent_log_store.has_staged_log()); - unsent_log_store.StoreLog("one"); + unsent_log_store.StoreLog("one", base::nullopt); EXPECT_FALSE(unsent_log_store.has_staged_log()); - unsent_log_store.StoreLog("two"); + unsent_log_store.StoreLog("two", base::nullopt); unsent_log_store.StageNextLog(); EXPECT_TRUE(unsent_log_store.has_staged_log()); EXPECT_EQ(unsent_log_store.staged_log(), Compress("two")); - unsent_log_store.StoreLog("three"); + unsent_log_store.StoreLog("three", base::nullopt); EXPECT_EQ(unsent_log_store.staged_log(), Compress("two")); EXPECT_EQ(unsent_log_store.size(), 3U); unsent_log_store.DiscardStagedLog(); @@ -275,9 +313,9 @@ TEST_F(UnsentLogStoreTest, DiscardOrder) { // a log is staged. TestUnsentLogStore unsent_log_store(&prefs_, kLogByteLimit); - unsent_log_store.StoreLog("one"); + unsent_log_store.StoreLog("one", base::nullopt); unsent_log_store.StageNextLog(); - unsent_log_store.StoreLog("two"); + unsent_log_store.StoreLog("two", base::nullopt); unsent_log_store.DiscardStagedLog(); unsent_log_store.PersistUnsentLogs(); @@ -293,7 +331,7 @@ TEST_F(UnsentLogStoreTest, Hashes) { const std::string foo_hash = base::SHA1HashString(kFooText); TestUnsentLogStore unsent_log_store(&prefs_, kLogByteLimit); - unsent_log_store.StoreLog(kFooText); + unsent_log_store.StoreLog(kFooText, base::nullopt); unsent_log_store.StageNextLog(); EXPECT_EQ(Compress(kFooText), unsent_log_store.staged_log()); @@ -304,7 +342,7 @@ TEST_F(UnsentLogStoreTest, Signatures) { const char kFooText[] = "foo"; TestUnsentLogStore unsent_log_store(&prefs_, kLogByteLimit); - unsent_log_store.StoreLog(kFooText); + unsent_log_store.StoreLog(kFooText, base::nullopt); unsent_log_store.StageNextLog(); EXPECT_EQ(Compress(kFooText), unsent_log_store.staged_log()); @@ -330,7 +368,7 @@ TEST_F(UnsentLogStoreTest, Signatures) { TestUnsentLogStore unsent_log_store_different_key(&prefs_, kLogByteLimit, key); - unsent_log_store_different_key.StoreLog(kFooText); + unsent_log_store_different_key.StoreLog(kFooText, base::nullopt); unsent_log_store_different_key.StageNextLog(); EXPECT_EQ(Compress(kFooText), unsent_log_store_different_key.staged_log()); @@ -345,4 +383,72 @@ TEST_F(UnsentLogStoreTest, Signatures) { EXPECT_EQ(expected_signature_base64, actual_signature_base64); } +TEST_F(UnsentLogStoreTest, UnsentLogMetadataMetrics) { + std::unique_ptr<TestUnsentLogStoreMetrics> metrics = + std::make_unique<TestUnsentLogStoreMetrics>(); + TestUnsentLogStoreMetrics* m = metrics.get(); + TestUnsentLogStore unsent_log_store(std::move(metrics), &prefs_, + kLogByteLimit * 10); + + // Prepare 4 logs. + const char kFooText[] = "foo"; + const base::HistogramBase::Count kFooSampleCount = 3; + + // The |foobar_log| whose compressed size is over 1kb will be staged first, so + // the persisted_size_in_kb shall be reduced by 1kb afterwards. + std::string foobar_log = GenerateLogWithMinCompressedSize(1024); + const base::HistogramBase::Count kFooBarSampleCount = 5; + + // The |oversize_log| shall not be persisted. + std::string oversize_log = + GenerateLogWithMinCompressedSize(kLogByteLimit * 10 + 1); + const base::HistogramBase::Count kOversizeLogSampleCount = 50; + + // The log without the SampleCount will not be counted to metrics. + const char kNoSampleLog[] = "no sample log"; + + unsent_log_store.StoreLog( + oversize_log, + base::make_optional<base::HistogramBase::Count>(kOversizeLogSampleCount)); + unsent_log_store.StoreLog(kNoSampleLog, base::nullopt); + unsent_log_store.StoreLog( + kFooText, base::Optional<base::HistogramBase::Count>(kFooSampleCount)); + // The foobar_log will be staged first. + unsent_log_store.StoreLog( + foobar_log, + base::Optional<base::HistogramBase::Count>(kFooBarSampleCount)); + + unsent_log_store.PersistUnsentLogs(); + + unsent_log_store.RecordMetaDataMertics(); + // The |oversize_log| was ignored, the kNoSampleLog won't be counted to + // metrics, + EXPECT_EQ(kFooSampleCount + kFooBarSampleCount, m->unsent_samples_count()); + EXPECT_EQ(0, m->sent_samples_count()); + EXPECT_EQ(2, m->persisted_size_in_kb()); + + // Pretend to send log. + unsent_log_store.StageNextLog(); + unsent_log_store.MarkStagedLogAsSent(); + unsent_log_store.DiscardStagedLog(); + unsent_log_store.PersistUnsentLogs(); + unsent_log_store.RecordMetaDataMertics(); + + // The |foobar_log| shall be sent. + EXPECT_EQ(kFooSampleCount, m->unsent_samples_count()); + EXPECT_EQ(kFooBarSampleCount, m->sent_samples_count()); + EXPECT_EQ(1, m->persisted_size_in_kb()); + + // Pretend |kFooText| upload failure. + unsent_log_store.StageNextLog(); + unsent_log_store.DiscardStagedLog(); + unsent_log_store.PersistUnsentLogs(); + unsent_log_store.RecordMetaDataMertics(); + + // Verify the failed upload wasn't added to the sent samples count. + EXPECT_EQ(0, m->unsent_samples_count()); + EXPECT_EQ(kFooBarSampleCount, m->sent_samples_count()); + EXPECT_EQ(1, m->persisted_size_in_kb()); +} + } // namespace metrics |