Logging checks added. #39
Conversation
| } | ||
| } | ||
| //Some unknown error has occurred, returning an internal error | ||
| logger.error("Internal Recipient Error "); |
There was a problem hiding this comment.
Please add the exception to this log. Internal recipient errors need to be debugged, and a stack trace in the logs would greatly help here
| DefaultSuccessMAP returnValue = new DefaultSuccessMAP(infrastructureComponent.getId(), | ||
| infrastructureComponent.getOutboundModelVersion(), | ||
| messageAndPayload.getMessage().getId(), | ||
| securityTokenProvider.getSecurityTokenAsDAT(), | ||
| responseSenderUri); | ||
| logger.info(" Id "+ messageAndPayload.getMessage().getId().toString()); |
There was a problem hiding this comment.
These two lines do not provide meaningful context. Imagine reading a line
"ID 123456" in the logs. That does not really help to understand what happened
| if (msg.getAffectedResource() != null && messageAndPayload.getPayload().isPresent()) { | ||
|
|
||
| logger.info(issueAt.getYear()+":"+issueAt.getMonth()+ ":"+ issueAt.getDay()+":"+issueAt.getHour()+":"+ issueAt.getMinute()); | ||
| logger.info("Component ID :"+ infrastructureComponent.getId()); |
There was a problem hiding this comment.
This will be difficult to maintain. We will need to log a ton of messages, so it would be better to have one function to print log entries for a generic message. That makes the code re-usable for other messages than ResourceUpdateMessages
| } | ||
| } | ||
| catch (RejectMessageException ignored) | ||
| { | ||
| logger.error("Message could not be processed."); |
There was a problem hiding this comment.
This block of error messages is incorrect. This will always occur when a new resource is registered and should not be logged as an error. Please note that the exception name "ignored" indicates that this exception does not matter and can be safely ignored.
| @@ -112,6 +138,9 @@ public DefaultSuccessMAP handleValidated(ResourceMAP messageAndPayload) throws R | |||
| try { | |||
| //No Exception occurred. Send MessageProcessedNotificationMessage | |||
| DefaultSuccessMAP returnValue = new DefaultSuccessMAP(infrastructureComponent.getId(), infrastructureComponent.getOutboundModelVersion(), messageAndPayload.getMessage().getId(), securityTokenProvider.getSecurityTokenAsDAT(), responseSenderAgent); | |||
| logger.info("Component ID :"+ infrastructureComponent.getId().toString()); | |||
There was a problem hiding this comment.
Same as above: Rather log the incoming message via some central message-logger function. Then, here you can print a log that the registration was successful
| } | ||
| //return the (rewritten) URI of the infrastructure component | ||
| logger.info("The rewritten URI of the " +infrastructureComponent+" " + infrastructureComponent.getId().toString()); |
There was a problem hiding this comment.
This line makes no sense. Logging "infrastructureComponent" will give you some Java object ID
Kindly have a quick review of the logging checks added.