2.0.0 #56
2.0.0 #56
Conversation
- We may want to turn on checkstyle in the build/compile phase but that throws too many errors right now. So it's only in the site phase for now. - The serialization code relies on some naming conventions divergent from Java conventions, so some style checks are suppressed (see ./suppresions.xml) - A few Sun conventions are disabled in ./checkstyle.xml because they are either problematic in a library context or they just don't make a lot of sense for this project.
This checkin is really just to see if Travis is configured right on my branch. But it does change some things to match checkstyle rules.
- Mainly ones where there should be no serialization side-effects - Also tweaks suppression config
I don't have any religion on this, but it's consistent with Sun style, and there's no clear reason to fight it.
- Preparation for jackson migration
I don't know the tests well enough to be confident these changes are all good, so I'm going to check a few things by hand, but this should be the last hurdle to migrating to Jackson serialization.
- Sticks with 2.8.11 (provided version) to avoid convergent dependency issues - Will need to deal with Map serialization mechanism change in Jackson 2.9 - This commit leaves traces of Gson in place; the next one will delete all traces of GSON. The purpose of this two-phased change is just so the bifurcation of the code is easy to identify.
Per Elijah's suggestion
Issue #48 on the honeybadger-io github issues list - candidate solution for removing thread local hack in HoneyBadgerNoticeLoader - Uses a Jackson-specific feature for injecting values. - passes integration tests locally, but may need to do some additional work to be sure it works all the way down the object graph - Probably need to remove a deprecated constructor or two in the DTOs
- This exposed a bit of a flaw in the HoneyBadgerNoticeLoader design, but because the only known use of that code is in Integration tests, I've implemented a change in the test. Load data and Memory usage is not available from the Notice retrieval API, and it was previously automatically generated at construction time, or via the thread-local version of configContext.
- TODO: Explain equivalent for gradle.properties - I just use environment variables locally but I'm not sure this is good general advice.
Revise Readme instructions for testing
Avoid Thread Local Storage in deserialization of Notices
- Still not quite sure how to do maven release yet since I don't have experience publishing to public maven repos, but I can probably update the instructions when we're a little closer
Update readme - Update test instructions - Explain current state of tags support
Complete serialization migration to Jackson and update documents
| this.config = config; | ||
| } | ||
|
|
||
| String pullFaultJson(final UUID faultId) throws IOException { |
There was a problem hiding this comment.
I would recommend the following improvement to HoneybadgerNoticeLoader.
This will allow you to parse the incoming JSON data as a stream rather than parsing it to a String and then to a JSON data structure. Additionally, it uses readValue(jsonText, ObjectNode.class) instead of readText(jsonText) because this way the parser should throw an error if it isn't getting a proper JSON object (I don't feel strongly about this change - take it or reject as you see fit).
--- a/honeybadger-java/src/main/java/io/honeybadger/loader/HoneybadgerNoticeLoader.java
+++ b/honeybadger-java/src/main/java/io/honeybadger/loader/HoneybadgerNoticeLoader.java
@@ -15,8 +15,8 @@ import org.apache.http.client.fluent.Response;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-import java.io.ByteArrayOutputStream;
import java.io.IOException;
+import java.io.InputStream;
import java.net.URI;
import java.util.UUID;
@@ -47,7 +47,7 @@ public class HoneybadgerNoticeLoader {
this.config = config;
}
- String pullFaultJson(final UUID faultId) throws IOException {
+ InputStream pullFaultJson(final UUID faultId) throws IOException {
String readApiKey = config.getHoneybadgerReadApiKey();
if (readApiKey == null) {
@@ -94,18 +94,18 @@ public class HoneybadgerNoticeLoader {
}
}
- ByteArrayOutputStream out = new ByteArrayOutputStream();
- httpResponse.getEntity().writeTo(out);
-
- return out.toString();
+ return httpResponse.getEntity().getContent();
}
public Notice findErrorDetails(final UUID faultId) throws IOException {
- String jsonText = pullFaultJson(faultId);
+ final ObjectNode originalJson;
+
+ try (InputStream jsonText = pullFaultJson(faultId)) {
+ originalJson = OBJECT_MAPPER.readValue(jsonText, ObjectNode.class);
+ }
// HACK: Since our API is not symmetric, we do this in order to rename fields
// and get *some* of the data that we sent.
- JsonNode originalJson = OBJECT_MAPPER.readTree(jsonText);
JsonNode cgiData = originalJson.get("web_environment");
((ObjectNode)originalJson.get("request"))
.replace("cgi_data", cgiData);There was a problem hiding this comment.
As HoneyBadgerNoticeLoader is currently only used by tests, I have mixed feelings about this. Probably makes more sense to optimize the submitError code in HoneybadgerReporter (or sendToHoneybadger maybe).
There was a problem hiding this comment.
That makes sense. If you want the context of the Json that errored, you may want to embed the actual Json in the exception. I forget if Jackson now does this for you or not, but I seem to remember that it only shows you a subset of the overall JSON when there is a parsing error.
| @@ -12,7 +12,7 @@ | |||
| */ | |||
| public interface ConfigContext { | |||
| /** Global reference to the currently used config per thread. */ | |||
| ThreadLocal<ConfigContext> threadLocal = new ThreadLocal<>(); | |||
| ThreadLocal<ConfigContext> THREAD_LOCAL = new ThreadLocal<>(); | |||
There was a problem hiding this comment.
After your refactors, I think this field is no longer needed and can be safely deleted. For a good reason too! The original implementation was wacky and was there for expediency.
There was a problem hiding this comment.
I thought I deleted that before. Maybe a weird merge quirk. I'll get rid of it.
| * @author <a href="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/dekobon">Elijah Zupancic</a> | ||
| * @since 1.0.9 | ||
| */ | ||
| @JsonInclude(JsonInclude.Include.NON_NULL) |
There was a problem hiding this comment.
Have you verified in the crywolf application that the shaded jars work properly with the Jackson annotations?
There was a problem hiding this comment.
Yes, CryWolf appears to work without issue. (There's one thing I was not 100% confident of related to setting the Context, I plan to do some testing around that tomorrow). I also created a branch with an example of 2.6 Play (which doesn't seem to have major differences from 1.x, but a couple of subtle ones)
There was a problem hiding this comment.
I would be sure to test again after fixing the shade relocation issues.
honeybadger-java-shaded/pom.xml
Outdated
| <pattern>com.github</pattern> | ||
| <shadedPattern>io.honeybadger.com.github</shadedPattern> | ||
| </relocation> | ||
| </relocations> |
There was a problem hiding this comment.
There was no relocation added for the jackson dependency, so the jackson libraries would conflict with anyone's existing libraries who imported the shaded jar.
I would add:
<relocation>
<pattern>com.fasterxml</pattern>
<shadedPattern>io.honeybadger.com.fasterxml</shadedPattern>
</relocation>There was a problem hiding this comment.
Will push that change in a bit.
There was a problem hiding this comment.
I believe this is working with the shaded artifact. I thought I was seeing an issue, but it turned out that the servlet app in crywolf explicitly sets the honeybadger.api_key in its web.xml, overriding my environment variable. I think I noticed that the first time I used crywolf, but I missed it today. Not sure if I should change CryWolf instructions or remove the key from the pom. (Or change the precedence of configuration methods?)
| "correct code. Response was [{}]. Retries={}", | ||
| responseCode, retries); | ||
| } else { | ||
| UUID id = parseErrorId(response); |
There was a problem hiding this comment.
This could return null we should handle this such that a user is left with an NullPointerException without an understanding of why it happened.
There was a problem hiding this comment.
I'm not quite sure I want to throw an exception for that case; it seems like that would create more confusion, and would change the interface, requiring a bit of rethinking of how consuming classes work. If I'm understanding right, it would only happen with malformed JSON responses; other error responses result in retries or failover. The null value should be sufficient for log parsing.
| notice.setError(noticeDetails); | ||
| } | ||
|
|
||
| String json; |
There was a problem hiding this comment.
I would put a comment here indicating that we are writing our JSON payload to a string because it allows us to resend it if the PUT HTTP call fails. If we were to use a stream, we wouldn't be able to easily recreate the stream in order to try again. If we really wanted to make it memory efficient, we could pass around a lambda function that would do the stream creation from the Notice instance.
There was a problem hiding this comment.
I think the subsequent retry logic is probably obvious enough, but I'll go ahead and add that.
| private static Set<Class<?>> findExceptionContextClasses() { | ||
| final String[] classNames = new String[] { | ||
| "org.apache.commons.lang3.exception.ExceptionContext", | ||
| "com.joyent.manta.org.apache.commons.lang3.exception.ExceptionContext" |
There was a problem hiding this comment.
We should probably delete the shaded class that I was using from testing with my monitoring utility and provide a way for users to configure their own class names here.
There was a problem hiding this comment.
I'll create a separate issue for the customization.
|
|
||
| <!-- Checks for Javadoc comments. --> | ||
| <!-- See http:https://checkstyle.sf.net/config_javadoc.html --> | ||
| <!--TODO: restore javadoc warnings once we're further along--> |
There was a problem hiding this comment.
You may want to address this TODO before merge.
JasonTrue
dismissed
dekobon’s stale review
Complied with most recommendations, just dismissing to make github cooperate
* Support configurable retry attempts Issue #47 - Discovered and fixed an unexpected bug that skipped retries in the case of an IO Exception - Added system property maximum_retry_attempts (Integer, constraints: value >= 1) - Changed the logic so the property is actually the max number of retries, not attempts. (This is a difference in behavior from 1.0) - Added explanatory note to property usage in Readme - Appropriate tests
|
OK, once CI passes (or gets fixed, assuming any disasters) I will merge up to Master. |
Moving 2.0.0 changes into master.
@dekobon please take a look to see if you notice any egregious issues. I haven't (yet) added support for hooking into the error handlers for application-specific tagging logic, but may look at that shortly.