Repository: brooklyn-server Updated Branches: refs/heads/master 01141a722 -> d05b9fd81
better logging of REST exceptions include the trace on the first encounter of a new type or simply a new place where a type is thrown from; subsequent instances won't get the trace but handy when debugging if the first one does Project: http://git-wip-us.apache.org/repos/asf/brooklyn-server/repo Commit: http://git-wip-us.apache.org/repos/asf/brooklyn-server/commit/5571b0de Tree: http://git-wip-us.apache.org/repos/asf/brooklyn-server/tree/5571b0de Diff: http://git-wip-us.apache.org/repos/asf/brooklyn-server/diff/5571b0de Branch: refs/heads/master Commit: 5571b0de2c62a81acfed95eabede3ec82e2e2caf Parents: 547feec Author: Alex Heneveld <[email protected]> Authored: Mon Apr 24 15:21:16 2017 +0100 Committer: Alex Heneveld <[email protected]> Committed: Mon Apr 24 15:26:55 2017 +0100 ---------------------------------------------------------------------- .../resources/AbstractBrooklynRestResource.java | 9 +++++ .../rest/resources/CatalogResource.java | 3 +- .../rest/util/DefaultExceptionMapper.java | 39 +++++++++++++++++--- 3 files changed, 44 insertions(+), 7 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/5571b0de/rest/rest-resources/src/main/java/org/apache/brooklyn/rest/resources/AbstractBrooklynRestResource.java ---------------------------------------------------------------------- diff --git a/rest/rest-resources/src/main/java/org/apache/brooklyn/rest/resources/AbstractBrooklynRestResource.java b/rest/rest-resources/src/main/java/org/apache/brooklyn/rest/resources/AbstractBrooklynRestResource.java index 082a620..3f33643 100644 --- a/rest/rest-resources/src/main/java/org/apache/brooklyn/rest/resources/AbstractBrooklynRestResource.java +++ b/rest/rest-resources/src/main/java/org/apache/brooklyn/rest/resources/AbstractBrooklynRestResource.java @@ -20,6 +20,7 @@ package org.apache.brooklyn.rest.resources; import javax.annotation.Nullable; import javax.ws.rs.core.Context; +import javax.ws.rs.core.Response; import javax.ws.rs.core.UriInfo; import javax.ws.rs.ext.ContextResolver; @@ -27,7 +28,9 @@ import org.apache.brooklyn.api.entity.Entity; import org.apache.brooklyn.api.mgmt.ManagementContext; import org.apache.brooklyn.core.config.render.RendererHints; import org.apache.brooklyn.core.mgmt.internal.ManagementContextInternal; +import org.apache.brooklyn.rest.domain.ApiError; import org.apache.brooklyn.rest.util.BrooklynRestResourceUtils; +import org.apache.brooklyn.rest.util.DefaultExceptionMapper; import org.apache.brooklyn.rest.util.ManagementContextProvider; import org.apache.brooklyn.rest.util.WebResourceUtils; import org.apache.brooklyn.rest.util.json.BrooklynJacksonJsonProvider; @@ -72,6 +75,12 @@ public abstract class AbstractBrooklynRestResource { return brooklynRestResourceUtils; } + /** returns a bad request Response wrapping the given exception */ + protected Response badRequest(Exception e) { + DefaultExceptionMapper.logExceptionDetailsForDebugging(e); + return ApiError.of(e).asBadRequestResponseJson(); + } + protected ObjectMapper mapper() { return mapper(mgmt()); } http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/5571b0de/rest/rest-resources/src/main/java/org/apache/brooklyn/rest/resources/CatalogResource.java ---------------------------------------------------------------------- diff --git a/rest/rest-resources/src/main/java/org/apache/brooklyn/rest/resources/CatalogResource.java b/rest/rest-resources/src/main/java/org/apache/brooklyn/rest/resources/CatalogResource.java index c2e800a..8c04aef 100644 --- a/rest/rest-resources/src/main/java/org/apache/brooklyn/rest/resources/CatalogResource.java +++ b/rest/rest-resources/src/main/java/org/apache/brooklyn/rest/resources/CatalogResource.java @@ -64,6 +64,7 @@ import org.apache.brooklyn.rest.domain.CatalogLocationSummary; import org.apache.brooklyn.rest.domain.CatalogPolicySummary; import org.apache.brooklyn.rest.filter.HaHotStateRequired; import org.apache.brooklyn.rest.transform.CatalogTransformer; +import org.apache.brooklyn.rest.util.DefaultExceptionMapper; import org.apache.brooklyn.rest.util.WebResourceUtils; import org.apache.brooklyn.util.collections.MutableList; import org.apache.brooklyn.util.collections.MutableMap; @@ -156,7 +157,7 @@ public class CatalogResource extends AbstractBrooklynRestResource implements Cat return buildCreateResponse(items); } catch (Exception e) { Exceptions.propagateIfFatal(e); - return ApiError.of(e).asBadRequestResponseJson(); + return badRequest(e); } } http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/5571b0de/rest/rest-resources/src/main/java/org/apache/brooklyn/rest/util/DefaultExceptionMapper.java ---------------------------------------------------------------------- diff --git a/rest/rest-resources/src/main/java/org/apache/brooklyn/rest/util/DefaultExceptionMapper.java b/rest/rest-resources/src/main/java/org/apache/brooklyn/rest/util/DefaultExceptionMapper.java index 010bcca..b2e640e 100644 --- a/rest/rest-resources/src/main/java/org/apache/brooklyn/rest/util/DefaultExceptionMapper.java +++ b/rest/rest-resources/src/main/java/org/apache/brooklyn/rest/util/DefaultExceptionMapper.java @@ -39,12 +39,15 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.yaml.snakeyaml.error.YAMLException; +import com.google.common.annotations.Beta; + @Provider public class DefaultExceptionMapper implements ExceptionMapper<Throwable> { private static final Logger LOG = LoggerFactory.getLogger(DefaultExceptionMapper.class); - static Set<Class<?>> warnedUnknownExceptions = MutableSet.of(); + static Set<Class<?>> encounteredUnknownExceptions = MutableSet.of(); + static Set<Object> encounteredExceptionRecords = MutableSet.of(); /** * Maps a throwable to a response. @@ -60,7 +63,7 @@ public class DefaultExceptionMapper implements ExceptionMapper<Throwable> { // Don't depend on jetty, could be running in other environments as well. if (throwable1.getClass().getName().equals("org.eclipse.jetty.io.EofException")) { if (LOG.isTraceEnabled()) { - LOG.trace("REST request running as {} threw: {}", Entitlements.getEntitlementContext(), + LOG.trace("REST request running as {} was disconnected, threw: {}", Entitlements.getEntitlementContext(), Exceptions.collapse(throwable1)); } return null; @@ -74,9 +77,7 @@ public class DefaultExceptionMapper implements ExceptionMapper<Throwable> { LOG.debug("REST request running as {} threw: {}", Entitlements.getEntitlementContext(), Exceptions.collapse(throwable1)); } - if (LOG.isTraceEnabled()) { - LOG.trace("Full details of "+Entitlements.getEntitlementContext()+" "+throwable1, throwable1); - } + logExceptionDetailsForDebugging(throwable1); // Some methods will throw this, which gets converted automatically if (throwable2 instanceof WebApplicationException) { @@ -103,7 +104,7 @@ public class DefaultExceptionMapper implements ExceptionMapper<Throwable> { } if (!Exceptions.isPrefixBoring(throwable2)) { - if ( warnedUnknownExceptions.add( throwable2.getClass() )) { + if ( encounteredUnknownExceptions.add( throwable2.getClass() )) { LOG.warn("REST call generated exception type "+throwable2.getClass()+" unrecognized in "+getClass()+" (subsequent occurrences will be logged debug only): " + throwable2, throwable2); } } @@ -120,6 +121,32 @@ public class DefaultExceptionMapper implements ExceptionMapper<Throwable> { return rb.build().asResponse(Status.INTERNAL_SERVER_ERROR, MediaType.APPLICATION_JSON_TYPE); } + /** Logs full details at trace, unless it is the first time we've seen this in which case it is debug */ + @Beta + public static void logExceptionDetailsForDebugging(Throwable t) { + if (LOG.isDebugEnabled()) { + if (firstEncounter(t)) { + // include debug trace for everything the first time we get one + LOG.debug("Full details of "+Entitlements.getEntitlementContext()+" "+t+" (logging debug on first encounter; subsequent instances will be logged trace)", t); + } else if (LOG.isTraceEnabled()) { + LOG.trace("Full details of "+Entitlements.getEntitlementContext()+" "+t, t); + } + } + } + + private static boolean firstEncounter(Throwable t) { + Set<Object> record = MutableSet.of(); + while (true) { + record.add(t.getClass()); + if (t.getStackTrace().length>0) { + if (record.add(t.getStackTrace()[0])) break; + } + t = t.getCause(); + if (t==null) break; + } + return encounteredExceptionRecords.add(record); + } + protected boolean isSevere(Throwable t) { // some things, like this, we want more prominent server notice of // (the list could be much larger but this is a start)
