Avoid creating ZuulException as it has a static reference to increment metrics.

fixes gh-2912
parent c3eedabb
...@@ -18,19 +18,19 @@ package org.springframework.cloud.netflix.zuul.filters.post; ...@@ -18,19 +18,19 @@ package org.springframework.cloud.netflix.zuul.filters.post;
import javax.servlet.RequestDispatcher; import javax.servlet.RequestDispatcher;
import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
import com.netflix.zuul.ZuulFilter;
import com.netflix.zuul.context.RequestContext;
import com.netflix.zuul.exception.ZuulException;
import org.apache.commons.logging.Log; import org.apache.commons.logging.Log;
import org.apache.commons.logging.LogFactory; import org.apache.commons.logging.LogFactory;
import org.springframework.beans.factory.annotation.Value; import org.springframework.beans.factory.annotation.Value;
import org.springframework.cloud.netflix.zuul.util.ZuulRuntimeException; import org.springframework.cloud.netflix.zuul.util.ZuulRuntimeException;
import org.springframework.http.HttpStatus;
import org.springframework.util.ReflectionUtils; import org.springframework.util.ReflectionUtils;
import org.springframework.util.StringUtils; import org.springframework.util.StringUtils;
import com.netflix.zuul.ZuulFilter;
import com.netflix.zuul.context.RequestContext;
import com.netflix.zuul.exception.ZuulException;
import static org.springframework.cloud.netflix.zuul.filters.support.FilterConstants.ERROR_TYPE; import static org.springframework.cloud.netflix.zuul.filters.support.FilterConstants.ERROR_TYPE;
import static org.springframework.cloud.netflix.zuul.filters.support.FilterConstants.SEND_ERROR_FILTER_ORDER; import static org.springframework.cloud.netflix.zuul.filters.support.FilterConstants.SEND_ERROR_FILTER_ORDER;
...@@ -70,16 +70,16 @@ public class SendErrorFilter extends ZuulFilter { ...@@ -70,16 +70,16 @@ public class SendErrorFilter extends ZuulFilter {
public Object run() { public Object run() {
try { try {
RequestContext ctx = RequestContext.getCurrentContext(); RequestContext ctx = RequestContext.getCurrentContext();
ZuulException exception = findZuulException(ctx.getThrowable()); ExceptionHolder exception = findZuulException(ctx.getThrowable());
HttpServletRequest request = ctx.getRequest(); HttpServletRequest request = ctx.getRequest();
request.setAttribute("javax.servlet.error.status_code", exception.nStatusCode); request.setAttribute("javax.servlet.error.status_code", exception.getStatusCode());
log.warn("Error during filtering", exception); log.warn("Error during filtering", exception.getThrowable());
request.setAttribute("javax.servlet.error.exception", exception); request.setAttribute("javax.servlet.error.exception", exception.getThrowable());
if (StringUtils.hasText(exception.errorCause)) { if (StringUtils.hasText(exception.getErrorCause())) {
request.setAttribute("javax.servlet.error.message", exception.errorCause); request.setAttribute("javax.servlet.error.message", exception.getErrorCause());
} }
RequestDispatcher dispatcher = request.getRequestDispatcher( RequestDispatcher dispatcher = request.getRequestDispatcher(
...@@ -87,7 +87,7 @@ public class SendErrorFilter extends ZuulFilter { ...@@ -87,7 +87,7 @@ public class SendErrorFilter extends ZuulFilter {
if (dispatcher != null) { if (dispatcher != null) {
ctx.set(SEND_ERROR_FILTER_RAN, true); ctx.set(SEND_ERROR_FILTER_RAN, true);
if (!ctx.getResponse().isCommitted()) { if (!ctx.getResponse().isCommitted()) {
ctx.setResponseStatusCode(exception.nStatusCode); ctx.setResponseStatusCode(exception.getStatusCode());
dispatcher.forward(request, ctx.getResponse()); dispatcher.forward(request, ctx.getResponse());
} }
} }
...@@ -98,24 +98,72 @@ public class SendErrorFilter extends ZuulFilter { ...@@ -98,24 +98,72 @@ public class SendErrorFilter extends ZuulFilter {
return null; return null;
} }
ZuulException findZuulException(Throwable throwable) { protected ExceptionHolder findZuulException(Throwable throwable) {
if (throwable.getCause() instanceof ZuulRuntimeException) { if (throwable.getCause() instanceof ZuulRuntimeException) {
// this was a failure initiated by one of the local filters // this was a failure initiated by one of the local filters
return (ZuulException) throwable.getCause().getCause(); return new ZuulExceptionHolder((ZuulException) throwable.getCause().getCause());
} }
if (throwable.getCause() instanceof ZuulException) { if (throwable.getCause() instanceof ZuulException) {
// wrapped zuul exception // wrapped zuul exception
return (ZuulException) throwable.getCause(); return new ZuulExceptionHolder((ZuulException) throwable.getCause());
} }
if (throwable instanceof ZuulException) { if (throwable instanceof ZuulException) {
// exception thrown by zuul lifecycle // exception thrown by zuul lifecycle
return (ZuulException) throwable; return new ZuulExceptionHolder((ZuulException) throwable);
}
// fallback
return new DefaultExceptionHolder(throwable);
}
protected interface ExceptionHolder {
Throwable getThrowable();
default int getStatusCode() {
return HttpStatus.INTERNAL_SERVER_ERROR.value();
}
default String getErrorCause() {
return null;
} }
}
protected static class DefaultExceptionHolder implements ExceptionHolder {
private final Throwable throwable;
public DefaultExceptionHolder(Throwable throwable) {
this.throwable = throwable;
}
@Override
public Throwable getThrowable() {
return this.throwable;
}
}
protected static class ZuulExceptionHolder implements ExceptionHolder {
private final ZuulException exception;
// fallback, should never get here public ZuulExceptionHolder(ZuulException exception) {
return new ZuulException(throwable, HttpServletResponse.SC_INTERNAL_SERVER_ERROR, null); this.exception = exception;
}
@Override
public Throwable getThrowable() {
return this.exception;
}
@Override
public int getStatusCode() {
return this.exception.nStatusCode;
}
@Override
public String getErrorCause() {
return this.exception.errorCause;
}
} }
public void setErrorPath(String errorPath) { public void setErrorPath(String errorPath) {
......
...@@ -18,7 +18,6 @@ ...@@ -18,7 +18,6 @@
package org.springframework.cloud.netflix.zuul.util; package org.springframework.cloud.netflix.zuul.util;
import com.netflix.zuul.exception.ZuulException; import com.netflix.zuul.exception.ZuulException;
import org.springframework.http.HttpStatus;
/** /**
* @author Spencer Gibb * @author Spencer Gibb
...@@ -30,6 +29,6 @@ public class ZuulRuntimeException extends RuntimeException { ...@@ -30,6 +29,6 @@ public class ZuulRuntimeException extends RuntimeException {
} }
public ZuulRuntimeException(Exception ex) { public ZuulRuntimeException(Exception ex) {
this(new ZuulException(ex, HttpStatus.INTERNAL_SERVER_ERROR.value(), null)); super(ex);
} }
} }
...@@ -23,17 +23,21 @@ import com.netflix.loadbalancer.Server; ...@@ -23,17 +23,21 @@ import com.netflix.loadbalancer.Server;
import com.netflix.loadbalancer.ServerList; import com.netflix.loadbalancer.ServerList;
import com.netflix.zuul.ZuulFilter; import com.netflix.zuul.ZuulFilter;
import com.netflix.zuul.context.RequestContext; import com.netflix.zuul.context.RequestContext;
import io.micrometer.core.instrument.MeterRegistry;
import io.micrometer.core.instrument.MockClock;
import io.micrometer.core.instrument.simple.SimpleConfig;
import io.micrometer.core.instrument.simple.SimpleMeterRegistry;
import org.junit.After; import org.junit.After;
import org.junit.Before; import org.junit.Before;
import org.junit.Test; import org.junit.Test;
import org.junit.runner.RunWith; import org.junit.runner.RunWith;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.boot.SpringBootConfiguration; import org.springframework.boot.SpringBootConfiguration;
import org.springframework.boot.autoconfigure.EnableAutoConfiguration; import org.springframework.boot.autoconfigure.EnableAutoConfiguration;
import org.springframework.boot.web.server.LocalServerPort;
import org.springframework.boot.test.context.SpringBootTest; import org.springframework.boot.test.context.SpringBootTest;
import org.springframework.boot.test.web.client.TestRestTemplate; import org.springframework.boot.test.web.client.TestRestTemplate;
import org.springframework.boot.web.server.LocalServerPort;
import org.springframework.cloud.netflix.ribbon.RibbonClient; import org.springframework.cloud.netflix.ribbon.RibbonClient;
import org.springframework.cloud.netflix.ribbon.StaticServerList; import org.springframework.cloud.netflix.ribbon.StaticServerList;
import org.springframework.cloud.netflix.zuul.EnableZuulProxy; import org.springframework.cloud.netflix.zuul.EnableZuulProxy;
...@@ -58,6 +62,8 @@ import static org.springframework.cloud.netflix.zuul.filters.support.FilterConst ...@@ -58,6 +62,8 @@ import static org.springframework.cloud.netflix.zuul.filters.support.FilterConst
@SpringBootTest(classes = SendErrorFilterIntegrationTests.Config.class, properties = "zuul.routes.filtertest:/filtertest/**", webEnvironment = RANDOM_PORT) @SpringBootTest(classes = SendErrorFilterIntegrationTests.Config.class, properties = "zuul.routes.filtertest:/filtertest/**", webEnvironment = RANDOM_PORT)
@DirtiesContext @DirtiesContext
public class SendErrorFilterIntegrationTests { public class SendErrorFilterIntegrationTests {
@Autowired
private MeterRegistry meterRegistry;
@LocalServerPort @LocalServerPort
private int port; private int port;
...@@ -79,6 +85,15 @@ public class SendErrorFilterIntegrationTests { ...@@ -79,6 +85,15 @@ public class SendErrorFilterIntegrationTests {
ResponseEntity<String> response = new TestRestTemplate().getForEntity(url, ResponseEntity<String> response = new TestRestTemplate().getForEntity(url,
String.class); String.class);
assertThat(response.getStatusCode()).isEqualTo(HttpStatus.INTERNAL_SERVER_ERROR); assertThat(response.getStatusCode()).isEqualTo(HttpStatus.INTERNAL_SERVER_ERROR);
assertMetrics("pre");
}
private void assertMetrics(String filterType) {
Double count = meterRegistry.counter("ZUUL::EXCEPTION:"+ filterType +"::500").count();
assertThat(count.longValue()).isEqualTo(1L);
count = meterRegistry.counter("ZUUL::EXCEPTION:null:500").count();
assertThat(count.longValue()).isEqualTo(0L);
} }
@Test @Test
...@@ -87,6 +102,8 @@ public class SendErrorFilterIntegrationTests { ...@@ -87,6 +102,8 @@ public class SendErrorFilterIntegrationTests {
ResponseEntity<String> response = new TestRestTemplate().getForEntity(url, ResponseEntity<String> response = new TestRestTemplate().getForEntity(url,
String.class); String.class);
assertThat(response.getStatusCode()).isEqualTo(HttpStatus.INTERNAL_SERVER_ERROR); assertThat(response.getStatusCode()).isEqualTo(HttpStatus.INTERNAL_SERVER_ERROR);
assertMetrics("route");
} }
@Test @Test
...@@ -95,6 +112,8 @@ public class SendErrorFilterIntegrationTests { ...@@ -95,6 +112,8 @@ public class SendErrorFilterIntegrationTests {
ResponseEntity<String> response = new TestRestTemplate().getForEntity(url, ResponseEntity<String> response = new TestRestTemplate().getForEntity(url,
String.class); String.class);
assertThat(response.getStatusCode()).isEqualTo(HttpStatus.INTERNAL_SERVER_ERROR); assertThat(response.getStatusCode()).isEqualTo(HttpStatus.INTERNAL_SERVER_ERROR);
assertMetrics("post");
} }
@SpringBootConfiguration @SpringBootConfiguration
...@@ -138,6 +157,11 @@ public class SendErrorFilterIntegrationTests { ...@@ -138,6 +157,11 @@ public class SendErrorFilterIntegrationTests {
} }
}; };
} }
@Bean
public MeterRegistry meterRegistry() {
return new SimpleMeterRegistry(SimpleConfig.DEFAULT, new MockClock());
}
} }
public static class RibbonConfig { public static class RibbonConfig {
......
...@@ -23,6 +23,7 @@ import org.springframework.beans.factory.annotation.Autowired; ...@@ -23,6 +23,7 @@ import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.boot.autoconfigure.EnableAutoConfiguration; import org.springframework.boot.autoconfigure.EnableAutoConfiguration;
import org.springframework.boot.test.context.SpringBootTest; import org.springframework.boot.test.context.SpringBootTest;
import org.springframework.cloud.netflix.zuul.EnableZuulServer; import org.springframework.cloud.netflix.zuul.EnableZuulServer;
import org.springframework.cloud.netflix.zuul.util.ZuulRuntimeException;
import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Bean;
import org.springframework.context.annotation.Configuration; import org.springframework.context.annotation.Configuration;
import org.springframework.test.annotation.DirtiesContext; import org.springframework.test.annotation.DirtiesContext;
...@@ -68,10 +69,15 @@ public class ZuulMetricsApplicationTests { ...@@ -68,10 +69,15 @@ public class ZuulMetricsApplicationTests {
@Test @Test
@SuppressWarnings("all") @SuppressWarnings("all")
public void shouldIncrementCounters() throws Exception { public void shouldIncrementCounters() throws Exception {
new ZuulRuntimeException(new Exception());
Double count = meterRegistry.counter("ZUUL::EXCEPTION:null:500").count();
assertEquals(count.longValue(), 0L);
new ZuulException("any", 500, "cause"); new ZuulException("any", 500, "cause");
new ZuulException("any", 500, "cause"); new ZuulException("any", 500, "cause");
Double count = meterRegistry.counter("ZUUL::EXCEPTION:cause:500").count(); count = meterRegistry.counter("ZUUL::EXCEPTION:cause:500").count();
assertEquals(count.longValue(), 2L); assertEquals(count.longValue(), 2L);
new ZuulException("any", 404, "cause2"); new ZuulException("any", 404, "cause2");
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment