Skip to content

Commit

Permalink
[sync] Add histograms for addTrustedSyncEncryptionRecoveryMethod() calls
Browse files Browse the repository at this point in the history
The goal is to understand the behavior around Javascript API
chrome.addTrustedSyncEncryptionRecoveryMethod(): the proportion of
invocations that fail and why.

Because the logic is plumbed through multiple layers, separate metrics
are introduced for the most relevant pieces.

Some of the metrics are instrumented for Android only, which is the
platform that requires most investigation. Instrumenting these metrics
for other platforms requires more intrusive changes and would anyway
lead to buckets with different semantics.

(cherry picked from commit ac44b3a)

Change-Id: Ia524e1b6b06a3e8887cd9bd31b5960b01306af7d
Fixed: 1403195
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4120275
Commit-Queue: Rushan Suleymanov <rushans@google.com>
Reviewed-by: Maksim Moskvitin <mmoskvitin@google.com>
Auto-Submit: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Rushan Suleymanov <rushans@google.com>
Reviewed-by: Alex Ilin <alexilin@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#1087976}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4131728
Reviewed-by: David Roger <droger@chromium.org>
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/branch-heads/5481@{#115}
Cr-Branched-From: 130f3e4-refs/heads/main@{#1084008}
  • Loading branch information
Mikel Astiz authored and Chromium LUCI CQ committed Jan 4, 2023
1 parent 1bfee0c commit 1485e5b
Show file tree
Hide file tree
Showing 4 changed files with 62 additions and 1 deletion.
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
import org.chromium.base.Promise;
import org.chromium.base.annotations.CalledByNative;
import org.chromium.base.annotations.NativeMethods;
import org.chromium.base.metrics.RecordHistogram;
import org.chromium.chrome.browser.AppHooks;
import org.chromium.components.signin.base.CoreAccountInfo;
import org.chromium.components.sync.TrustedVaultUserActionTriggerForUMA;
Expand Down Expand Up @@ -349,11 +350,13 @@ private static void addTrustedRecoveryMethod(long nativeTrustedVaultClientAndroi
int requestId, CoreAccountInfo accountInfo, byte[] publicKey, int methodTypeHint) {
assert isNativeRegistered(nativeTrustedVaultClientAndroid);

Consumer<Void> responseCallback = unused -> {
Consumer<Void> responseCallback = completion -> {
if (!isNativeRegistered(nativeTrustedVaultClientAndroid)) {
// Native already unregistered, no response needed.
return;
}
RecordHistogram.recordBooleanHistogram(
"Sync.TrustedVaultJavascriptAddRecoveryMethodSucceeded", completion != null);
TrustedVaultClientJni.get().addTrustedRecoveryMethodCompleted(
nativeTrustedVaultClientAndroid, requestId);
};
Expand Down
6 changes: 6 additions & 0 deletions chrome/browser/sync/trusted_vault_client_android.cc
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

#include "base/android/jni_android.h"
#include "base/check_op.h"
#include "base/metrics/histogram_functions.h"
#include "base/notreached.h"
#include "chrome/android/chrome_jni_headers/TrustedVaultClient_jni.h"
#include "components/sync/driver/sync_service_utils.h"
Expand Down Expand Up @@ -224,6 +225,11 @@ void TrustedVaultClientAndroid::AddTrustedRecoveryMethod(

const CoreAccountInfo account_info =
gaia_account_info_by_gaia_id_cb_.Run(gaia_id);

base::UmaHistogramBoolean(
"Sync.TrustedVaultJavascriptAddRecoveryMethodUserKnown",
account_info != CoreAccountInfo());

if (account_info == CoreAccountInfo()) {
std::move(cb).Run();
return;
Expand Down
15 changes: 15 additions & 0 deletions chrome/renderer/sync_encryption_keys_extension.cc
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
#include "base/bind.h"
#include "base/feature_list.h"
#include "base/logging.h"
#include "base/metrics/histogram_functions.h"
#include "build/buildflag.h"
#include "components/sync/base/features.h"
#include "content/public/common/isolated_world_ids.h"
Expand Down Expand Up @@ -69,6 +70,11 @@ std::vector<std::vector<uint8_t>> EncryptionKeysAsBytes(
return encryption_keys_as_bytes;
}

void RecordCallToAddTrustedSyncEncryptionRecoveryMethodToUma(bool valid_args) {
base::UmaHistogramBoolean(
"Sync.TrustedVaultJavascriptAddRecoveryMethodValidArgs", valid_args);
}

} // namespace

// static
Expand Down Expand Up @@ -236,27 +242,35 @@ void SyncEncryptionKeysExtension::AddTrustedSyncEncryptionRecoveryMethod(

v8::Local<v8::Function> callback;
if (!args->GetNext(&callback)) {
RecordCallToAddTrustedSyncEncryptionRecoveryMethodToUma(
/*valid_args=*/false);
DLOG(ERROR) << "No callback";
args->ThrowError();
return;
}

std::string gaia_id;
if (!args->GetNext(&gaia_id)) {
RecordCallToAddTrustedSyncEncryptionRecoveryMethodToUma(
/*valid_args=*/false);
DLOG(ERROR) << "No account ID";
args->ThrowError();
return;
}

v8::Local<v8::ArrayBuffer> public_key;
if (!args->GetNext(&public_key)) {
RecordCallToAddTrustedSyncEncryptionRecoveryMethodToUma(
/*valid_args=*/false);
DLOG(ERROR) << "No public key";
args->ThrowError();
return;
}

int method_type_hint = 0;
if (!args->GetNext(&method_type_hint)) {
RecordCallToAddTrustedSyncEncryptionRecoveryMethodToUma(
/*valid_args=*/false);
DLOG(ERROR) << "No method type hint";
args->ThrowError();
return;
Expand All @@ -269,6 +283,7 @@ void SyncEncryptionKeysExtension::AddTrustedSyncEncryptionRecoveryMethod(
render_frame()->GetRemoteAssociatedInterfaces()->GetInterface(&remote_);
}

RecordCallToAddTrustedSyncEncryptionRecoveryMethodToUma(/*valid_args=*/true);
remote_->AddTrustedRecoveryMethod(
gaia_id, ArrayBufferAsBytes(public_key), method_type_hint,
base::BindOnce(&SyncEncryptionKeysExtension::RunCompletionCallback,
Expand Down
37 changes: 37 additions & 0 deletions tools/metrics/histograms/metadata/sync/histograms.xml
Original file line number Diff line number Diff line change
Expand Up @@ -1488,6 +1488,43 @@ chromium-metrics-reviews@google.com.
</summary>
</histogram>

<histogram name="Sync.TrustedVaultJavascriptAddRecoveryMethodSucceeded"
enum="BooleanSuccess" expires_after="2023-06-18">
<owner>mmoskvitin@google.com</owner>
<owner>mastiz@chromium.org</owner>
<component>Services&gt;Sync</component>
<summary>
Records whether invocations to the Javascript API
chrome.addTrustedSyncEncryptionRecoveryMethod() completed successfully.
Instrumented on Android only.
</summary>
</histogram>

<histogram name="Sync.TrustedVaultJavascriptAddRecoveryMethodUserKnown"
enum="BooleanKnown" expires_after="2023-06-18">
<owner>mmoskvitin@google.com</owner>
<owner>mastiz@chromium.org</owner>
<component>Services&gt;Sync</component>
<summary>
Records whether invocations to the Javascript API
chrome.addTrustedSyncEncryptionRecoveryMethod() specify a user ID that is
known by IdentityManager. Instrumented on Android only.
</summary>
</histogram>

<histogram name="Sync.TrustedVaultJavascriptAddRecoveryMethodValidArgs"
enum="BooleanValid" expires_after="2023-06-18">
<owner>mmoskvitin@google.com</owner>
<owner>mastiz@chromium.org</owner>
<component>Services&gt;Sync</component>
<summary>
Records invocations to the Javascript API
chrome.addTrustedSyncEncryptionRecoveryMethod() and whether the passed
arguments could be successfully parsed (which doesn't imply the function
actually succeeded).
</summary>
</histogram>

<histogram name="Sync.TrustedVaultKeyRetrievalTrigger"
enum="TrustedVaultUserActionTrigger" expires_after="2023-06-11">
<owner>mmoskvitin@google.com</owner>
Expand Down

0 comments on commit 1485e5b

Please sign in to comment.