-
Notifications
You must be signed in to change notification settings - Fork 6
Add event message #47
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
Conversation
kortemik
left a comment
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.
will re-review once comments are fixed
src/test/java/com/teragrep/cfe_16/event/EventMessageStubTest.java
Outdated
Show resolved
Hide resolved
src/test/java/com/teragrep/cfe_16/event/EventMessageStubTest.java
Outdated
Show resolved
Hide resolved
src/test/java/com/teragrep/cfe_16/event/EventMessageStubTest.java
Outdated
Show resolved
Hide resolved
Laukkala
left a comment
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.
Only real issue I found was with the fact that Exceptions might not get logged if Response.body() is not called for some reason. Currently it seems that body() is being called eventually, but it might cause problems later on if another developer forgets to call it after receiving a Response. I would suggest separating the responsibility of writing to logs to another object, and leave the responsibility of generating the user-facing message as it is.
src/main/java/com/teragrep/cfe_16/response/ExceptionJsonResponse.java
Outdated
Show resolved
Hide resolved
Laukkala
left a comment
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.
With the changes to logging, this looks good to me once review comments from other reviewers have been resolved
src/main/java/com/teragrep/cfe_16/response/ExceptionJsonResponse.java
Outdated
Show resolved
Hide resolved
|
I'll remove the HttpStatus fields from Responses tomorrow, and look into refactoring the This is because of how Spring Boot handles response codes |
- Adds equalsVerifier to project dependencies
…l or not an JSON object - Does not throw an exception anymore
…an id field for it
- Now uses ExceptionEventContext for the data - toString() methods implemented for XForwarded* objects - equals() and hashCode() methods implemented for Xforwarded*Stubs
…nstead of IllegalStateException
…d catch it in HECServiceImpl.sendEvents
…nd there - Response code can now be set by each Response type - Integration Test tests that a failed request returns response code 400 and the correct response message and an id field - Status and Application types can be removed from the Response implementations
…ponseEntities - This has removed the status fields from all Response implementations - ResponseEntities contain the status code and content type
- These requests are supposed to fail
8c0a771 to
3ccdf30
Compare
src/main/java/com/teragrep/cfe_16/response/AcknowledgedJsonResponse.java
Outdated
Show resolved
Hide resolved
src/main/java/com/teragrep/cfe_16/response/ExceptionJsonResponse.java
Outdated
Show resolved
Hide resolved
|
otherwise ok |
…onNodeResponseEntity
…odeResponseEntity
A smaller PR from #15, which brings the EventMessage related classes.
HttpEventData