Skip to content

Commit

Permalink
[UKM] Create a Source ID type for metrics with no URL
Browse files Browse the repository at this point in the history
Also whitelist it, so it is not omitted when logging metrics.
Adds convenience ukm::NoIdentifierSourceId() to get a source id of this
new type.

cc/trees/layer_tree_host_unittest_checkerimaging.cc was updated as it was
triggering the new DCHECK in components/ukm/ukm_recorder_impl.cc

Bug: 1204578
Change-Id: I13a85fa22aea58899618dd69a514ac9a8949d130
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2863253
Reviewed-by: Yue Ru Sun <yrsun@chromium.org>
Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
Reviewed-by: Robert Kaplow <rkaplow@chromium.org>
Commit-Queue: Javier Flores <javierrobles@chromium.org>
Cr-Commit-Position: refs/heads/master@{#879423}
  • Loading branch information
Javier Ernesto Flores Robles authored and Chromium LUCI CQ committed May 5, 2021
1 parent 6751cf5 commit 22fbdfc
Show file tree
Hide file tree
Showing 4 changed files with 34 additions and 14 deletions.
10 changes: 6 additions & 4 deletions cc/trees/layer_tree_host_unittest_checkerimaging.cc
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,9 @@ const char kCheckerboardImagesMetric[] = "CheckerboardedImagesCount";

class LayerTreeHostCheckerImagingTest : public LayerTreeTest {
public:
LayerTreeHostCheckerImagingTest() : url_(GURL("https://example.com")),
ukm_source_id_(123) {}
LayerTreeHostCheckerImagingTest()
: url_(GURL("https://example.com")),
ukm_source_id_(ukm::AssignNewSourceId()) {}

void BeginTest() override {
layer_tree_host()->SetSourceURL(ukm_source_id_, url_);
Expand All @@ -36,8 +37,9 @@ class LayerTreeHostCheckerImagingTest : public LayerTreeTest {
recorder->UpdateSourceURL(ukm_source_id_, url_);

// Change the source to ensure any accumulated metrics are flushed.
impl->ukm_manager()->SetSourceId(200);
recorder->UpdateSourceURL(200, GURL("chrome://test2"));
ukm::SourceId newSourceId = ukm::AssignNewSourceId();
impl->ukm_manager()->SetSourceId(newSourceId);
recorder->UpdateSourceURL(newSourceId, GURL("chrome://test2"));

const auto& entries = recorder->GetEntriesByName(kRenderingEvent);
ASSERT_EQ(1u, entries.size());
Expand Down
21 changes: 12 additions & 9 deletions components/ukm/ukm_recorder_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -48,11 +48,11 @@ std::string GetWhitelistEntries() {
}

bool IsWhitelistedSourceId(SourceId source_id) {
return GetSourceIdType(source_id) == SourceIdType::NAVIGATION_ID ||
GetSourceIdType(source_id) == SourceIdType::APP_ID ||
GetSourceIdType(source_id) == SourceIdType::HISTORY_ID ||
GetSourceIdType(source_id) == SourceIdType::WEBAPK_ID ||
GetSourceIdType(source_id) == SourceIdType::PAYMENT_APP_ID;
SourceIdType type = GetSourceIdType(source_id);
return type == SourceIdType::NAVIGATION_ID || type == SourceIdType::APP_ID ||
type == SourceIdType::HISTORY_ID || type == SourceIdType::WEBAPK_ID ||
type == SourceIdType::PAYMENT_APP_ID ||
type == SourceIdType::NO_URL_ID;
}

// Returns whether |url| has one of the schemes supported for logging to UKM.
Expand Down Expand Up @@ -337,10 +337,12 @@ void UkmRecorderImpl::StoreRecordingsInReport(Report* report) {
for (const auto& kv : recordings_.sources) {
// Don't keep sources of these types after current report because their
// entries are logged only at source creation time.
if (GetSourceIdType(kv.first) == ukm::SourceIdObj::Type::APP_ID ||
GetSourceIdType(kv.first) == ukm::SourceIdObj::Type::HISTORY_ID ||
GetSourceIdType(kv.first) == ukm::SourceIdObj::Type::WEBAPK_ID ||
GetSourceIdType(kv.first) == SourceIdType::PAYMENT_APP_ID) {
SourceIdType type = GetSourceIdType(kv.first);
if (type == ukm::SourceIdObj::Type::APP_ID ||
type == ukm::SourceIdObj::Type::HISTORY_ID ||
type == ukm::SourceIdObj::Type::WEBAPK_ID ||
type == SourceIdType::PAYMENT_APP_ID ||
type == SourceIdType::NO_URL_ID) {
MarkSourceForDeletion(kv.first);
}
// If the source id is not whitelisted, don't send it unless it has
Expand Down Expand Up @@ -579,6 +581,7 @@ int UkmRecorderImpl::PruneOldSources(size_t max_kept_sources) {
void UkmRecorderImpl::UpdateSourceURL(SourceId source_id,
const GURL& unsanitized_url) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
DCHECK(GetSourceIdType(source_id) != SourceIdType::NO_URL_ID);

if (base::Contains(recordings_.sources, source_id))
return;
Expand Down
7 changes: 7 additions & 0 deletions services/metrics/public/cpp/ukm_source_id.cc
Original file line number Diff line number Diff line change
Expand Up @@ -82,4 +82,11 @@ SourceIdType GetSourceIdType(SourceId source_id) {
return ukm::SourceIdObj::FromInt64(source_id).GetType();
}

SourceId NoURLSourceId() {
static const SourceId source_id =
SourceIdObj::FromOtherId(AssignNewSourceId(), SourceIdType::NO_URL_ID)
.ToInt64();
return source_id;
}

} // namespace ukm
10 changes: 9 additions & 1 deletion services/metrics/public/cpp/ukm_source_id.h
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,12 @@ class METRICS_EXPORT SourceIdObj {
// shared workers and service workers). Shared workers and service workers
// can be connected to multiple clients (e.g. documents or other workers).
WORKER_ID = 7,
kMaxValue = WORKER_ID,
// Source ID type for metrics that doesn't need to be associated with a
// specific URL. Metrics with this type will be whitelisted and always
// recorded. A source ID of this type can be obtained with NoURLSourceId().
NO_URL_ID = 8,

kMaxValue = NO_URL_ID,
};

// Default constructor has the invalid value.
Expand Down Expand Up @@ -119,6 +124,9 @@ METRICS_EXPORT SourceId AssignNewSourceId();
METRICS_EXPORT SourceId ConvertToSourceId(int64_t other_id,
SourceIdType id_type);

// Utility for getting source ID with NO_URL_ID type.
METRICS_EXPORT SourceId NoURLSourceId();

// Get the SourceIdType of the SourceId object.
METRICS_EXPORT SourceIdType GetSourceIdType(SourceId source_id);

Expand Down

0 comments on commit 22fbdfc

Please sign in to comment.