Skip to content

Commit 9997a68

Browse files
XcelledXcelled
authored
Merge pull request #93 from HubSpot/revert-91-mk-record-error-details
Revert "Record error details instead of just the check name"
2 parents f50603c + 26397d6 commit 9997a68

File tree

4 files changed

+56
-73
lines changed

4 files changed

+56
-73
lines changed

‎check_api/src/main/java/com/google/errorprone/hubspot/HubSpotMetrics.java

Lines changed: 49 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -25,21 +25,39 @@
2525
import java.util.concurrent.ConcurrentHashMap;
2626
import java.util.function.Supplier;
2727

28-
29-
import com.fasterxml.jackson.annotation.JsonProperty;
28+
import com.fasterxml.jackson.annotation.JsonValue;
29+
import com.google.common.base.Strings;
3030
import com.google.common.base.Suppliers;
3131
import com.google.common.base.Throwables;
32-
import com.google.common.collect.HashMultimap;
3332
import com.google.common.collect.ImmutableMap;
34-
import com.google.common.collect.Multimap;
35-
import com.google.common.collect.Multimaps;
3633
import com.google.common.util.concurrent.AtomicLongMap;
3734
import com.google.errorprone.DescriptionListener;
3835
import com.google.errorprone.ErrorProneTimings;
3936
import com.google.errorprone.matchers.Suppressible;
4037
import com.sun.tools.javac.util.Context;
4138

