Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2863053002:
Extensions: Add metrics to distinguish between the different kinds of declarative API function call… (Closed)
|
Created:
3 years, 7 months ago by karandeepb Modified:
3 years, 7 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, asvitkine+watch_chromium.org, extensions-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionExtensions: Add metrics to distinguish between the different kinds of declarative API function calls.
Currently the metrics recorded by the Extensions.FunctionCalls histogram for the
add/remove/get rules declarative extension functions do not distinguish between
the declarativeContent and the declarativeWebRequest APIs. This CL adds a new
histogram Extensions.DeclarativeAPICallType to distinguish between the calls to
the two APIs. Additionaly, to achieve more granular metrics, the declarative web
request API usage by webviews is considered separately.
This should help us gather data regarding how often the declarative web request
API is used and to decide a future course of action for the API once Declarative
Net Request is launched.
BUG=696822
Review-Url: https://codereview.chromium.org/2863053002
Cr-Commit-Position: refs/heads/master@{#469860}
Committed: https://chromium.googlesource.com/chromium/src/+/a641d5e814782a12dd261b234b761be85b734175
Patch Set 1 #Patch Set 2 : Rebase #
Total comments: 21
Patch Set 3 : Bot failures + nits. #Patch Set 4 : Address comments. #
Total comments: 6
Patch Set 5 : Address comments. #
Messages
Total messages: 25 (13 generated)
The CQ bit was checked by karandeepb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
karandeepb@chromium.org changed reviewers: + lazyboy@chromium.org
PTAL Istiaque.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
Seems there are some bot failures. Please hold off the review Istiaque. Thanks.
Description was changed from ========== Extensions: Add metrics to distinguish between the different kinds of declarative API function calls. Currently the metrics recorded by the Extensions.FunctionCalls histogram for the add/remove/get rules extension functions of the declarativeContent and declarativeWebRequest API do not distinguish between the two APIs, since they follow the same codepath. This CL adds a new histogram Extensions.DeclarativeAPICallType to distinguish between the calls to the two APIs. Additionaly, to achieve more granular metrics, the declarative web request API usage by webviews is considered separately. BUG=696822 ========== to ========== Extensions: Add metrics to distinguish between the different kinds of declarative API function calls. Currently the metrics recorded by the Extensions.FunctionCalls histogram for the add/remove/get rules declarative extension functions do not distinguish between the declarativeContent and the declarativeWebRequest APIs. This CL adds a new histogram Extensions.DeclarativeAPICallType to distinguish between the calls to the two APIs. Additionaly, to achieve more granular metrics, the declarative web request API usage by webviews is considered separately. This should help us gather data regarding how often the declarative web request API is used and to decide a future course of action for the API once Declarative Net Request is launched. BUG=696822 ==========
PTAL Istiaque. https://codereview.chromium.org/2863053002/diff/20001/extensions/browser/api/... File extensions/browser/api/declarative/declarative_api.cc (right): https://codereview.chromium.org/2863053002/diff/20001/extensions/browser/api/... extensions/browser/api/declarative/declarative_api.cc:51: DECLARATIVE_WEB_REQUEST_WEBVIEW, These are the 3 event name variations I could find for declarative events.
I had some comments sitting as drafts in a previous a patch set, sending them anyway. Will do another pass after everything is settled, thanks. https://codereview.chromium.org/2863053002/diff/20001/extensions/browser/api/... File extensions/browser/api/declarative/declarative_api.cc (right): https://codereview.chromium.org/2863053002/diff/20001/extensions/browser/api/... extensions/browser/api/declarative/declarative_api.cc:41: constexpr char kDeclarativeContentPrefix[] = "declarativeContent"; All of these should have "." at the end to be safe. https://codereview.chromium.org/2863053002/diff/20001/extensions/browser/api/... extensions/browser/api/declarative/declarative_api.cc:49: DECLARATIVE_CONTENT, For newly added enums, I think we started preferring kConstant style instead of macro style names. https://codereview.chromium.org/2863053002/diff/20001/extensions/browser/api/... extensions/browser/api/declarative/declarative_api.cc:52: UNKNOWN, nit: DECLARATIVE_API_TYPE_UNKNOWN, same for MAX below https://codereview.chromium.org/2863053002/diff/20001/extensions/browser/api/... extensions/browser/api/declarative/declarative_api.cc:58: enum DeclarativeAPICallType { rename to DeclarativeAPIFunctionName https://codereview.chromium.org/2863053002/diff/20001/extensions/browser/api/... extensions/browser/api/declarative/declarative_api.cc:74: base::CompareCase::INSENSITIVE_ASCII)) These should be case sensitive. https://codereview.chromium.org/2863053002/diff/20001/extensions/browser/api/... extensions/browser/api/declarative/declarative_api.cc:87: UMA_HISTOGRAM_ENUMERATION("Extensions.DeclarativeAPICallType", call_type, I'd name it Extensions.DeclarativeAPIFunctionCalls or similar, to make it similar to Extensions.FunctionCalls UMA. https://codereview.chromium.org/2863053002/diff/20001/extensions/browser/api/... File extensions/browser/api/declarative/declarative_api.h (right): https://codereview.chromium.org/2863053002/diff/20001/extensions/browser/api/... extensions/browser/api/declarative/declarative_api.h:32: virtual void LogAPICall(const std::string& event_name) const = 0; Log is too generic for this function name, name it RecordUMA or RecordHistograms
Patchset #4 (id:60001) has been deleted
PTAL Istiaque. https://codereview.chromium.org/2863053002/diff/20001/extensions/browser/api/... File extensions/browser/api/declarative/declarative_api.cc (right): https://codereview.chromium.org/2863053002/diff/20001/extensions/browser/api/... extensions/browser/api/declarative/declarative_api.cc:41: constexpr char kDeclarativeContentPrefix[] = "declarativeContent"; On 2017/05/05 21:07:42, lazyboy wrote: > All of these should have "." at the end to be safe. Done. https://codereview.chromium.org/2863053002/diff/20001/extensions/browser/api/... extensions/browser/api/declarative/declarative_api.cc:49: DECLARATIVE_CONTENT, On 2017/05/05 21:07:42, lazyboy wrote: > For newly added enums, I think we started preferring kConstant style instead of > macro style names. Correct - https://codereview.chromium.org/2820553002. Done. https://codereview.chromium.org/2863053002/diff/20001/extensions/browser/api/... extensions/browser/api/declarative/declarative_api.cc:52: UNKNOWN, On 2017/05/05 21:07:41, lazyboy wrote: > nit: DECLARATIVE_API_TYPE_UNKNOWN, same for MAX below This is an enum class. So the enum type would always be a prefix anyway. Done for MAX. https://codereview.chromium.org/2863053002/diff/20001/extensions/browser/api/... extensions/browser/api/declarative/declarative_api.cc:58: enum DeclarativeAPICallType { On 2017/05/05 21:07:42, lazyboy wrote: > rename to DeclarativeAPIFunctionName But these aren't exactly function names. Does DeclarativeAPIFunctionCallType work? https://codereview.chromium.org/2863053002/diff/20001/extensions/browser/api/... extensions/browser/api/declarative/declarative_api.cc:74: base::CompareCase::INSENSITIVE_ASCII)) On 2017/05/05 21:07:42, lazyboy wrote: > These should be case sensitive. Had resolved this. https://codereview.chromium.org/2863053002/diff/20001/extensions/browser/api/... extensions/browser/api/declarative/declarative_api.cc:87: UMA_HISTOGRAM_ENUMERATION("Extensions.DeclarativeAPICallType", call_type, On 2017/05/05 21:07:42, lazyboy wrote: > I'd name it Extensions.DeclarativeAPIFunctionCalls or similar, to make it > similar to Extensions.FunctionCalls UMA. Done. https://codereview.chromium.org/2863053002/diff/20001/extensions/browser/api/... File extensions/browser/api/declarative/declarative_api.h (right): https://codereview.chromium.org/2863053002/diff/20001/extensions/browser/api/... extensions/browser/api/declarative/declarative_api.h:32: virtual void LogAPICall(const std::string& event_name) const = 0; On 2017/05/05 21:07:42, lazyboy wrote: > Log is too generic for this function name, name it RecordUMA or RecordHistograms Done.
declarative_api.* LGTM with nits. I'd be interested to know isherman's thoughts about https://codereview.chromium.org/2863053002/diff/80001/extensions/browser/api/... https://codereview.chromium.org/2863053002/diff/20001/extensions/browser/api/... File extensions/browser/api/declarative/declarative_api.cc (right): https://codereview.chromium.org/2863053002/diff/20001/extensions/browser/api/... extensions/browser/api/declarative/declarative_api.cc:52: UNKNOWN, On 2017/05/05 22:33:42, karandeepb wrote: > On 2017/05/05 21:07:41, lazyboy wrote: > > nit: DECLARATIVE_API_TYPE_UNKNOWN, same for MAX below > > This is an enum class. So the enum type would always be a prefix anyway. Done > for MAX. Ah right. Maybe we can also drop "Declarative" substrs from the other ones then? https://codereview.chromium.org/2863053002/diff/20001/extensions/browser/api/... extensions/browser/api/declarative/declarative_api.cc:56: // These values are written to logs. New enum values can be added, but existing nit: These values are recorded as UMA. https://codereview.chromium.org/2863053002/diff/20001/extensions/browser/api/... extensions/browser/api/declarative/declarative_api.cc:58: enum DeclarativeAPICallType { On 2017/05/05 22:33:42, karandeepb wrote: > On 2017/05/05 21:07:42, lazyboy wrote: > > rename to DeclarativeAPIFunctionName > > But these aren't exactly function names. Does DeclarativeAPIFunctionCallType > work? 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? https://codereview.chromium.org/2863053002/diff/80001/extensions/browser/api/... File extensions/browser/api/declarative/declarative_api.cc (right): https://codereview.chromium.org/2863053002/diff/80001/extensions/browser/api/... extensions/browser/api/declarative/declarative_api.cc:71: DeclarativeAPIType GetDeclarativeAPITypeFromEventName( nit: FromEventName is redundant since it should be obvious from the |event_name| parameter. https://codereview.chromium.org/2863053002/diff/80001/extensions/browser/api/... extensions/browser/api/declarative/declarative_api.cc:179: RecordUMA(event_name); Since this requires us to search for 3 prefixes in GetDeclarativeAPITypeFromEventName... I wonder if we can somehow avoid generating histogram keys when we don't need them, i.e. when UMA is not enabled. isherman@ might be able to answer that.
Patchset #5 (id:100001) has been deleted
karandeepb@chromium.org changed reviewers: + isherman@chromium.org
Thanks Istiaque. +isherman@ for metrics review. Also please see Istiaque's previous comment. And is there a way to know if UMA logging is enabled so as to avoid computation done specifically for UMA? Thanks. https://codereview.chromium.org/2863053002/diff/20001/extensions/browser/api/... File extensions/browser/api/declarative/declarative_api.cc (right): https://codereview.chromium.org/2863053002/diff/20001/extensions/browser/api/... extensions/browser/api/declarative/declarative_api.cc:52: UNKNOWN, On 2017/05/05 23:11:16, lazyboy wrote: > On 2017/05/05 22:33:42, karandeepb wrote: > > On 2017/05/05 21:07:41, lazyboy wrote: > > > nit: DECLARATIVE_API_TYPE_UNKNOWN, same for MAX below > > > > This is an enum class. So the enum type would always be a prefix anyway. Done > > for MAX. > > Ah right. Maybe we can also drop "Declarative" substrs from the other ones then? Done. https://codereview.chromium.org/2863053002/diff/20001/extensions/browser/api/... extensions/browser/api/declarative/declarative_api.cc:56: // These values are written to logs. New enum values can be added, but existing On 2017/05/05 23:11:16, lazyboy wrote: > nit: These values are recorded as UMA. Done. https://codereview.chromium.org/2863053002/diff/20001/extensions/browser/api/... extensions/browser/api/declarative/declarative_api.cc:58: enum DeclarativeAPICallType { On 2017/05/05 23:11:16, lazyboy wrote: > On 2017/05/05 22:33:42, karandeepb wrote: > > On 2017/05/05 21:07:42, lazyboy wrote: > > > rename to DeclarativeAPIFunctionName > > > > But these aren't exactly function names. Does DeclarativeAPIFunctionCallType > > work? > > 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? Yeah. Changed to DeclarativeAPIFunctionType https://codereview.chromium.org/2863053002/diff/80001/extensions/browser/api/... File extensions/browser/api/declarative/declarative_api.cc (right): https://codereview.chromium.org/2863053002/diff/80001/extensions/browser/api/... extensions/browser/api/declarative/declarative_api.cc:71: DeclarativeAPIType GetDeclarativeAPITypeFromEventName( On 2017/05/05 23:11:16, lazyboy wrote: > nit: FromEventName is redundant since it should be obvious from the |event_name| > parameter. Done. https://codereview.chromium.org/2863053002/diff/80001/extensions/browser/api/... extensions/browser/api/declarative/declarative_api.cc:179: RecordUMA(event_name); On 2017/05/05 23:11:16, lazyboy wrote: > Since this requires us to search for 3 prefixes in > GetDeclarativeAPITypeFromEventName... > I wonder if we can somehow avoid generating histogram keys when we don't need > them, i.e. when UMA is not enabled. isherman@ might be able to answer that. Acknowledged.
Metrics LGTM https://codereview.chromium.org/2863053002/diff/80001/extensions/browser/api/... File extensions/browser/api/declarative/declarative_api.cc (right): https://codereview.chromium.org/2863053002/diff/80001/extensions/browser/api/... extensions/browser/api/declarative/declarative_api.cc:179: RecordUMA(event_name); On 2017/05/05 23:11:16, lazyboy wrote: > Since this requires us to search for 3 prefixes in > GetDeclarativeAPITypeFromEventName... > I wonder if we can somehow avoid generating histogram keys when we don't need > them, i.e. when UMA is not enabled. isherman@ might be able to answer that. It's possible, but you should not do this. We do not want to create performance incentives for users to opt out of UMA -- we want that decision to be made purely based on users' privacy preferences. So, please make sure that your metrics collection code is sufficiently performant that you'd be perfectly happy to run it for all Chrome users =)
On 2017/05/05 23:45:15, Ilya Sherman wrote: > Metrics LGTM > > https://codereview.chromium.org/2863053002/diff/80001/extensions/browser/api/... > File extensions/browser/api/declarative/declarative_api.cc (right): > > https://codereview.chromium.org/2863053002/diff/80001/extensions/browser/api/... > extensions/browser/api/declarative/declarative_api.cc:179: > RecordUMA(event_name); > On 2017/05/05 23:11:16, lazyboy wrote: > > Since this requires us to search for 3 prefixes in > > GetDeclarativeAPITypeFromEventName... > > I wonder if we can somehow avoid generating histogram keys when we don't need > > them, i.e. when UMA is not enabled. isherman@ might be able to answer that. > > It's possible, but you should not do this. We do not want to create performance > incentives for users to opt out of UMA -- we want that decision to be made > purely based on users' privacy preferences. So, please make sure that your > metrics collection code is sufficiently performant that you'd be perfectly happy > to run it for all Chrome users =) Makes sense, thanks.
The CQ bit was checked by karandeepb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lazyboy@chromium.org Link to the patchset: https://codereview.chromium.org/2863053002/#ps120001 (title: "Address comments.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
still lgtm. https://codereview.chromium.org/2863053002/diff/80001/extensions/browser/api/... File extensions/browser/api/declarative/declarative_api.cc (right): https://codereview.chromium.org/2863053002/diff/80001/extensions/browser/api/... extensions/browser/api/declarative/declarative_api.cc:179: RecordUMA(event_name); On 2017/05/05 23:45:15, Ilya Sherman wrote: > On 2017/05/05 23:11:16, lazyboy wrote: > > Since this requires us to search for 3 prefixes in > > GetDeclarativeAPITypeFromEventName... > > I wonder if we can somehow avoid generating histogram keys when we don't need > > them, i.e. when UMA is not enabled. isherman@ might be able to answer that. > > It's possible, but you should not do this. We do not want to create performance > incentives for users to opt out of UMA -- we want that decision to be made > purely based on users' privacy preferences. So, please make sure that your > metrics collection code is sufficiently performant that you'd be perfectly happy > to run it for all Chrome users =) Good to know that! Thanks. The code here isn't a big deal, I was just curious.
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1494028295933290,
"parent_rev": "a45c8831be6492b82adf6f719c9a8dd72d140693", "commit_rev":
"a641d5e814782a12dd261b234b761be85b734175"}
Message was sent while issue was closed.
Description was changed from ========== Extensions: Add metrics to distinguish between the different kinds of declarative API function calls. Currently the metrics recorded by the Extensions.FunctionCalls histogram for the add/remove/get rules declarative extension functions do not distinguish between the declarativeContent and the declarativeWebRequest APIs. This CL adds a new histogram Extensions.DeclarativeAPICallType to distinguish between the calls to the two APIs. Additionaly, to achieve more granular metrics, the declarative web request API usage by webviews is considered separately. This should help us gather data regarding how often the declarative web request API is used and to decide a future course of action for the API once Declarative Net Request is launched. BUG=696822 ========== to ========== Extensions: Add metrics to distinguish between the different kinds of declarative API function calls. Currently the metrics recorded by the Extensions.FunctionCalls histogram for the add/remove/get rules declarative extension functions do not distinguish between the declarativeContent and the declarativeWebRequest APIs. This CL adds a new histogram Extensions.DeclarativeAPICallType to distinguish between the calls to the two APIs. Additionaly, to achieve more granular metrics, the declarative web request API usage by webviews is considered separately. This should help us gather data regarding how often the declarative web request API is used and to decide a future course of action for the API once Declarative Net Request is launched. BUG=696822 Review-Url: https://codereview.chromium.org/2863053002 Cr-Commit-Position: refs/heads/master@{#469860} Committed: https://chromium.googlesource.com/chromium/src/+/a641d5e814782a12dd261b234b76... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:120001) as https://chromium.googlesource.com/chromium/src/+/a641d5e814782a12dd261b234b76... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
