[GitHub] [incubator-zipkin] adriancole commented on a change in pull request #2552: Fixes logging and metrics for collectors

[GitBox]

adriancole commented on a change in pull request #2552: Fixes logging and 
metrics for collectors
URL: https://github.com/apache/incubator-zipkin/pull/2552#discussion_r280004552
 
 

 ##########
 File path: zipkin-collector/core/src/main/java/zipkin2/collector/Collector.java
 ##########
 @@ -178,60 +198,56 @@ void warn(String message, Throwable e) {
     return sampled;
   }
 
-  Callback<Void> acceptSpansCallback(final List<Span> spans) {
+  Callback<Void> storeSpansCallback(final List<Span> spans) {
     return new Callback<Void>() {
-      @Override
-      public void onSuccess(Void value) {}
+      @Override public void onSuccess(Void value) {
+      }
 
-      @Override
-      public void onError(Throwable t) {
-        errorStoringSpans(spans, t);
+      @Override public void onError(Throwable t) {
+        handleStorageError(spans, t, NOOP_CALLBACK);
       }
 
-      @Override
-      public String toString() {
-        return appendSpanIds(spans, new 
StringBuilder("AcceptSpans(")).append(")").toString();
+      @Override public String toString() {
+        return appendSpanIds(spans, new StringBuilder("StoreSpans(")) + ")";
       }
     };
   }
 
-  RuntimeException errorReading(Throwable e) {
-    return errorReading("Cannot decode spans", e);
-  }
-
-  RuntimeException errorReading(String message, Throwable e) {
+  void handleDecodeError(Throwable e, Callback<Void> callback) {
     metrics.incrementMessagesDropped();
-    return doError(message, e);
+    handleError(e, "Cannot decode spans"::toString, callback);
   }
 
   /**
    * When storing spans, an exception can be raised before or after the fact. 
This adds context of
    * span ids to give logs more relevance.
    */
-  RuntimeException errorStoringSpans(List<Span> spans, Throwable e) {
+  void handleStorageError(List<Span> spans, Throwable e, Callback<Void> 
callback) {
     metrics.incrementSpansDropped(spans.size());
     // The exception could be related to a span being huge. Instead of filling 
logs,
     // print trace id, span id pairs
-    StringBuilder msg = appendSpanIds(spans, new StringBuilder("Cannot store 
spans "));
-    return doError(msg.toString(), e);
+    handleError(e, () -> appendSpanIds(spans, new StringBuilder("Cannot store 
spans ")), callback);
   }
 
-  RuntimeException doError(String message, Throwable e) {
+  void handleError(Throwable e, Supplier<String> defaultLogMessage, 
Callback<Void> callback) {
+    propagateIfFatal(e);
+    callback.onError(e);
+    if (!logger.isLoggable(FINE)) return;
+
     String error = e.getMessage() != null ? e.getMessage() : "";
-    if (e instanceof RuntimeException
-        && (error.startsWith("Malformed") || error.startsWith("Truncated"))) {
-      if (shouldWarn()) warn(error, e);
-      return (RuntimeException) e;
-    } else {
-      if (shouldWarn()) {
-        message = format("%s due to %s(%s)", message, 
e.getClass().getSimpleName(), error);
-        warn(message, e);
-      }
-      return new RuntimeException(message, e);
+    // We have specific code that customizes log messages. Use this when the 
case.
 
 Review comment:
   PS I really don't think this special casing is helpful anymore

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services