4239
public class HubSpotMetrics {
40+
41+
enum ErrorType {
42+
EXCEPTIONS("errorProneExceptions"),
43+
MISSING("errorProneMissingChecks"),
44+
INIT_ERRORS("errorProneInitErrors"),
45+
LISTENER_INIT_ERRORS("errorProneListenerInitErrors"),
46+
LISTENER_ON_DESCRIBE_ERROR("errorProneListenerDescribeErrors"),
47+
UNHANDLED_ERRORS("errorProneUnhandledErrors");
48+
49+
final String value;
50+
51+
ErrorType(String value) {
52+
this.value = value;
53+
}
54+
55+
@JsonValue
56+
public String getValue() {
57+
return value;
58+
}
59+
}
60+
4361
public static synchronized HubSpotMetrics instance(Context context) {
4462
HubSpotMetrics metrics = context.get(HubSpotMetrics.class);
4563

@@ -59,23 +77,26 @@ private static HubSpotMetrics create(Context context) {
5977
return metrics;
6078
}
6179

62-
private final ErrorState errors;
80+
private final ConcurrentHashMap<ErrorType, Set<String>> errors;
6381
private final AtomicLongMap<String> timings;
82+
6483
private final Supplier<FileManager> fileManagerSupplier;
6584

6685

6786
HubSpotMetrics(Supplier<FileManager> fileManagerSupplier) {
68-
this.errors = new ErrorState();
87+
this.errors = new ConcurrentHashMap<>();
6988
this.timings = AtomicLongMap.create();
7089
this.fileManagerSupplier = fileManagerSupplier;
7190
}
7291

73-
public void recordError(Suppressible s, Throwable t) {
74-
errors.exceptions.put(s.canonicalName(), t);
92+
public void recordError(Suppressible s) {
93+
errors.computeIfAbsent(ErrorType.EXCEPTIONS, ignored -> ConcurrentHashMap.newKeySet())
94+
.add(s.canonicalName());
7595
}
7696

7797
public void recordMissingCheck(String checkName) {
78-
errors.missing.add(checkName);
98+
errors.computeIfAbsent(ErrorType.MISSING, ignored -> ConcurrentHashMap.newKeySet())
99+
.add(checkName);
79100
}
80101

81102
public void recordTimings(Context context) {
@@ -85,11 +106,13 @@ public void recordTimings(Context context) {
85106
}
86107

87108
public void recordListenerDescribeError(DescriptionListener listener, Throwable t) {
88-
errors.listenerOnDescribeErrors.put(listener.getClass().getCanonicalName(), t);
109+
errors.computeIfAbsent(ErrorType.LISTENER_ON_DESCRIBE_ERROR, ignored -> ConcurrentHashMap.newKeySet())
110+
.add(toErrorMessage(t));
89111
}
90112

91113
public void recordUncaughtException(Throwable throwable) {
92-
errors.unhandledErrors.add(throwable);
114+
errors.computeIfAbsent(ErrorType.UNHANDLED_ERRORS, ignored -> ConcurrentHashMap.newKeySet())
115+
.add(toErrorMessage(throwable));
93116

94117
fileManagerSupplier.get().getUncaughtExceptionPath().ifPresent(p -> {
95118
// this should only ever be called once so overwriting is fine
@@ -103,12 +126,22 @@ public void recordUncaughtException(Throwable throwable) {
103126
});
104127
}
105128

106-
public void recordCheckLoadError(String name, Throwable t) {
107-
errors.initErrors.put(name, t);
129+
public void recordCheckLoadError(Throwable t) {
130+
errors.computeIfAbsent(ErrorType.INIT_ERRORS, ignored -> ConcurrentHashMap.newKeySet())
131+
.add(toErrorMessage(t));
108132
}
109133

110-
public void recordListenerInitError(String name, Throwable t) {
111-
errors.listenerInitErrors.put(name, t);
134+
public void recordListenerInitError(Throwable t) {
135+
errors.computeIfAbsent(ErrorType.LISTENER_INIT_ERRORS, ignored -> ConcurrentHashMap.newKeySet())
136+
.add(toErrorMessage(t));
137+
}
138+
139+
private static String toErrorMessage(Throwable e) {
140+
if (Strings.isNullOrEmpty(e.getMessage())) {
141+
return "Unknown error";
142+
} else {
143+
return e.getMessage();
144+
}
112145
}
113146

114147
private void write() {
@@ -126,23 +159,4 @@ private Map<String, Long> computeFinalTimings() {
126159
.collect(ImmutableMap.toImmutableMap(Entry::getKey, Entry::getValue));
127160
}
128161

129-
private static class ErrorState {
130-
@JsonProperty("errorProneExceptions")
131-
public final Multimap<String, Throwable> exceptions = Multimaps.synchronizedMultimap(HashMultimap.create());
132-
133-
@JsonProperty("errorProneMissingChecks")
134-
public final Set<String> missing = ConcurrentHashMap.newKeySet();
135-
136-
@JsonProperty("errorProneInitErrors")
137-
public final Multimap<String, Throwable> initErrors = Multimaps.synchronizedMultimap(HashMultimap.create());
138-
139-
@JsonProperty("errorProneListenerInitErrors")
140-
public final Multimap<String, Throwable> listenerInitErrors = Multimaps.synchronizedMultimap(HashMultimap.create());
141-
142-
@JsonProperty("errorProneListenerDescribeErrors")
143-
public final Multimap<String, Throwable> listenerOnDescribeErrors = Multimaps.synchronizedMultimap(HashMultimap.create());
144-
145-
@JsonProperty("errorProneUnhandledErrors")
146-
public final Set<Throwable> unhandledErrors = ConcurrentHashMap.newKeySet();
147-
}
148162
}

‎check_api/src/main/java/com/google/errorprone/hubspot/HubSpotUtils.java

Lines changed: 5 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,6 @@
2222
import java.util.Iterator;
2323
import java.util.List;
2424
import java.util.Optional;
25-
import java.util.concurrent.atomic.AtomicInteger;
2625

2726
import com.google.common.collect.ImmutableList;
2827
import com.google.errorprone.BugCheckerInfo;
@@ -58,17 +57,12 @@ public static void init(JavacTask task) {
5857
public static ScannerSupplier createScannerSupplier(Iterable<BugChecker> extraBugCheckers) {
5958
ImmutableList.Builder<BugCheckerInfo> builder = ImmutableList.builder();
6059
Iterator<BugChecker> iter = extraBugCheckers.iterator();
61-
62-
AtomicInteger i = new AtomicInteger(0);
63-
6460
while (iter.hasNext()) {
65-
BugChecker checker = null;
6661
try {
67-
checker = iter.next();
68-
builder.add(BugCheckerInfo.create(checker.getClass()));
62+
Class<? extends BugChecker> checker = iter.next().getClass();
63+
builder.add(BugCheckerInfo.create(checker));
6964
} catch (Throwable e) {
70-
String name = checker == null ? ("Unknown_" + i.incrementAndGet()) : checker.canonicalName();
71-
METRICS.ifPresent(metrics -> metrics.recordCheckLoadError(name, e));
65+
METRICS.ifPresent(metrics -> metrics.recordCheckLoadError(e));
7266
}
7367
}
7468
return ScannerSupplier.fromBugCheckerInfos(builder.build());
@@ -77,16 +71,11 @@ public static ScannerSupplier createScannerSupplier(Iterable<BugChecker> extraBu
7771
public static List<DescriptionListener> loadDescriptionListeners(Iterable<CustomDescriptionListenerFactory> factories, DescriptionListenerResources resources) {
7872
Iterator<CustomDescriptionListenerFactory> iter = factories.iterator();
7973
ImmutableList.Builder<DescriptionListener> listeners = ImmutableList.builder();
80-
81-
AtomicInteger i = new AtomicInteger(0);
8274
while (iter.hasNext()) {
83-
CustomDescriptionListenerFactory listener = null;
8475
try {
85-
listener = iter.next();
86-
listeners.add(listener.createFactory(resources));
76+
listeners.add(iter.next().createFactory(resources));
8777
} catch (Throwable t) {
88-
String name = listener == null ? ("Unknown_" + i.incrementAndGet()) : listener.getClass().getCanonicalName();
89-
METRICS.ifPresent(metrics -> metrics.recordListenerInitError(name, t));
78+
METRICS.ifPresent(metrics -> metrics.recordListenerInitError(t));
9079
}
9180
}
9281

‎check_api/src/main/java/com/google/errorprone/hubspot/module/CompilationEndAwareErrorPoneAnalyzer.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -154,7 +154,7 @@ private void onModuleFinished() {
154154
);
155155
} catch (Throwable t) {
156156
if (HubSpotUtils.isErrorHandlingEnabled(errorProneOptions)) {
157-
HubSpotMetrics.instance(context).recordError(bugChecker, t);
157+
HubSpotMetrics.instance(context).recordError(bugChecker);
158158
} else {
159159
throw t;
160160
}

‎check_api/src/main/java/com/google/errorprone/scanner/ErrorProneScanner.java

Lines changed: 1 addition & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -453,7 +453,7 @@ private <M extends Suppressible, T extends Tree> VisitorState processMatchers(
453453
stateWithSuppressionInformation);
454454
} catch (Exception | AssertionError t) {
455455
if (HubSpotUtils.isErrorHandlingEnabled(errorProneOptions)) {
456-
HubSpotMetrics.instance(oldState.context).recordError(matcher, getError(matcher, t));
456+
HubSpotMetrics.instance(oldState.context).recordError(matcher);
457457
} else {
458458
handleError(matcher, t);
459459
}
@@ -907,26 +907,6 @@ public Void visitWildcard(WildcardTree tree, VisitorState visitorState) {
907907
return super.visitWildcard(tree, state);
908908
}
909909

910-
/**
911-
* This is copied from the body of #handleError. Ideally we'd change that method to call this one,
912-
* but that would force us to declare `throws Throwable` which turns into a huge mess. To avoid
913-
* all of that we just copy the body verbatim.
914-
*/
915-
protected Throwable getError(Suppressible s, Throwable t) {
916-
if (t instanceof ErrorProneError) {
917-
return (ErrorProneError) t;
918-
}
919-
if (t instanceof CompletionFailure) {
920-
return (CompletionFailure) t;
921-
}
922-
TreePath path = getCurrentPath();
923-
return new ErrorProneError(
924-
s.canonicalName(),
925-
t,
926-
(DiagnosticPosition) path.getLeaf(),
927-
path.getCompilationUnit().getSourceFile());
928-
}
929-
930910
/**
931911
* Handles an exception thrown by an individual BugPattern. By default, wraps the exception in an
932912
* {@link ErrorProneError} and rethrows. May be overridden by subclasses, for example to log the

0 commit comments

Comments
 (0)