Skip to content

Commit

Permalink
Track details of net and http errors in metrics.
Browse files Browse the repository at this point in the history 
Bug: b/250618093
Change-Id: I303513909d69e58c19b084e42a47d4277c7da1d2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4274927
Reviewed-by: David Roger <droger@chromium.org>
Reviewed-by: Alex Ilin <alexilin@chromium.org>
Commit-Queue: Tomek Jurkiewicz <tju@google.com>
Cr-Commit-Position: refs/heads/main@{#1108876}
  • Loading branch information
Tomasz Jurkiewicz authored and Chromium LUCI CQ committed Feb 23, 2023
1 parent e839d32 commit 8e88b29
Show file tree
Hide file tree
Showing 6 changed files with 108 additions and 36 deletions.
2 changes: 1 addition & 1 deletion chrome/browser/supervised_user/child_accounts/child_account_service.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ FamilyInfoFetcher::ErrorCode ConvertStatus(KidsExternalFetcherStatus status) {
switch (status.state()) {
case KidsExternalFetcherStatus::GOOGLE_SERVICE_AUTH_ERROR:
return FamilyInfoFetcher::ErrorCode::kTokenError;
case KidsExternalFetcherStatus::HTTP_ERROR:
case KidsExternalFetcherStatus::NET_OR_HTTP_ERROR:
return FamilyInfoFetcher::ErrorCode::kNetworkError;
case KidsExternalFetcherStatus::INVALID_RESPONSE:
return FamilyInfoFetcher::ErrorCode::kServiceError;
Expand Down
77 changes: 50 additions & 27 deletions chrome/browser/supervised_user/child_accounts/family_info_fetcher.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,8 @@ const char kListFamilyMembersRequestHistogramPrefix[] =
"Signin.ListFamilyMembersRequest";
const char kListFamilyMembersRequestStatusHistogramName[] =
"Signin.ListFamilyMembersRequest.Status";
const char kListFamilyMembersRequestNetOrHttpStatusHistogramName[] =
"Signin.ListFamilyMembersRequest.NetOrHttpStatus";
const char kListFamilyMembersRequestLatencyHistogramName[] =
"Signin.ListFamilyMembersRequest.Latency";

Expand All @@ -61,7 +63,7 @@ std::string ToStatusKey(KidsExternalFetcherStatus::State status) {
return "NoError";
case KidsExternalFetcherStatus::GOOGLE_SERVICE_AUTH_ERROR:
return "AuthError";
case KidsExternalFetcherStatus::HTTP_ERROR:
case KidsExternalFetcherStatus::NET_OR_HTTP_ERROR:
return "HttpError";
case KidsExternalFetcherStatus::INVALID_RESPONSE:
return "ParseError";
Expand All @@ -76,11 +78,15 @@ std::string LatencyPerStatusKey(KidsExternalFetcherStatus::State status) {
/*separator=*/".");
}

bool IgnoreUnsupportedApiCalls(const GURL& request_url) {
// Ignore tracing all api calls except for family members.
return request_url != supervised_user::KidsManagementGetFamilyMembersURL();
}

void RecordMetrics(KidsExternalFetcherStatus::State status,
base::TimeTicks start_time,
const GURL& request_url) {
if (request_url != supervised_user::KidsManagementGetFamilyMembersURL()) {
// Ignore tracing all api calls except for family members.
if (IgnoreUnsupportedApiCalls(request_url)) {
return;
}

Expand All @@ -91,6 +97,18 @@ void RecordMetrics(KidsExternalFetcherStatus::State status,
latency);
base::UmaHistogramTimes(LatencyPerStatusKey(status), latency);
}

void RecordMetricsForNetOrHttpErrorState(int net_or_http_error,
base::TimeTicks start_time,
const GURL& request_url) {
RecordMetrics(KidsExternalFetcherStatus::State::NET_OR_HTTP_ERROR, start_time,
request_url);
if (IgnoreUnsupportedApiCalls(request_url)) {
return;
}
base::UmaHistogramSparse(
kListFamilyMembersRequestNetOrHttpStatusHistogramName, net_or_http_error);
}
} // namespace

// These correspond to enum FamilyInfoFetcher::FamilyMemberRole, in order.
Expand All @@ -101,14 +119,11 @@ FamilyInfoFetcher::FamilyProfile::FamilyProfile() = default;

FamilyInfoFetcher::FamilyProfile::FamilyProfile(const std::string& id,
const std::string& name)
: id(id), name(name) {
}
: id(id), name(name) {}

FamilyInfoFetcher::FamilyProfile::~FamilyProfile() {
}
FamilyInfoFetcher::FamilyProfile::~FamilyProfile() = default;

FamilyInfoFetcher::FamilyMember::FamilyMember() {
}
FamilyInfoFetcher::FamilyMember::FamilyMember() = default;

FamilyInfoFetcher::FamilyMember::FamilyMember(
const std::string& obfuscated_gaia_id,
Expand All @@ -122,14 +137,12 @@ FamilyInfoFetcher::FamilyMember::FamilyMember(
display_name(display_name),
email(email),
profile_url(profile_url),
profile_image_url(profile_image_url) {
}
profile_image_url(profile_image_url) {}

FamilyInfoFetcher::FamilyMember::FamilyMember(const FamilyMember& other) =
default;

FamilyInfoFetcher::FamilyMember::~FamilyMember() {
}
FamilyInfoFetcher::FamilyMember::~FamilyMember() = default;

FamilyInfoFetcher::FamilyInfoFetcher(
Consumer* consumer,
Expand Down Expand Up @@ -253,8 +266,9 @@ void FamilyInfoFetcher::OnSimpleLoaderComplete(
simple_url_loader_->ResponseInfo()->headers->response_code();
}
std::string body;
if (response_body)
if (response_body) {
body = std::move(*response_body);
}
OnSimpleLoaderCompleteInternal(simple_url_loader_->NetError(), response_code,
body);
}
Expand Down Expand Up @@ -286,16 +300,16 @@ void FamilyInfoFetcher::OnSimpleLoaderCompleteInternal(

if (response_code != net::HTTP_OK) {
DLOG(WARNING) << "HTTP error " << response_code;
RecordMetrics(KidsExternalFetcherStatus::State::HTTP_ERROR,
simple_url_loader_start_time_, request_url_);
RecordMetricsForNetOrHttpErrorState(
response_code, simple_url_loader_start_time_, request_url_);
consumer_->OnFailure(ErrorCode::kNetworkError);
return;
}

if (net_error != net::OK) {
DLOG(WARNING) << "NetError " << net_error;
RecordMetrics(KidsExternalFetcherStatus::State::HTTP_ERROR,
simple_url_loader_start_time_, request_url_);
RecordMetricsForNetOrHttpErrorState(
net_error, simple_url_loader_start_time_, request_url_);
consumer_->OnFailure(ErrorCode::kNetworkError);
return;
}
Expand Down Expand Up @@ -331,40 +345,49 @@ bool FamilyInfoFetcher::ParseMembers(const base::Value::List& list,
bool FamilyInfoFetcher::ParseMember(const base::Value::Dict& dict,
FamilyMember* member) {
const std::string* obfuscated_gaia_id = dict.FindString(kIdUserId);
if (!obfuscated_gaia_id)
if (!obfuscated_gaia_id) {
return false;
}
member->obfuscated_gaia_id = *obfuscated_gaia_id;
const std::string* role_str = dict.FindString(kIdRole);
if (!role_str)
if (!role_str) {
return false;
if (!StringToRole(*role_str, &member->role))
}
if (!StringToRole(*role_str, &member->role)) {
return false;
}
const base::Value::Dict* profile_dict = dict.FindDict(kIdProfile);
if (profile_dict)
if (profile_dict) {
ParseProfile(*profile_dict, member);
}
return true;
}

// static
void FamilyInfoFetcher::ParseProfile(const base::Value::Dict& dict,
FamilyMember* member) {
const std::string* display_name = dict.FindString(kIdDisplayName);
if (display_name)
if (display_name) {
member->display_name = *display_name;
}
const std::string* email = dict.FindString(kIdEmail);
if (email)
if (email) {
member->email = *email;
}
const std::string* profile_url = dict.FindString(kIdProfileUrl);
if (profile_url)
if (profile_url) {
member->profile_url = *profile_url;
}
const std::string* profile_image_url = dict.FindString(kIdProfileImageUrl);
if (profile_image_url)
if (profile_image_url) {
member->profile_image_url = *profile_image_url;
}
if (member->profile_image_url.empty()) {
const std::string* def_profile_image_url =
dict.FindString(kIdDefaultProfileImageUrl);
if (def_profile_image_url)
if (def_profile_image_url) {
member->profile_image_url = *def_profile_image_url;
}
}
}

Expand Down
33 changes: 28 additions & 5 deletions chrome/browser/supervised_user/kids_chrome_management/kids_external_fetcher.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ using ::base::StringPiece;
using ::base::TimeDelta;
using ::base::TimeTicks;
using ::base::UmaHistogramEnumeration;
using ::base::UmaHistogramSparse;
using ::base::UmaHistogramTimes;
using ::base::Unretained;
using ::kids_chrome_management::ListFamilyMembersRequest;
Expand All @@ -61,6 +62,14 @@ bool HasHttpOkResponse(const network::SimpleURLLoader& loader) {
net::HTTP_OK;
}

int CombineNetAndHttpErrors(const network::SimpleURLLoader& loader) {
if (loader.NetError() != net::OK || !loader.ResponseInfo() ||
!loader.ResponseInfo()->headers) {
return loader.NetError();
}
return loader.ResponseInfo()->headers->response_code();
}

std::unique_ptr<network::SimpleURLLoader> InitializeSimpleUrlLoader(
StringPiece payload,
StringPiece access_token,
Expand Down Expand Up @@ -114,7 +123,7 @@ std::string ConvertStateToMetricLabel(KidsExternalFetcherStatus::State state) {
return "NoError";
case KidsExternalFetcherStatus::GOOGLE_SERVICE_AUTH_ERROR:
return "AuthError";
case KidsExternalFetcherStatus::HTTP_ERROR:
case KidsExternalFetcherStatus::NET_OR_HTTP_ERROR:
return "HttpError";
case KidsExternalFetcherStatus::INVALID_RESPONSE:
return "ParseError";
Expand Down Expand Up @@ -209,6 +218,10 @@ class FetcherImpl final : public KidsExternalFetcher<Request, Response> {
std::unique_ptr<Response> response) {
TimeDelta latency = TimeTicks::Now() - start_time;
UmaHistogramEnumeration(CreateMetricKey<Request>("Status"), status.state());
if (status.state() == KidsExternalFetcherStatus::State::NET_OR_HTTP_ERROR) {
UmaHistogramSparse(CreateMetricKey<Request>("NetOrHttpStatus"),
status.net_or_http_error_code().value());
}
UmaHistogramTimes(CreateMetricKey<Request>("Latency"), latency);
UmaHistogramTimes(CreateMetricKey<Request>(
"Latency", ConvertStateToMetricLabel(status.state())),
Expand Down Expand Up @@ -261,7 +274,8 @@ class FetcherImpl final : public KidsExternalFetcher<Request, Response> {
std::unique_ptr<std::string> response_body) {
if (!IsLoadingSuccessful(*simple_url_loader_) ||
!HasHttpOkResponse(*simple_url_loader_)) {
std::move(callback).Run(KidsExternalFetcherStatus::HttpError(),
std::move(callback).Run(KidsExternalFetcherStatus::NetOrHttpError(
CombineNetAndHttpErrors(*simple_url_loader_)),
std::make_unique<Response>());
return;
}
Expand Down Expand Up @@ -302,6 +316,9 @@ KidsExternalFetcherStatus::KidsExternalFetcherStatus(State state)
: state_(state) {
DCHECK(state != State::GOOGLE_SERVICE_AUTH_ERROR);
}
KidsExternalFetcherStatus::KidsExternalFetcherStatus(
NetOrHttpErrorType error_code)
: state_(State::NET_OR_HTTP_ERROR), net_or_http_error_code_(error_code) {}
KidsExternalFetcherStatus::KidsExternalFetcherStatus(
class GoogleServiceAuthError google_service_auth_error)
: KidsExternalFetcherStatus(GOOGLE_SERVICE_AUTH_ERROR,
Expand All @@ -319,8 +336,9 @@ KidsExternalFetcherStatus KidsExternalFetcherStatus::GoogleServiceAuthError(
class GoogleServiceAuthError error) {
return KidsExternalFetcherStatus(error);
}
KidsExternalFetcherStatus KidsExternalFetcherStatus::HttpError() {
return KidsExternalFetcherStatus(State::HTTP_ERROR);
KidsExternalFetcherStatus KidsExternalFetcherStatus::NetOrHttpError(
int net_or_http_error_code) {
return KidsExternalFetcherStatus(NetOrHttpErrorType(net_or_http_error_code));
}
KidsExternalFetcherStatus KidsExternalFetcherStatus::InvalidResponse() {
return KidsExternalFetcherStatus(State::INVALID_RESPONSE);
Expand All @@ -333,7 +351,7 @@ bool KidsExternalFetcherStatus::IsOk() const {
return state_ == State::NO_ERROR;
}
bool KidsExternalFetcherStatus::IsTransientError() const {
if (state_ == State::HTTP_ERROR) {
if (state_ == State::NET_OR_HTTP_ERROR) {
return true;
}
if (state_ == State::GOOGLE_SERVICE_AUTH_ERROR) {
Expand All @@ -357,6 +375,11 @@ bool KidsExternalFetcherStatus::IsPersistentError() const {
KidsExternalFetcherStatus::State KidsExternalFetcherStatus::state() const {
return state_;
}
KidsExternalFetcherStatus::NetOrHttpErrorType
KidsExternalFetcherStatus::net_or_http_error_code() const {
return net_or_http_error_code_;
}

const GoogleServiceAuthError&
KidsExternalFetcherStatus::google_service_auth_error() const {
return google_service_auth_error_;
Expand Down
16 changes: 14 additions & 2 deletions chrome/browser/supervised_user/kids_chrome_management/kids_external_fetcher.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
#include "base/functional/callback_forward.h"
#include "base/memory/scoped_refptr.h"
#include "base/strings/string_piece.h"
#include "base/types/strong_alias.h"
#include "components/signin/public/identity_manager/identity_manager.h"
#include "components/supervised_user/core/browser/proto/kidschromemanagement_messages.pb.h"
#include "google_apis/gaia/google_service_auth_error.h"
Expand Down Expand Up @@ -43,19 +44,24 @@
// numeric values should never be reused.
class KidsExternalFetcherStatus {
public:
using NetOrHttpErrorType = base::StrongAlias<class NetOrHttpErrorTag, int>;

enum State {
NO_ERROR = 0, // No error.
GOOGLE_SERVICE_AUTH_ERROR = 1, // Error occurred during the access token
// fetching phase. See
// GetGoogleServiceAuthError for details.
HTTP_ERROR = 2, // The request was performed, but http returned errors.
NET_OR_HTTP_ERROR = 2, // The request was performed, but network or
// http returned errors. This is default chromium
// approach to combine those two error domains.
INVALID_RESPONSE = 3, // The request was performed without error, but http
// response could not be processed or was unexpected.
DATA_ERROR = 4, // The request was parsed, but did not contain all required
// data. Not signalled by this fetcher itself, but might be
// used by consumers to indicate data problem.
kMaxValue = DATA_ERROR, // keep last, required for metrics.
};

// Status might be used in base::expected context as possible error, since it
// contains two error-enabled attributes which are copyable / assignable.
KidsExternalFetcherStatus(const KidsExternalFetcherStatus&);
Expand All @@ -71,7 +77,9 @@ class KidsExternalFetcherStatus {
GoogleServiceAuthError
error); // The copy follows the interface of
// https://source.chromium.org/chromium/chromium/src/+/main:components/signin/public/identity_manager/primary_account_access_token_fetcher.h;l=241;drc=8ba1bad80dc22235693a0dd41fe55c0fd2dbdabd
static KidsExternalFetcherStatus HttpError();
static KidsExternalFetcherStatus NetOrHttpError(
int error_code = 0); // Either net::Error (negative numbers, 0 denotes
// success) or HTTP error (standard error codes).
static KidsExternalFetcherStatus InvalidResponse();
static KidsExternalFetcherStatus DataError();

Expand All @@ -85,11 +93,13 @@ class KidsExternalFetcherStatus {
bool IsPersistentError() const;

State state() const;
NetOrHttpErrorType net_or_http_error_code() const;
const class GoogleServiceAuthError& google_service_auth_error() const;

private:
// Disallows impossible states.
explicit KidsExternalFetcherStatus(State state);
explicit KidsExternalFetcherStatus(NetOrHttpErrorType error_code);
explicit KidsExternalFetcherStatus(
class GoogleServiceAuthError
google_service_auth_error); // Implies State ==
Expand All @@ -99,6 +109,8 @@ class KidsExternalFetcherStatus {
class GoogleServiceAuthError google_service_auth_error);

State state_;
NetOrHttpErrorType net_or_http_error_code_{
0}; // Meaningful iff state_ == NET_OR_HTTP_ERROR
class GoogleServiceAuthError google_service_auth_error_;
};

Expand Down
5 changes: 4 additions & 1 deletion chrome/browser/supervised_user/kids_chrome_management/kids_external_fetcher_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,10 @@ TEST_F(KidsExternalFetcherTest, HandlesServerError) {
net::HTTP_BAD_REQUEST);
EXPECT_FALSE(receiver.GetResult().has_value());
EXPECT_EQ(receiver.GetResult().error().state(),
KidsExternalFetcherStatus::State::HTTP_ERROR);
KidsExternalFetcherStatus::State::NET_OR_HTTP_ERROR);
EXPECT_EQ(receiver.GetResult().error().net_or_http_error_code(),
KidsExternalFetcherStatus::NetOrHttpErrorType(
net::ERR_HTTP_RESPONSE_CODE_FAILURE));
}

} // namespace
11 changes: 11 additions & 0 deletions tools/metrics/histograms/metadata/signin/histograms.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -727,6 +727,17 @@ the browser and content area. -->
<summary>Tracks the latency of calls to the Kids Management API.</summary>
</histogram>

<histogram name="Signin.ListFamilyMembersRequest.NetOrHttpStatus"
enum="CombinedHttpResponseAndNetErrorCode" expires_after="2023-11-01">
<owner>tju@google.com</owner>
<owner>chrome-kids-eng@google.com</owner>
<summary>
The status of the net or http communication to the Kids Management API. This
is recorded for each request to the Kids Management API (ListFamilyMembers
rpc).
</summary>
</histogram>

<histogram name="Signin.ListFamilyMembersRequest.Status"
enum="KidsExternalFetcherStatus" expires_after="2023-11-01">
<owner>tju@google.com</owner>
Expand Down

0 comments on commit 8e88b29

Please sign in to comment.