Abacn commented on code in PR #40142:
URL: https://github.com/apache/beam/pull/40142#discussion_r4066444634
##########
sdks/java/extensions/google-cloud-platform-core/src/main/java/org/apache/beam/sdk/extensions/gcp/util/Transport.java:
##########
@@ -119,6 +127,107 @@ public static Storage.Builder newStorageClient(GcsOptions
options) {
return storageBuilder;
}
+ /**
+ * Wraps an {@link HttpRequestInitializer} so that HTTP execute and response
interceptors
+ * increment {@link Counter} instances pre-bound to the given {@link
MetricsContainer}. This
+ * guarantees that GCS HTTP metrics are attributed directly to the step that
created the channel,
+ * even when requests execute on background worker threads.
+ *
+ * <p>The counters are exhaustive, so that a report can be checked for
consistency:
+ *
+ * <ul>
+ * <li>{@code request_count} counts every attempt, retries included,
because the request
+ * interceptor runs once per attempt.
+ * <li>Every attempt ends up in exactly one of {@code status_2xx}, {@code
status_3xx}, {@code
Review Comment:
The Javadoc states that `status_2xx + status_3xx + status_4xx + status_5xx +
status_other + request_no_response == request_count`. This isn't true whenever
there is retry inside HttpResponseInterceptor:
However, in google-http-client's `HttpRequest.java.execute()`,
HttpExecuteInterceptor.intercept() is called inside the do { ... } while
(retryRequest) loop (once per attempt), whereas
HttpResponseInterceptor.interceptResponse() is called outside/after the retry
loop (only once for the final response).
We can simply drop inaccurate javadocs
##########
sdks/java/extensions/google-cloud-platform-core/src/main/java/org/apache/beam/sdk/extensions/gcp/util/GcsUtilV1.java:
##########
@@ -615,11 +639,25 @@ SeekableByteChannel open(GcsPath path,
GoogleCloudStorageReadOptions readOptions
ServiceCallMetric serviceCallMetric =
new ServiceCallMetric(MonitoringInfoConstants.Urns.API_REQUEST_COUNT,
baseLabels);
try {
+ GoogleCloudStorage gcpStorage = this.googleCloudStorage;
+ MetricsContainer container = null;
+ if (gcsCountersOptions.getPerformanceMetricsEnabled()) {
+ container = MetricsEnvironment.getCurrentContainer();
+ if (container != null) {
+ HttpRequestInitializer scopedInitializer =
+ Transport.withMetricsContainer(this.httpRequestInitializer,
container, false);
+ gcpStorage =
Review Comment:
This creates an unclosed GoogleCloudStorageImpl instance on every
GcsUtilV1.open() when metrics are enabled, may have performance implications
One of the original suspected reason for OOM was gRPC cache size grew. It
wasn't the case, but with this change it could surface.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
