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

119 changes: 119 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,119 @@
/*
* 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;
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;

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 getExceptionEvent(HttpRequest httpRequest) {
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, 1);
}

@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);
}

/** Hold the instance of the event bus here */
private static EventBus eventBus = new EventBus();

Expand Down
127 changes: 127 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,127 @@
/*
* 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;
private HttpRequest request;
private final long startTime = System.currentTimeMillis();

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();
}

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() {
stopTracking();
postToEventQueue(GcsJsonApiEvent.getExceptionEvent(request));
}

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();
postToEventQueue(GcsJsonApiEvent.getResponseEvent(response, stopWatch.elapsed().toMillis()));
}

void trackBackOffCompleted(long backOffStartTime) {
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();
this.request = request;

postToEventQueue(GcsJsonApiEvent.getRequestStartedEvent(request));

return this;
}

private void stopTracking() {
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",
request.getUrl(), stopWatch.elapsed().toMillis());
}
} else {
// Control can reach here only in case of a bug. Did not want to add an assert due to huge
// blast radius.
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)
.add("elapsed", System.currentTimeMillis() - startTime)
.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);
// 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();

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

tracker.trackBackOffCompleted(backoffStartTime);

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

return result;
}
}
}
Loading