Skip to content

Commit

Permalink
Always report metrics back to CronetURLRequest adapters
Browse files Browse the repository at this point in the history
Right now we report metrics only when the adapter has an active
listener. Up until now, this made sense since the listener was the only
consumer of these metrics.

With the introduction of the logging API this is no longer the case:
logger are also interested in these metrics and in the current design
there will always be a logger.

Bug: b:226554121

Change-Id: I136fc6d23b5ebbf61478aa06ce8fcba30b8e2db0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3660507
Reviewed-by: Richard Coles <torne@chromium.org>
Commit-Queue: Stefano Duo <stefanoduo@google.com>
Cr-Commit-Position: refs/heads/main@{#1009519}
  • Loading branch information
StefanoDuo authored and Chromium LUCI CQ committed Jun 1, 2022
1 parent 9f6160b commit c43e4cd
Show file tree
Hide file tree
Showing 9 changed files with 13 additions and 41 deletions.
12 changes: 3 additions & 9 deletions components/cronet/android/cronet_bidirectional_stream_adapter.cc
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,6 @@ static jlong JNI_CronetBidirectionalStream_CreateBidirectionalStream(
const base::android::JavaParamRef<jobject>& jbidi_stream,
jlong jurl_request_context_adapter,
jboolean jsend_request_headers_automatically,
jboolean jenable_metrics,
jboolean jtraffic_stats_tag_set,
jint jtraffic_stats_tag,
jboolean jtraffic_stats_uid_set,
Expand All @@ -92,9 +91,9 @@ static jlong JNI_CronetBidirectionalStream_CreateBidirectionalStream(
CronetBidirectionalStreamAdapter* adapter =
new CronetBidirectionalStreamAdapter(
context_adapter, env, jbidi_stream,
jsend_request_headers_automatically, jenable_metrics,
jtraffic_stats_tag_set, jtraffic_stats_tag, jtraffic_stats_uid_set,
jtraffic_stats_uid, jnetwork_handle);
jsend_request_headers_automatically, jtraffic_stats_tag_set,
jtraffic_stats_tag, jtraffic_stats_uid_set, jtraffic_stats_uid,
jnetwork_handle);

return reinterpret_cast<jlong>(adapter);
}
Expand All @@ -104,7 +103,6 @@ CronetBidirectionalStreamAdapter::CronetBidirectionalStreamAdapter(
JNIEnv* env,
const base::android::JavaParamRef<jobject>& jbidi_stream,
bool send_request_headers_automatically,
bool enable_metrics,
bool traffic_stats_tag_set,
int32_t traffic_stats_tag,
bool traffic_stats_uid_set,
Expand All @@ -113,7 +111,6 @@ CronetBidirectionalStreamAdapter::CronetBidirectionalStreamAdapter(
: context_(context),
owner_(env, jbidi_stream),
send_request_headers_automatically_(send_request_headers_automatically),
enable_metrics_(enable_metrics),
traffic_stats_tag_set_(traffic_stats_tag_set),
traffic_stats_tag_(traffic_stats_tag),
traffic_stats_uid_set_(traffic_stats_uid_set),
Expand Down Expand Up @@ -474,9 +471,6 @@ CronetBidirectionalStreamAdapter::GetHeadersArray(
}

void CronetBidirectionalStreamAdapter::MaybeReportMetrics() {
if (!enable_metrics_)
return;

if (!bidi_stream_)
return;
net::LoadTimingInfo load_timing_info;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,6 @@ class CronetBidirectionalStreamAdapter
JNIEnv* env,
const base::android::JavaParamRef<jobject>& jbidi_stream,
bool jsend_request_headers_automatically,
bool enable_metrics,
bool traffic_stats_tag_set,
int32_t traffic_stats_tag,
bool traffic_stats_uid_set,
Expand Down Expand Up @@ -174,8 +173,6 @@ class CronetBidirectionalStreamAdapter
// Java object that owns this CronetBidirectionalStreamAdapter.
base::android::ScopedJavaGlobalRef<jobject> owner_;
const bool send_request_headers_automatically_;
// Whether metrics collection is enabled when |this| is created.
const bool enable_metrics_;
// Whether |traffic_stats_tag_| should be applied.
const bool traffic_stats_tag_set_;
// TrafficStats tag to apply to URLRequest.
Expand Down
7 changes: 2 additions & 5 deletions components/cronet/android/cronet_url_request_adapter.cc
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,6 @@ static jlong JNI_CronetUrlRequest_CreateRequestAdapter(
jint jpriority,
jboolean jdisable_cache,
jboolean jdisable_connection_migration,
jboolean jenable_metrics,
jboolean jtraffic_stats_tag_set,
jint jtraffic_stats_tag,
jboolean jtraffic_stats_uid_set,
Expand All @@ -85,8 +84,8 @@ static jlong JNI_CronetUrlRequest_CreateRequestAdapter(
CronetURLRequestAdapter* adapter = new CronetURLRequestAdapter(
context_adapter, env, jurl_request, url,
static_cast<net::RequestPriority>(jpriority), jdisable_cache,
jdisable_connection_migration, jenable_metrics, jtraffic_stats_tag_set,
jtraffic_stats_tag, jtraffic_stats_uid_set, jtraffic_stats_uid,
jdisable_connection_migration, jtraffic_stats_tag_set, jtraffic_stats_tag,
jtraffic_stats_uid_set, jtraffic_stats_uid,
static_cast<net::Idempotency>(jidempotency), jnetwork_handle);

return reinterpret_cast<jlong>(adapter);
Expand All @@ -100,7 +99,6 @@ CronetURLRequestAdapter::CronetURLRequestAdapter(
net::RequestPriority priority,
jboolean jdisable_cache,
jboolean jdisable_connection_migration,
jboolean jenable_metrics,
jboolean jtraffic_stats_tag_set,
jint jtraffic_stats_tag,
jboolean jtraffic_stats_uid_set,
Expand All @@ -114,7 +112,6 @@ CronetURLRequestAdapter::CronetURLRequestAdapter(
priority,
jdisable_cache == JNI_TRUE,
jdisable_connection_migration == JNI_TRUE,
jenable_metrics == JNI_TRUE,
jtraffic_stats_tag_set == JNI_TRUE,
jtraffic_stats_tag,
jtraffic_stats_uid_set == JNI_TRUE,
Expand Down
1 change: 0 additions & 1 deletion components/cronet/android/cronet_url_request_adapter.h
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,6 @@ class CronetURLRequestAdapter : public CronetURLRequest::Callback {
net::RequestPriority priority,
jboolean jdisable_cache,
jboolean jdisable_connection_migration,
jboolean jenable_metrics,
jboolean jtraffic_stats_tag_set,
jint jtraffic_stats_tag,
jboolean jtraffic_stats_uid_set,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -280,9 +280,8 @@ public void start() {
mNativeStream = CronetBidirectionalStreamJni.get().createBidirectionalStream(
CronetBidirectionalStream.this,
mRequestContext.getUrlRequestContextAdapter(),
!mDelayRequestHeadersUntilFirstFlush,
mRequestContext.hasRequestFinishedListener(), mTrafficStatsTagSet,
mTrafficStatsTag, mTrafficStatsUidSet, mTrafficStatsUid, mNetworkHandle);
!mDelayRequestHeadersUntilFirstFlush, mTrafficStatsTagSet, mTrafficStatsTag,
mTrafficStatsUidSet, mTrafficStatsUid, mNetworkHandle);
mRequestContext.onRequestStarted();
// Non-zero startResult means an argument error.
int startResult = CronetBidirectionalStreamJni.get().start(mNativeStream,
Expand Down Expand Up @@ -837,8 +836,8 @@ interface Natives {
// Native methods are implemented in cronet_bidirectional_stream_adapter.cc.
long createBidirectionalStream(CronetBidirectionalStream caller,
long urlRequestContextAdapter, boolean sendRequestHeadersAutomatically,
boolean enableMetricsCollection, boolean trafficStatsTagSet, int trafficStatsTag,
boolean trafficStatsUidSet, int trafficStatsUid, long networkHandle);
boolean trafficStatsTagSet, int trafficStatsTag, boolean trafficStatsUidSet,
int trafficStatsUid, long networkHandle);

@NativeClassQualifiedName("CronetBidirectionalStreamAdapter")
int start(long nativePtr, CronetBidirectionalStream caller, String url, int priority,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -225,8 +225,6 @@ public void start() {
mUrlRequestAdapter = CronetUrlRequestJni.get().createRequestAdapter(
CronetUrlRequest.this, mRequestContext.getUrlRequestContextAdapter(),
mInitialUrl, mPriority, mDisableCache, mDisableConnectionMigration,
mRequestContext.hasRequestFinishedListener()
|| mRequestFinishedListener != null,
mTrafficStatsTagSet, mTrafficStatsTag, mTrafficStatsUidSet,
mTrafficStatsUid, mIdempotency, mNetworkHandle);
mRequestContext.onRequestStarted();
Expand Down Expand Up @@ -859,9 +857,8 @@ public void run() {
interface Natives {
long createRequestAdapter(CronetUrlRequest caller, long urlRequestContextAdapter,
String url, int priority, boolean disableCache, boolean disableConnectionMigration,
boolean enableMetrics, boolean trafficStatsTagSet, int trafficStatsTag,
boolean trafficStatsUidSet, int trafficStatsUid, int idempotency,
long networkHandle);
boolean trafficStatsTagSet, int trafficStatsTag, boolean trafficStatsUidSet,
int trafficStatsUid, int idempotency, long networkHandle);

@NativeClassQualifiedName("CronetURLRequestAdapter")
boolean setHttpMethod(long nativePtr, CronetUrlRequest caller, String method);
Expand Down
6 changes: 1 addition & 5 deletions components/cronet/cronet_url_request.cc
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,6 @@ CronetURLRequest::CronetURLRequest(
net::RequestPriority priority,
bool disable_cache,
bool disable_connection_migration,
bool enable_metrics,
bool traffic_stats_tag_set,
int32_t traffic_stats_tag,
bool traffic_stats_uid_set,
Expand All @@ -77,7 +76,6 @@ CronetURLRequest::CronetURLRequest(
CalculateLoadFlags(context->default_load_flags(),
disable_cache,
disable_connection_migration),
enable_metrics,
traffic_stats_tag_set,
traffic_stats_tag,
traffic_stats_uid_set,
Expand Down Expand Up @@ -180,7 +178,6 @@ CronetURLRequest::NetworkTasks::NetworkTasks(
const GURL& url,
net::RequestPriority priority,
int load_flags,
bool enable_metrics,
bool traffic_stats_tag_set,
int32_t traffic_stats_tag,
bool traffic_stats_uid_set,
Expand All @@ -193,7 +190,6 @@ CronetURLRequest::NetworkTasks::NetworkTasks(
initial_load_flags_(load_flags),
received_byte_count_from_redirects_(0l),
error_reported_(false),
enable_metrics_(enable_metrics),
metrics_reported_(false),
traffic_stats_tag_set_(traffic_stats_tag_set),
traffic_stats_tag_(traffic_stats_tag),
Expand Down Expand Up @@ -399,7 +395,7 @@ void CronetURLRequest::NetworkTasks::MaybeReportMetrics() {
// be a native URLRequest. In this case, the caller gets the exception
// immediately, and the onFailed callback isn't called, so don't report
// metrics either.
if (!enable_metrics_ || metrics_reported_ || !url_request_) {
if (metrics_reported_ || !url_request_) {
return;
}
metrics_reported_ = true;
Expand Down
7 changes: 1 addition & 6 deletions components/cronet/cronet_url_request.h
Original file line number Diff line number Diff line change
Expand Up @@ -110,8 +110,7 @@ class CronetURLRequest {
// methods will be invoked.
virtual void OnDestroyed() = 0;

// Invoked right before request is destroyed to report collected metrics if
// |enable_metrics| is true in CronetURLRequest::CronetURLRequest().
// Invoked right before request is destroyed to report collected metrics.
virtual void OnMetricsCollected(const base::Time& request_start_time,
const base::TimeTicks& request_start,
const base::TimeTicks& dns_start,
Expand Down Expand Up @@ -146,7 +145,6 @@ class CronetURLRequest {
net::RequestPriority priority,
bool disable_cache,
bool disable_connection_migration,
bool enable_metrics,
bool traffic_stats_tag_set,
int32_t traffic_stats_tag,
bool traffic_stats_uid_set,
Expand Down Expand Up @@ -210,7 +208,6 @@ class CronetURLRequest {
const GURL& url,
net::RequestPriority priority,
int load_flags,
bool enable_metrics,
bool traffic_stats_tag_set,
int32_t traffic_stats_tag,
bool traffic_stats_uid_set,
Expand Down Expand Up @@ -284,8 +281,6 @@ class CronetURLRequest {
// OnSSLCertificateError().
bool error_reported_;

// Whether detailed metrics should be collected and reported.
const bool enable_metrics_;
// Whether metrics have been reported.
bool metrics_reported_;

Expand Down
2 changes: 0 additions & 2 deletions components/cronet/native/url_request.cc
Original file line number Diff line number Diff line change
Expand Up @@ -378,8 +378,6 @@ Cronet_RESULT Cronet_UrlRequestImpl::InitWithParams(
engine_->cronet_url_request_context(), std::move(network_tasks),
GURL(url), ConvertRequestPriority(params->priority),
params->disable_cache, true /* params->disableConnectionMigration */,
request_finished_listener_ != nullptr ||
engine_->HasRequestFinishedListener() /* params->enableMetrics */,
// TODO(pauljensen): Consider exposing TrafficStats API via C++ API.
false /* traffic_stats_tag_set */, 0 /* traffic_stats_tag */,
false /* traffic_stats_uid_set */, 0 /* traffic_stats_uid */,
Expand Down

0 comments on commit c43e4cd

Please sign in to comment.