Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Track latency, retry and backoff time of GCS JSON API requests #1207

103 changes: 103 additions & 0 deletions util/src/main/java/com/google/cloud/hadoop/util/GcsJsonApiEvent.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
package com.google.cloud.hadoop.util;
arunkumarchacko marked this conversation as resolved.
Show resolved Hide resolved

import static com.google.common.base.Preconditions.checkArgument;

import com.google.api.client.http.HttpRequest;
import com.google.api.client.http.HttpResponse;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.MoreObjects;
import java.util.HashMap;
import java.util.Map;
import javax.annotation.Nonnegative;
import javax.annotation.Nonnull;

public class GcsJsonApiEvent {
public enum EventType {
BACKOFF,
STARTED,
RETRYSKIPPED,
EXCEPTION,
RESPONSE,
}

public static final String BACKOFFTIME = "BACKOFFTIME";
public static final String RETRYCOUNT = "RETRYCOUNT";
public static final String STATUS_CODE = "STATUS_CODE";
public static final String DURATION = "DURATION";
private final EventType eventType;

// Setting this to object so that we do not have to create the URL string.
private final Object context;

private final String method;

private Map<String, Object> properties;

static GcsJsonApiEvent getResponseEvent(HttpResponse httpResponse, @Nonnegative long duration) {
GcsJsonApiEvent result = new GcsJsonApiEvent(httpResponse.getRequest(), EventType.RESPONSE, 2);
result.set(STATUS_CODE, httpResponse.getStatusCode());
result.set(DURATION, duration);

return result;
}

static GcsJsonApiEvent getRequestStartedEvent(HttpRequest request) {
return new GcsJsonApiEvent(request, EventType.STARTED);
}

static GcsJsonApiEvent getExceptionEveent(HttpRequest httpRequest) {
singhravidutt marked this conversation as resolved.
Show resolved Hide resolved
return new GcsJsonApiEvent(httpRequest, EventType.EXCEPTION);
}

static GcsJsonApiEvent getBackoffEvent(
HttpRequest request, @Nonnegative long backOffTime, @Nonnegative int retryCount) {
return new GcsJsonApiEvent(request, EventType.BACKOFF, 2)
.set(BACKOFFTIME, backOffTime)
.set(RETRYCOUNT, retryCount);
}

@VisibleForTesting
singhravidutt marked this conversation as resolved.
Show resolved Hide resolved
GcsJsonApiEvent(@Nonnull HttpRequest request, EventType eventType) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just extract the meaningful information from HttpRequest and wrap it around RequestContext. Doing that will make it easier to extend it for gRPC as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For gRPC we can create another method or refactor this once we add it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gRPC implementation is already there are we are even moving towards GA. Ideally, we should add this support for gRPC as well. Least we can do is, any new code added should be extendable to gRPC as well, else we are just be pilling on the tech debt.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gRPC support will come in a separate change.

this.eventType = eventType;
this.context = request.getUrl();
this.method = request.getRequestMethod();
}

EventType getEventType() {
return eventType;
}

Object getContext() {
return context;
}

String getMethod() {
return method;
}

Object getProperty(String key) {
return properties == null ? null : properties.get(key);
}

private GcsJsonApiEvent(HttpRequest request, EventType eventType, int capacity) {
this(request, eventType);
this.properties = new HashMap<>(capacity);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the purpose of setting capacity? Looking into the usage of it in getBackoffEvent it seems it will immediately allocate extra capacity as well as rehash it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is done to limit the size of the map. BackOffEvent has two additional properties, hence setting the capacity to 2.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

setting this capacity will not limit the size of hashMap. It's actually not capacity but initialCapacity
https://docs.oracle.com/javase/8/docs/api/java/util/HashMap.html#HashMap-int-

There is no way you will be able to limit the size of hashMap unless ofcourse you customize and override the implementation.

Am I missing something here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you provide the capacity, it will set the capacity to the next power of 2. It will not resize. What you think it will resize?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As per the documentation When the number of entries in the hash table exceeds the product of the load factor and the current capacity, the hash table is rehashed. With Default load factor of 0.75 it will rehash as soon as there are two entries in HashMap.

}

