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

Issue 764465 link

Starred by 1 user
Status: Fixed
Owner: ----
Closed: Apr 2018
Cc:
skare@chromium.org
isherman@chromium.org
asvitk...@chromium.org
█ rkaplow@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Task



Sign in to add a comment

Ensure there is only ever one instance of UkmRecorder in browser process

Project Member Reported by bmcquade@chromium.org, Sep 12 2017 
In tests, it's possible to have multiple UkmRecorder instances accessible at the same time: both a TestUkmRecorder instance and a UkmService (which implements UkmRecorder) can be accessed, as UkmService is accessible via MetricsServiceClient::GetUkmService().

We should make it impossible to have multiple instances.

Steven suggested:
"One direction I have considered going is to make UkmService compose rather than inherit from UkmRecorder, which might make it possible to have a UkmService with a TestUkmRecorder."

We should consider switching to this composition-based approach, where the UkmService owns a UkmRecorder(either test or real) instance.
Project Member

Comment 1 by bugdroid1@chromium.org, Oct 18 2017 

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

commit 65dab1ae7e53d8f21f578629528aab2c951fc440
Author: Steven Holte <holte@google.com>
Date: Wed Oct 18 00:12:07 2017

Add DelegatingUkmRecorder and remove BrowserProcess::ukm_recorder

Bug:  764463 , 764465 
Change-Id: I54e0d215152b705d2dc92a9ed258886db1339d33
Reviewed-on: https://chromium-review.googlesource.com/701933
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Robert Kaplow <rkaplow@chromium.org>
Reviewed-by: Bryan McQuade <bmcquade@chromium.org>
Commit-Queue: Steven Holte <holte@chromium.org>
Cr-Commit-Position: refs/heads/master@{#509617}
[modify] https://crrev.com/65dab1ae7e53d8f21f578629528aab2c951fc440/chrome/browser/android/contextualsearch/contextual_search_ranker_logger_impl.cc
[modify] https://crrev.com/65dab1ae7e53d8f21f578629528aab2c951fc440/chrome/browser/browser_process.h
[modify] https://crrev.com/65dab1ae7e53d8f21f578629528aab2c951fc440/chrome/browser/browser_process_impl.cc
[modify] https://crrev.com/65dab1ae7e53d8f21f578629528aab2c951fc440/chrome/browser/browser_process_impl.h
[modify] https://crrev.com/65dab1ae7e53d8f21f578629528aab2c951fc440/chrome/browser/chrome_content_browser_client.cc
[modify] https://crrev.com/65dab1ae7e53d8f21f578629528aab2c951fc440/chrome/browser/metrics/process_memory_metrics_emitter_browsertest.cc
[modify] https://crrev.com/65dab1ae7e53d8f21f578629528aab2c951fc440/chrome/browser/metrics/ukm_browsertest.cc
[modify] https://crrev.com/65dab1ae7e53d8f21f578629528aab2c951fc440/chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc
[modify] https://crrev.com/65dab1ae7e53d8f21f578629528aab2c951fc440/chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc
[modify] https://crrev.com/65dab1ae7e53d8f21f578629528aab2c951fc440/chrome/browser/page_load_metrics/observers/local_network_requests_page_load_metrics_observer.cc
[modify] https://crrev.com/65dab1ae7e53d8f21f578629528aab2c951fc440/chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc
[modify] https://crrev.com/65dab1ae7e53d8f21f578629528aab2c951fc440/chrome/browser/page_load_metrics/observers/previews_ukm_observer.cc
[modify] https://crrev.com/65dab1ae7e53d8f21f578629528aab2c951fc440/chrome/browser/page_load_metrics/observers/service_worker_page_load_metrics_observer.cc
[modify] https://crrev.com/65dab1ae7e53d8f21f578629528aab2c951fc440/chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer.cc
[modify] https://crrev.com/65dab1ae7e53d8f21f578629528aab2c951fc440/chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc
[modify] https://crrev.com/65dab1ae7e53d8f21f578629528aab2c951fc440/chrome/browser/password_manager/chrome_password_manager_client.cc
[modify] https://crrev.com/65dab1ae7e53d8f21f578629528aab2c951fc440/chrome/browser/payments/android/journey_logger_android.cc
[modify] https://crrev.com/65dab1ae7e53d8f21f578629528aab2c951fc440/chrome/browser/payments/chrome_payment_request_delegate.cc
[modify] https://crrev.com/65dab1ae7e53d8f21f578629528aab2c951fc440/chrome/browser/plugins/plugin_info_message_filter.cc
[modify] https://crrev.com/65dab1ae7e53d8f21f578629528aab2c951fc440/chrome/browser/resource_coordinator/resource_coordinator_web_contents_observer.cc
[modify] https://crrev.com/65dab1ae7e53d8f21f578629528aab2c951fc440/chrome/browser/resource_coordinator/tab_manager_web_contents_data.cc
[modify] https://crrev.com/65dab1ae7e53d8f21f578629528aab2c951fc440/chrome/browser/translate/translate_ranker_factory.cc
[modify] https://crrev.com/65dab1ae7e53d8f21f578629528aab2c951fc440/chrome/browser/ui/autofill/chrome_autofill_client.cc
[modify] https://crrev.com/65dab1ae7e53d8f21f578629528aab2c951fc440/chrome/test/base/testing_browser_process.cc
[modify] https://crrev.com/65dab1ae7e53d8f21f578629528aab2c951fc440/chrome/test/base/testing_browser_process.h
[modify] https://crrev.com/65dab1ae7e53d8f21f578629528aab2c951fc440/components/ukm/content/source_url_recorder_browsertest.cc
[modify] https://crrev.com/65dab1ae7e53d8f21f578629528aab2c951fc440/components/ukm/test_ukm_recorder.cc
[modify] https://crrev.com/65dab1ae7e53d8f21f578629528aab2c951fc440/components/ukm/ukm_service.cc
[modify] https://crrev.com/65dab1ae7e53d8f21f578629528aab2c951fc440/services/metrics/public/cpp/BUILD.gn
[add] https://crrev.com/65dab1ae7e53d8f21f578629528aab2c951fc440/services/metrics/public/cpp/delegating_ukm_recorder.cc
[add] https://crrev.com/65dab1ae7e53d8f21f578629528aab2c951fc440/services/metrics/public/cpp/delegating_ukm_recorder.h
[modify] https://crrev.com/65dab1ae7e53d8f21f578629528aab2c951fc440/services/metrics/public/cpp/ukm_recorder.cc
[modify] https://crrev.com/65dab1ae7e53d8f21f578629528aab2c951fc440/services/metrics/public/cpp/ukm_recorder.h

Comment 2 by holte@chromium.org, Apr 27 2018

Status: Fixed (was: Available)
The DelegatingUkmRecorder solves this issue for the browser process.
►

Sign in to add a comment