Extensions: Add metrics to distinguish between the different kinds of declarative API function call…

extensions/browser/api/declarative/declarative_api.cc

Index: extensions/browser/api/declarative/declarative_api.cc
diff --git a/extensions/browser/api/declarative/declarative_api.cc b/extensions/browser/api/declarative/declarative_api.cc
index 8049813f6c18121e5084c8aa23cc54f4a13e53e0..38c8cad4c851b611eb66679980d754c6c4b50179 100644
--- a/extensions/browser/api/declarative/declarative_api.cc
+++ b/extensions/browser/api/declarative/declarative_api.cc
@@ -9,7 +9,9 @@
 #include "base/base64.h"
 #include "base/bind.h"
 #include "base/bind_helpers.h"
+#include "base/metrics/histogram_macros.h"
 #include "base/single_thread_task_runner.h"
+#include "base/strings/string_util.h"
 #include "base/task_runner_util.h"
 #include "base/values.h"
 #include "content/public/browser/browser_thread.h"
@@ -35,7 +37,56 @@ namespace extensions {
 
 namespace {
 
-const char kDeclarativeEventPrefix[] = "declarative";
+constexpr char kDeclarativeEventPrefix[] = "declarative";
+constexpr char kDeclarativeContentPrefix[] = "declarativeContent";

[lazyboy, 2017/05/05 21:07:42]

All of these should have "." at the end to be safe.

[karandeepb, 2017/05/05 22:33:42]

Done.

+constexpr char kDeclarativeWebRequestPrefix[] = "declarativeWebRequest";
+constexpr char kDeclarativeWebRequestWebViewPrefix[] =
+    "webViewInternal.declarativeWebRequest";
+
+// The type of Declarative API. To collect more granular metrics, a distinction
+// is made when the declarative web request API is used from a webview.
+enum class DeclarativeAPIType {
+  DECLARATIVE_CONTENT,

[lazyboy, 2017/05/05 21:07:42]

For newly added enums, I think we started preferring kConstant style instead of
macro style names.

[karandeepb, 2017/05/05 22:33:42]

Correct - https://codereview.chromium.org/2820553002. Done.

+  DECLARATIVE_WEB_REQUEST,
+  DECLARATIVE_WEB_REQUEST_WEBVIEW,

[karandeepb, 2017/05/05 20:23:24]

These are the 3 event name variations I could find for declarative events.

+  UNKNOWN,

[lazyboy, 2017/05/05 21:07:41]

nit: DECLARATIVE_API_TYPE_UNKNOWN, same for MAX below

[karandeepb, 2017/05/05 22:33:42]

This is an enum class. So the enum type would always be a prefix anyway. Done
for MAX.

[lazyboy, 2017/05/05 23:11:16]

Ah right. Maybe we can also drop "Declarative" substrs from the other ones then?

[karandeepb, 2017/05/05 23:35:05]

Done.

+};
+
+// Describes the possible types of declarative API calls.
+// These values are written to logs.  New enum values can be added, but existing

[lazyboy, 2017/05/05 23:11:16]

nit: These values are recorded as UMA.

[karandeepb, 2017/05/05 23:35:05]

Done.

+// enum values must never be renumbered or deleted and reused.
+enum DeclarativeAPICallType {

[lazyboy, 2017/05/05 21:07:42]

rename to DeclarativeAPIFunctionName

[karandeepb, 2017/05/05 22:33:42]

But these aren't exactly function names. Does DeclarativeAPIFunctionCallType
work?

[lazyboy, 2017/05/05 23:11:16]

They correspond to get/add/remove functions, right? But I'm not strong on the
naming here, except the "Call" part. I'd remove "Call":
DeclarativeAPIFunctionCallType -> DeclarativeAPIFunctionType OK?

[karandeepb, 2017/05/05 23:35:05]

Yeah. Changed to DeclarativeAPIFunctionType

+  DECLARATIVE_CONTENT_ADD_RULES = 0,
+  DECLARATIVE_CONTENT_REMOVE_RULES = 1,
+  DECLARATIVE_CONTENT_GET_RULES = 2,
+  DECLARATIVE_WEB_REQUEST_ADD_RULES = 3,
+  DECLARATIVE_WEB_REQUEST_REMOVE_RULES = 4,
+  DECLARATIVE_WEB_REQUEST_GET_RULES = 5,
+  DECLARATIVE_WEB_REQUEST_WEBVIEW_ADD_RULES = 6,
+  DECLARATIVE_WEB_REQUEST_WEBVIEW_REMOVE_RULES = 7,
+  DECLARATIVE_WEB_REQUEST_WEBVIEW_GET_RULES = 8,
+  MAX,
+};
+
+DeclarativeAPIType GetDeclarativeAPITypeFromEventName(
+    const std::string& event_name) {
+  if (base::StartsWith(event_name, kDeclarativeContentPrefix,
+                       base::CompareCase::INSENSITIVE_ASCII))

[lazyboy, 2017/05/05 21:07:42]

These should be case sensitive.

[karandeepb, 2017/05/05 22:33:42]

Had resolved this.

+    return DeclarativeAPIType::DECLARATIVE_CONTENT;
+  if (base::StartsWith(event_name, kDeclarativeWebRequestPrefix,
+                       base::CompareCase::INSENSITIVE_ASCII))
+    return DeclarativeAPIType::DECLARATIVE_WEB_REQUEST;
+  if (base::StartsWith(event_name, kDeclarativeWebRequestWebViewPrefix,
+                       base::CompareCase::INSENSITIVE_ASCII))
+    return DeclarativeAPIType::DECLARATIVE_WEB_REQUEST_WEBVIEW;
+  return DeclarativeAPIType::UNKNOWN;
+}
+
+void LogAPICallHelper(DeclarativeAPICallType call_type) {
+  DCHECK_LT(call_type, MAX);
+  UMA_HISTOGRAM_ENUMERATION("Extensions.DeclarativeAPICallType", call_type,

[lazyboy, 2017/05/05 21:07:42]

I'd name it Extensions.DeclarativeAPIFunctionCalls or similar, to make it
similar to Extensions.FunctionCalls UMA.

[karandeepb, 2017/05/05 22:33:42]

Done.

+                            MAX);
+}
 
 void ConvertBinaryDictionaryValuesToBase64(base::DictionaryValue* dict);
 
@@ -125,6 +176,8 @@ bool RulesFunction::RunAsync() {
   EXTENSION_FUNCTION_VALIDATE(args_->GetInteger(1, &web_view_instance_id));
   int embedder_process_id = render_frame_host()->GetProcess()->GetID();
 
+  LogAPICall(event_name);
+
   bool from_web_view = web_view_instance_id != 0;
   // If we are not operating on a particular <webview>, then the key is 0.
   int rules_registry_id = RulesRegistryService::kDefaultRulesRegistryID;
@@ -189,6 +242,26 @@ bool EventsEventAddRulesFunction::RunAsyncOnCorrectThread() {
   return error_.empty();
 }
 
+void EventsEventAddRulesFunction::LogAPICall(
+    const std::string& event_name) const {
+  DeclarativeAPICallType call_type;
+  switch (GetDeclarativeAPITypeFromEventName(event_name)) {
+    case DeclarativeAPIType::DECLARATIVE_CONTENT:
+      call_type = DECLARATIVE_CONTENT_ADD_RULES;
+      break;
+    case DeclarativeAPIType::DECLARATIVE_WEB_REQUEST:
+      call_type = DECLARATIVE_WEB_REQUEST_ADD_RULES;
+      break;
+    case DeclarativeAPIType::DECLARATIVE_WEB_REQUEST_WEBVIEW:
+      call_type = DECLARATIVE_WEB_REQUEST_WEBVIEW_ADD_RULES;
+      break;
+    case DeclarativeAPIType::UNKNOWN:
+      NOTREACHED();
+      return;
+  }
+  LogAPICallHelper(call_type);
+}
+
 bool EventsEventRemoveRulesFunction::RunAsyncOnCorrectThread() {
   std::unique_ptr<RemoveRules::Params> params(
       RemoveRules::Params::Create(*args_));
@@ -204,6 +277,26 @@ bool EventsEventRemoveRulesFunction::RunAsyncOnCorrectThread() {
   return error_.empty();
 }
 
+void EventsEventRemoveRulesFunction::LogAPICall(
+    const std::string& event_name) const {
+  DeclarativeAPICallType call_type;
+  switch (GetDeclarativeAPITypeFromEventName(event_name)) {
+    case DeclarativeAPIType::DECLARATIVE_CONTENT:
+      call_type = DECLARATIVE_CONTENT_REMOVE_RULES;
+      break;
+    case DeclarativeAPIType::DECLARATIVE_WEB_REQUEST:
+      call_type = DECLARATIVE_WEB_REQUEST_REMOVE_RULES;
+      break;
+    case DeclarativeAPIType::DECLARATIVE_WEB_REQUEST_WEBVIEW:
+      call_type = DECLARATIVE_WEB_REQUEST_WEBVIEW_REMOVE_RULES;
+      break;
+    case DeclarativeAPIType::UNKNOWN:
+      NOTREACHED();
+      return;
+  }
+  LogAPICallHelper(call_type);
+}
+
 bool EventsEventGetRulesFunction::RunAsyncOnCorrectThread() {
   std::unique_ptr<GetRules::Params> params(GetRules::Params::Create(*args_));
   EXTENSION_FUNCTION_VALIDATE(params.get());
@@ -224,4 +317,24 @@ bool EventsEventGetRulesFunction::RunAsyncOnCorrectThread() {
   return true;
 }
 
+void EventsEventGetRulesFunction::LogAPICall(
+    const std::string& event_name) const {
+  DeclarativeAPICallType call_type;
+  switch (GetDeclarativeAPITypeFromEventName(event_name)) {
+    case DeclarativeAPIType::DECLARATIVE_CONTENT:
+      call_type = DECLARATIVE_CONTENT_GET_RULES;
+      break;
+    case DeclarativeAPIType::DECLARATIVE_WEB_REQUEST:
+      call_type = DECLARATIVE_WEB_REQUEST_GET_RULES;
+      break;
+    case DeclarativeAPIType::DECLARATIVE_WEB_REQUEST_WEBVIEW:
+      call_type = DECLARATIVE_WEB_REQUEST_WEBVIEW_GET_RULES;
+      break;
+    case DeclarativeAPIType::UNKNOWN:
+      NOTREACHED();
+      return;
+  }
+  LogAPICallHelper(call_type);
+}
+
 }  // namespace extensions