@Override
public String toString() {
return MoreObjects.toStringHelper(this)
.add("method", method)
.add("type", eventType)
.add("properties", properties)
.add("context", context)
.toString();
}

private GcsJsonApiEvent set(String key, Object value) {
checkArgument(properties != null, "properties cannot be null");

this.properties.put(key, value);
return this;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,10 @@
/** Event Bus class */
public class GoogleCloudStorageEventBus {

public static void postGcsJsonApiEvent(GcsJsonApiEvent gcsJsonApiEvent) {
eventBus.post(gcsJsonApiEvent);
}

/** Translates increment of statistics from API calls into StatisticsType */
public enum StatisticsType {
DIRECTORIES_DELETED
Expand Down
121 changes: 121 additions & 0 deletions util/src/main/java/com/google/cloud/hadoop/util/RequestTracker.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,121 @@
/*
* Copyright 2024 Google Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package com.google.cloud.hadoop.util;

import com.google.api.client.http.HttpRequest;
import com.google.api.client.http.HttpResponse;
import com.google.common.base.MoreObjects;
import com.google.common.base.Stopwatch;
import com.google.common.flogger.GoogleLogger;
import java.util.concurrent.TimeUnit;

class RequestTracker {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, this tracker is very specific to httpRequest type and can't be extend for other request types.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class can be refactored when we add support for gRPC.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

similar comment.

private static final GoogleLogger logger = GoogleLogger.forEnclosingClass();
private static final long LOGGING_THRESHOLD = 200;
private Stopwatch stopWatch;
private Object context;
private int retryCount;
private long backOffTime;

protected RequestTracker() {}

public static RequestTracker create(HttpRequest request) {
return new RequestTracker().init(request);
}

void trackResponse(HttpResponse response) {
// The response might have been already tracked. For eg. if we get an unsuccessful response and
// it given up after the configured retries, RetryHttpRequestInitializer response interceptor
// will also get called.
if (stopWatch.isRunning()) {
postToEventQueue(GcsJsonApiEvent.getResponseEvent(response, stopWatch.elapsed().toMillis()));
stopTracking(response.getRequest());
}

if (retryCount != 0) {
// Change to minute
logger.atInfo().atMostEvery(10, TimeUnit.SECONDS).log(
"Operation completed after retries with code '%s'. %s", response.getStatusCode(), this);
singhravidutt marked this conversation as resolved.
Show resolved Hide resolved
}
}

void trackIOException(HttpRequest httpRequest) {
singhravidutt marked this conversation as resolved.
Show resolved Hide resolved
stopTracking(httpRequest);
postToEventQueue(GcsJsonApiEvent.getExceptionEveent(httpRequest));
}

void trackUnsuccessfulResponseHandler(HttpResponse response) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't trackResponse also be triggered along with trackUnsuccessfulResponseHandler for response code != 2xx? Is that meaningful to even have trackUnsuccessfulResponseHandler?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

trackResponse is triggered only if it choose not to retry. For e.g. for a sequence 429, 429, 200, trackResponse is called only once.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

trackResponse is called from response interceptor and trackUnsuccessfulResponseHandler is called from Unsuccessful response handler. I expect the responseInterceptor to be called no matter if response was unsuccessful. Weird.

stopTracking(response.getRequest());
postToEventQueue(GcsJsonApiEvent.getResponseEvent(response, stopWatch.elapsed().toMillis()));
}

void trackBackOffCompleted(long backOffStartTime, HttpRequest request) {
arunkumarchacko marked this conversation as resolved.
Show resolved Hide resolved
arunkumarchacko marked this conversation as resolved.
Show resolved Hide resolved
long diff = System.currentTimeMillis() - backOffStartTime;
postToEventQueue(GcsJsonApiEvent.getBackoffEvent(request, diff, retryCount));
backOffTime += diff;
}

void trackRetryStarted() {
stopWatch.reset();
stopWatch.start();
retryCount++;
}

void trackRetrySkipped(boolean hasResponse) {
if (!hasResponse && this.retryCount != 0) {
logger.atInfo().atMostEvery(10, TimeUnit.SECONDS).log(
"Retry skipped after %s retries. context=%s", retryCount, this);
}
}

protected void postToEventQueue(GcsJsonApiEvent event) {
GoogleCloudStorageEventBus.postGcsJsonApiEvent(event);
}

protected RequestTracker init(HttpRequest request) {
arunkumarchacko marked this conversation as resolved.
Show resolved Hide resolved
stopWatch = Stopwatch.createStarted();
context = request.getUrl();

postToEventQueue(GcsJsonApiEvent.getRequestStartedEvent(request));

return this;
}

private void stopTracking(HttpRequest httpRequest) {
if (stopWatch.isRunning()) {
stopWatch.stop();

if (stopWatch.elapsed().toMillis() > LOGGING_THRESHOLD) {
logger.atInfo().atMostEvery(10, TimeUnit.SECONDS).log(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will only log request which took longer time, what about cases where a request retried 10 times and total aggregate time was beyond the threshold limit but same is not true for individual requests?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it is retried, it is getting logged in the trackResponse

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, but it's not capturing elapsedTime correct?

singhravidutt marked this conversation as resolved.
Show resolved Hide resolved
"Detected high latency for %s. duration=%s",
httpRequest.getUrl(), stopWatch.elapsed().toMillis());
}
} else {
logger.atWarning().atMostEvery(1, TimeUnit.MINUTES).log(
singhravidutt marked this conversation as resolved.
Show resolved Hide resolved
"Can stop only an already executing request. details=%s", this);
}
}

@Override
public String toString() {
return MoreObjects.toStringHelper(this)
.add("retryCount", retryCount)
.add("totalBackoffTime", backOffTime)
.add("context", context)
.toString();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -80,14 +80,17 @@ public void initialize(HttpRequest request) throws IOException {
credentials.initialize(request);
}

RequestTracker tracker = getRequestTracker(request);
singhravidutt marked this conversation as resolved.
Show resolved Hide resolved

request
// Request will be retried if server errors (5XX) or I/O errors are encountered.
.setNumberOfRetries(options.getMaxRequestRetries())
// Set the timeout configurations.
.setConnectTimeout(toIntExact(options.getConnectTimeout().toMillis()))
.setReadTimeout(toIntExact(options.getReadTimeout().toMillis()))
.setUnsuccessfulResponseHandler(new UnsuccessfulResponseHandler(credentials))
.setIOExceptionHandler(new IoExceptionHandler());
.setUnsuccessfulResponseHandler(new UnsuccessfulResponseHandler(credentials, tracker))
.setIOExceptionHandler(new IoExceptionHandler(tracker))
.setResponseInterceptor(tracker::trackResponse);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This adds a hard dependency of trackResponse to take in only HttpResponse. I would recommend spilling out the interceptor here and use trackResponse inside of it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did not want to add one more interceptor.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's anyhow adding an interceptor, it's just that we don't have much control over it.


HttpHeaders headers = request.getHeaders();
if (isNullOrEmpty(headers.getUserAgent()) && !isNullOrEmpty(options.getDefaultUserAgent())) {
Expand All @@ -100,6 +103,10 @@ public void initialize(HttpRequest request) throws IOException {
request.setInterceptor(new InvocationIdInterceptor(request.getInterceptor()));
}

protected RequestTracker getRequestTracker(HttpRequest request) {
return RequestTracker.create(request);
}

public Credentials getCredentials() {
return credentials == null ? null : credentials.getCredentials();
}
Expand Down Expand Up @@ -151,30 +158,38 @@ private static class UnsuccessfulResponseHandler implements HttpUnsuccessfulResp

private final HttpCredentialsAdapter credentials;
private final HttpBackOffUnsuccessfulResponseHandler delegate;
private final RequestTracker tracker;

public UnsuccessfulResponseHandler(HttpCredentialsAdapter credentials) {
public UnsuccessfulResponseHandler(HttpCredentialsAdapter credentials, RequestTracker tracker) {
this.credentials = credentials;
this.delegate =
new HttpBackOffUnsuccessfulResponseHandler(BACKOFF_BUILDER.build())
.setBackOffRequired(BACK_OFF_REQUIRED);
this.tracker = tracker;
}

@Override
public boolean handleResponse(HttpRequest request, HttpResponse response, boolean supportsRetry)
throws IOException {
logResponseCode(request, response);
tracker.trackUnsuccessfulResponseHandler(response);

if (credentials != null && credentials.handleResponse(request, response, supportsRetry)) {
// If credentials decides it can handle it, the return code or message indicated something
// specific to authentication, and no backoff is desired.
return true;
}

long backOffStartTime = System.currentTimeMillis();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about using using getCurrentIntervalMillis?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is the advantage?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't have to calculate the elapsed time by ourselves and get it directly from retryStrategy.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getCurrentIntervalMillis is not the backOffTime, it is the next backoff interval.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you query it before the actual backOff being requested it will provide you the value you are looking for.

if (delegate.handleResponse(request, response, supportsRetry)) {
tracker.trackBackOffCompleted(backOffStartTime, request);
arunkumarchacko marked this conversation as resolved.
Show resolved Hide resolved
// Otherwise, we defer to the judgement of our internal backoff handler.
tracker.trackRetryStarted();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would recommend overriding HttpBackOffUnsuccessfulResponseHandler handleResponse and adding this logic there instead of polluting UnsuccessfulResponseHandler's handleResponse which has nothing to do with the retries.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you elaborate. UnsuccessfulResponseHandler derives from HttpBackOffUnsuccessfulResponseHandler.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, UnsuccessfulResponseHandler is derived from HttpUnsuccessfulResponseHandler. It has HttpBackOffUnsuccessfulResponseHandler to delagate the backoffs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok. I get what you are saying. Agree, that would have worked as well. The current implementation is fine too.

return true;
}

tracker.trackRetrySkipped(true);

escapeRedirectPath(request, response);

return false;
Expand Down Expand Up @@ -224,10 +239,12 @@ private void escapeRedirectPath(HttpRequest request, HttpResponse response) {
private static class IoExceptionHandler implements HttpIOExceptionHandler {

private final HttpIOExceptionHandler delegate;
private final RequestTracker tracker;

public IoExceptionHandler() {
public IoExceptionHandler(RequestTracker tracker) {
// Retry IOExceptions such as "socket timed out" of "insufficient bytes written" with backoff.
this.delegate = new HttpBackOffIOExceptionHandler(BACKOFF_BUILDER.build());
this.tracker = tracker;
}

@Override
Expand All @@ -236,7 +253,20 @@ public boolean handleIOException(HttpRequest httpRequest, boolean supportsRetry)
// We sadly don't get anything helpful to see if this is something we want to log.
// As a result we'll turn down the logging level to debug.
logger.atFine().log("Encountered an IOException when accessing URL %s", httpRequest.getUrl());
return delegate.handleIOException(httpRequest, supportsRetry);
tracker.trackIOException(httpRequest);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar comments as of UnsuccessfulResponseHandler handleResponse?


long backoffStartTime = System.currentTimeMillis();
boolean result = delegate.handleIOException(httpRequest, supportsRetry);

tracker.trackBackOffCompleted(backoffStartTime, httpRequest);

if (result) {
tracker.trackRetryStarted();
} else {
tracker.trackRetrySkipped(false);
}

return result;
}
}
}
Loading