-
Notifications
You must be signed in to change notification settings - Fork 149
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
NR-64056 Error grouping #1195
NR-64056 Error grouping #1195
Conversation
delete grouperimpl
Various ErrorDataImpl changes
add correction todo
… null handling in ErrorDataImpl
newrelic-agent/src/main/java/com/newrelic/api/agent/NewRelicApiImplementation.java
Outdated
Show resolved
Hide resolved
newrelic-agent/src/main/java/com/newrelic/agent/errors/ErrorDataImpl.java
Outdated
Show resolved
Hide resolved
newrelic-agent/src/main/java/com/newrelic/agent/errors/ErrorDataImpl.java
Outdated
Show resolved
Hide resolved
newrelic-agent/src/main/java/com/newrelic/agent/errors/ErrorDataImpl.java
Outdated
Show resolved
Hide resolved
newrelic-agent/src/main/java/com/newrelic/agent/errors/ErrorDataImpl.java
Outdated
Show resolved
Hide resolved
newrelic-agent/src/main/java/com/newrelic/agent/errors/ErrorDataImpl.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this looks good. There are some NPE risks and minor code style comments I'd like to be addressed beforehand.
newrelic-agent/src/test/java/com/newrelic/agent/errors/ErrorDataImplTest.java
Show resolved
Hide resolved
newrelic-agent/src/main/java/com/newrelic/agent/errors/ErrorGroupCallbackHolder.java
Outdated
Show resolved
Hide resolved
newrelic-agent/src/main/java/com/newrelic/agent/errors/ErrorServiceImpl.java
Outdated
Show resolved
Hide resolved
newrelic-agent/src/main/java/com/newrelic/agent/service/analytics/ErrorEventFactory.java
Outdated
Show resolved
Hide resolved
newrelic-agent/src/main/java/com/newrelic/agent/service/analytics/ErrorEventFactory.java
Outdated
Show resolved
Hide resolved
newrelic-agent/src/main/java/com/newrelic/agent/service/analytics/ErrorEventFactory.java
Outdated
Show resolved
Hide resolved
newrelic-api/src/main/java/com/newrelic/api/agent/ErrorGroupCallback.java
Show resolved
Hide resolved
Agent.LOG.warning(MessageFormat.format("Customer errorGroupCallback implementation threw an exception: {0}", e.getMessage())); | ||
NewRelic.getAgent().getLogger().log(Level.WARNING, "Customer errorGroupCallback implementation threw an exception: {0}", e.getMessage()); | ||
NewRelic.getAgent().getLogger().log(Level.FINEST, "Customer errorGroupCallback implementation stacktrace {0}:", | ||
Arrays.toString(e.getStackTrace()).replace(",", "\n")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd avoid the manual stack trace to string code there. You could use
NewRelic.getAgent().getLogger().log(Level.FINEST, e, "Customer errorGroupCallback stacktrace.");
though it will repeat the message.
So another option would be:
if (NewRelic.getAgent().getLogger().isLoggable(Level.FINEST)) {
NewRelic.getAgent().getLogger().log(Level.FINEST, e, "Customer errorGroupCallback threw an exception.");
} else {
NewRelic.getAgent().getLogger().log(Level.WARNING, "Customer errorGroupCallback implementation threw an exception: {0}", e.getMessage());
}
Implements the error grouping feature: