Skip to content

Commit

Permalink
#26373: Making CircuitBreakerUrl more informative about the errors w…
Browse files Browse the repository at this point in the history 
…hen consuming an http endpoint
  • Loading branch information
@victoralfaro-dotcms
victoralfaro-dotcms committed Oct 5, 2023
1 parent 3f81f03 commit d244da2
Show file tree
Hide file tree
Showing 8 changed files with 170 additions and 96 deletions.
35 changes: 29 additions & 6 deletions dotCMS/src/integration-test/java/com/dotcms/http/CircuitBreakerUrlTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
import com.dotcms.rest.exception.BadRequestException;
import com.dotcms.util.network.IPUtils;
import com.dotmarketing.exception.DotRuntimeException;
import com.dotmarketing.util.Config;
import java.io.IOException;
import java.io.OutputStream;
import java.util.ArrayList;
Expand All @@ -32,11 +31,8 @@

public class CircuitBreakerUrlTest {



final static String goodUrl = "https://www.dotcms.com";



// this will redirect to https
final static String redirectUrl = "http://www.dotcms.com";
final static String badUrl = "https://localhost:9999/test";
Expand All @@ -47,7 +43,7 @@ public class CircuitBreakerUrlTest {
final static String PARAM_VALUE="PARAM SEEMS TO BE WORKING";

@Test()
public void test_circuitBreakerConnectionControl() throws Exception {
public void test_circuitBreakerConnectionControl() {

final DotSubmitter dotSubmitter = DotConcurrentFactory.getInstance().getSubmitter();
final CircuitBreakerUrl.CircuitBreakerConnectionControl circuitBreakerConnectionControl =
Expand Down Expand Up @@ -391,4 +387,31 @@ public void testBadRequest() throws Exception {
cburl.doOut(nos);
}

/**
* Method to test: {@link CircuitBreakerUrl#doOut(OutputStream)}
* Given scenario: Invoke {@link CircuitBreakerUrl#doOut(OutputStream)} using a bad request
* Expected Result: http status should be evaluated
*/
@Test
public void testBadRequest_dontThrow() throws Exception {
final NullOutputStream nos = new NullOutputStream();

final String key = "testBreaker";
final int timeout = 2000;

CircuitBreaker breaker = CurcuitBreakerPool.getBreaker(key);

assert (breaker.isClosed());

CircuitBreakerUrl cburl = CircuitBreakerUrl.builder()
.setUrl("http://sdsfsf.com")
.setMethod(Method.POST)
.setTimeout(timeout).setCircuitBreaker(breaker)
.setThrowWhenNot2xx(false)
.build();
cburl.doOut(nos);

assert (cburl.response() >= 400 && cburl.response() <= 499);
}

}
2 changes: 2 additions & 0 deletions dotCMS/src/main/java/com/dotcms/analytics/AnalyticsAPIImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -316,6 +316,7 @@ private CircuitBreakerUrl.Response<AccessToken> requestAccessToken(final Analyti
.setTryAgainAttempts(ANALYTICS_ACCESS_TOKEN_RENEW_ATTEMPTS)
.setHeaders(accessTokenHeaders())
.setRawData(prepareRequestData(analyticsApp))
.setThrowWhenNot2xx(false)
.build()
.doResponse(AccessToken.class);
logTokenResponse(response, analyticsApp);
Expand Down Expand Up @@ -366,6 +367,7 @@ private CircuitBreakerUrl.Response<AnalyticsKey> requestAnalyticsKey(final Analy
.setTimeout(ANALYTICS_KEY_RENEW_TIMEOUT)
.setTryAgainAttempts(ANALYTICS_KEY_RENEW_ATTEMPTS)
.setHeaders(analyticsKeyHeaders(accessToken))
.setThrowWhenNot2xx(false)
.build()
.doResponse(AnalyticsKey.class);
logKeyResponse(response, analyticsApp);
Expand Down
13 changes: 12 additions & 1 deletion dotCMS/src/main/java/com/dotcms/analytics/bayesian/BayesianAPIImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,11 +34,22 @@ public class BayesianAPIImpl implements BayesianAPI {
*/
@Override
public BayesianResult doBayesian(final BayesianInput input) {
final UUID metricId = UUID.randomUUID();
final long start = System.currentTimeMillis();
Logger.debug(this, String.format("BAYESIAN-CALCULATION [%s] START <<<<<", metricId));

// validate input
return noopFallback(input)
final BayesianResult bayesianResult = noopFallback(input)
.orElseGet(() -> input.type() == ABTestingType.AB
? getAbBayesianResult(input)
: getAbcBayesianResult(input));

final long end = System.currentTimeMillis();
Logger.debug(
this,
String.format("BAYESIAN-CALCULATION [%s] END - took [%d] secs >>>>>", metricId, (end - start) / 1000));

return bayesianResult;
}

/**
Expand Down
35 changes: 27 additions & 8 deletions dotCMS/src/main/java/com/dotcms/cube/CubeJSClient.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,16 +10,18 @@
import com.dotcms.http.CircuitBreakerUrl.Response;
import com.dotcms.util.DotPreconditions;
import com.dotcms.util.JsonUtil;
import com.dotmarketing.util.Logger;
import com.dotmarketing.util.UtilMethods;
import com.google.common.collect.ImmutableMap;
import com.liferay.util.StringPool;
import org.jetbrains.annotations.NotNull;

import javax.ws.rs.core.HttpHeaders;
import java.io.IOException;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.UUID;

/**
* CubeJS Client it allow to send a Request to a Cube JS Server.
Expand Down Expand Up @@ -99,26 +101,24 @@ public CubeJSResultSet send(final CubeJSQuery query) {
DotPreconditions.notNull(accessToken, "Access token not must be NULL");

final CircuitBreakerUrl cubeJSClient;
final String cubeJsUrl = String.format("%s/cubejs-api/v1/load", url);
try {
cubeJSClient = CircuitBreakerUrl.builder()
.setMethod(Method.GET)
.setHeaders(cubeJsHeaders(accessToken))
.setUrl(String.format("%s/cubejs-api/v1/load", url))
.setUrl(cubeJsUrl)
.setParams(map("query", query.toString()))
.setTimeout(4000)
.setThrowWhenNot2xx(false)
.build();
} catch (AnalyticsException e) {
throw new RuntimeException(e);
}

final Response<String> response = cubeJSClient.doResponse();
if (response.getStatusCode() == -1) {
throw new RuntimeException("CubeJS Server is not available");
}
final Response<String> response = getStringResponse(cubeJSClient, cubeJsUrl);

try {
final String responseAsString = UtilMethods.isSet(response) ? response.getResponse() :
StringPool.BLANK;
final String responseAsString = response.getResponse();
final Map<String, Object> responseAsMap = UtilMethods.isSet(responseAsString) && !responseAsString.equals("[]") ?
JsonUtil.getJsonFromString(responseAsString) : new HashMap<>();
final List<Map<String, Object>> data = (List<Map<String, Object>>) responseAsMap.get("data");
Expand All @@ -129,6 +129,25 @@ public CubeJSResultSet send(final CubeJSQuery query) {
}
}

private static Response<String> getStringResponse(final CircuitBreakerUrl cubeJSClient, final String url) {
final long start = System.currentTimeMillis();
final UUID metricId = UUID.randomUUID();
Logger.debug(CubeJSClient.class, String.format("CUBEJS-REQUEST [%s] START <<<<<", metricId));

final Response<String> response = cubeJSClient.doResponse();

final long end = System.currentTimeMillis();
Logger.debug(
CubeJSClient.class,
String.format("CUBEJS-REQUEST [%s] END - took [%d] secs >>>>>", metricId, (end - start) / 1000));

if (!CircuitBreakerUrl.isWithin2xx(response.getStatusCode())) {
throw new RuntimeException("CubeJS Server is not available");
}

return response;
}

/**
* Prepares access token request headers in a {@link Map} with values found in a {@link AccessToken} instance.
*
Expand Down
101 changes: 70 additions & 31 deletions dotCMS/src/main/java/com/dotcms/http/CircuitBreakerUrl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
import com.dotmarketing.util.Config;
import com.dotmarketing.util.Logger;
import com.dotmarketing.util.UtilMethods;
import com.fasterxml.jackson.core.type.TypeReference;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.ImmutableMap;
import com.liferay.util.StringPool;
Expand All @@ -21,6 +20,7 @@
import org.apache.commons.io.IOUtils;
import org.apache.http.Header;
import org.apache.http.HttpResponse;
import org.apache.http.client.ClientProtocolException;
import org.apache.http.client.config.RequestConfig;
import org.apache.http.client.methods.CloseableHttpResponse;
import org.apache.http.client.methods.HttpEntityEnclosingRequestBase;
Expand All @@ -40,7 +40,6 @@
import java.io.OutputStream;
import java.io.Serializable;
import java.net.URISyntaxException;
import java.util.HashMap;
import java.util.Map;
import java.util.Set;
import java.util.concurrent.ConcurrentHashMap;
Expand Down Expand Up @@ -72,13 +71,15 @@ public class CircuitBreakerUrl {
private final HttpRequestBase request;
private final boolean verbose;
private final String rawData;
private int response=-1;
private int response = -1;
private Header[] responseHeaders;
private final boolean allowRedirects;
private final boolean throwWhenNot2xx;

private static final Lazy<Integer> circuitBreakerMaxConnTotal = Lazy.of(()->Config.getIntProperty("CIRCUIT_BREAKER_MAX_CONN_TOTAL",100));
private static final Lazy<Boolean> allowAccessToPrivateSubnets = Lazy.of(()->Config.getBooleanProperty("ALLOW_ACCESS_TO_PRIVATE_SUBNETS", false));
private static final CircuitBreakerConnectionControl circuitBreakerConnectionControl = new CircuitBreakerConnectionControl(circuitBreakerMaxConnTotal.get());

/**
*
* @param proxyUrl
Expand Down Expand Up @@ -107,16 +108,32 @@ public CircuitBreakerUrl(final String proxyUrl, final long timeoutMs, final Circ
this(proxyUrl, timeoutMs, circuitBreaker, false);
}

public CircuitBreakerUrl(String proxyUrl, long timeoutMs, CircuitBreaker circuitBreaker, boolean verbose) {
this(proxyUrl, timeoutMs, circuitBreaker, new HttpGet(proxyUrl), ImmutableMap.of(),ImmutableMap.of(), verbose, null);
public CircuitBreakerUrl(final String proxyUrl,
final long timeoutMs,
final CircuitBreaker circuitBreaker,
final boolean verbose) {
this(
proxyUrl,
timeoutMs,
circuitBreaker,
new HttpGet(proxyUrl),
ImmutableMap.of(),
ImmutableMap.of(),
verbose,
null);
}


@VisibleForTesting
public CircuitBreakerUrl(String proxyUrl, long timeoutMs, CircuitBreaker circuitBreaker, HttpRequestBase request,
Map<String, String> params, Map<String, String> headers, boolean verbose, final String rawData) {
this(proxyUrl, timeoutMs, circuitBreaker, request,
params, headers, verbose, rawData, false);
public CircuitBreakerUrl(final String proxyUrl,
final long timeoutMs,
final CircuitBreaker circuitBreaker,
final HttpRequestBase request,
final Map<String, String> params,
final Map<String, String> headers,
final boolean verbose,
final String rawData) {
this(proxyUrl, timeoutMs, circuitBreaker, request, params, headers, verbose, rawData, false, true);

}

Expand All @@ -129,17 +146,31 @@ public CircuitBreakerUrl(String proxyUrl, long timeoutMs, CircuitBreaker circuit
* @param request
* @param params
* @param headers
* @param verbose
* @param rawData
* @param allowRedirects
* @param throwWhenNot2xx
*/
@VisibleForTesting
public CircuitBreakerUrl(String proxyUrl, long timeoutMs, CircuitBreaker circuitBreaker, HttpRequestBase request,
Map<String, String> params, Map<String, String> headers, boolean verbose, final String rawData, boolean allowRedirects) {
public CircuitBreakerUrl(final String proxyUrl,
final long timeoutMs,
final CircuitBreaker circuitBreaker,
final HttpRequestBase request,
final Map<String, String> params,
final Map<String, String> headers,
final boolean verbose,
final String rawData,
final boolean allowRedirects,
final boolean throwWhenNot2xx) {
this.proxyUrl = proxyUrl;
this.timeoutMs = timeoutMs;
this.circuitBreaker = circuitBreaker;
this.verbose=verbose;
this.verbose = verbose;
this.request = request;
this.rawData=rawData;
this.allowRedirects=allowRedirects;
this.rawData = rawData;
this.allowRedirects = allowRedirects;
this.throwWhenNot2xx = throwWhenNot2xx;

for (final String head : headers.keySet()) {
request.addHeader(head, headers.get(head));
}
Expand All @@ -165,14 +196,22 @@ public CircuitBreakerUrl(String proxyUrl, long timeoutMs, CircuitBreaker circuit
} catch (URISyntaxException e) {
throw new DotStateException(e);
}


}

public String doString() throws IOException {
try (ByteArrayOutputStream out = new ByteArrayOutputStream()) {
doOut(out);
return out.toString();
final String output = out.toString();
if (!isWithin2xx(this.response)) {
Logger.warn(
this,
String.format(
"Invalid response detected when consuming [%s] with http status [%d] and response:\n%s",
this.proxyUrl,
this.response,
output));
}
return output;
}
}

Expand Down Expand Up @@ -220,7 +259,7 @@ public void doOut(final HttpServletResponse response) throws IOException {
})
.onFailure(failure -> Logger.warn(this, "Connection attempts failed " + failure.getMessage()))
.run(ctx -> {
RequestConfig config = RequestConfig.custom()
final RequestConfig config = RequestConfig.custom()
.setConnectTimeout(Math.toIntExact(this.timeoutMs))
.setRedirectsEnabled(allowRedirects)
.setConnectionRequestTimeout(Math.toIntExact(this.timeoutMs))
Expand All @@ -242,11 +281,17 @@ public void doOut(final HttpServletResponse response) throws IOException {
this.response = innerResponse.getStatusLine().getStatusCode();

IOUtils.copy(innerResponse.getEntity().getContent(), out);
} catch (IOException ex) {
Logger.error(
this,
String.format("Error accessing [%s] due to [%s]", proxyUrl, ex.getMessage()),
ex);
throw ex;
}

// throw an error if the request is bad
if(this.response<200 || this.response>299){
throw new BadRequestException("got invalid response for url: " + this.proxyUrl + " response: " + this.response);
if (!isWithin2xx(this.response) && throwWhenNot2xx) {
throw new BadRequestException("Got invalid response for url: " + this.proxyUrl + " response: " + this.response);
}
}
});
Expand All @@ -259,6 +304,10 @@ public void doOut(final HttpServletResponse response) throws IOException {
}
}

public static boolean isWithin2xx(final int response) {
return response >= 200 && response <= 299;
}

private void copyHeaders(final HttpResponse innerResponse, final HttpServletResponse response) {
final Header contentTypeHeader = innerResponse.getFirstHeader("Content-Type");

Expand Down Expand Up @@ -287,16 +336,6 @@ public String toString() {
return "CircuitBreakerUrl [proxyUrl=" + proxyUrl + ", timeoutMs=" + timeoutMs + ", circuitBreaker=" + circuitBreaker + "]";
}

final static TypeReference<HashMap<String,Object>> typeRef = new TypeReference<HashMap<String,Object>>() {};

public Map<String,Object> doMap() {
return Try.of(() -> DotObjectMapperProvider.getInstance()
.getDefaultObjectMapper()
.readValue(doString(), typeRef))
.onFailure(e->Logger.warnAndDebug(CircuitBreakerUrl.class, e))
.getOrElse(HashMap::new);
}

public <T extends Serializable> T doObject(final Class<T> clazz) {
return Try.of(() -> DotObjectMapperProvider.getInstance().getDefaultObjectMapper().readValue(doString(), clazz))
.onFailure(e -> Logger.warnAndDebug(CircuitBreakerUrl.class, e))
Expand All @@ -309,7 +348,7 @@ public <T extends Serializable> Response<T> doResponse(final Class<T> clazz) {

public Response<String> doResponse() {
try {
return new Response(this);
return new Response<>(this);
} catch (IOException e) {
Logger.error(this, e.getMessage(), e);
return null;
Expand Down
Loading

0 comments on commit d244da2

Please sign in to comment.