egeria: catch (Throwable e) and catch(Exception e) without logging

catch(Throwable e) and catch(Exception e) need to log in the logger.

Example:

                        catch (Throwable  error)
                        {
                            OMAGAdminErrorCode errorCode = OMAGAdminErrorCode.BAD_ACCESS_SERVICE_ADMIN_CLASS;
                            String        errorMessage = errorCode.getErrorMessageId()
                                                       + errorCode.getFormattedErrorMessage(serverName,
                                                        accessServiceAdminClassName, 
                                                        accessServiceConfig.getAccessServiceName());

                            throw new OMAGConfigurationErrorException(errorCode.getHTTPErrorCode(),
                                                                      this.getClass().getName(),
                                                                      methodName,
                                                                      errorMessage,
                                                                      errorCode.getSystemAction(),
                                                                      errorCode.getUserAction());
                        }

This leads only to AuditLogs like:

OMAS-DATA-ENGINE-0004 The Data Engine Open Metadata Access Service (OMAS) is unable to initialize a new instance; error message is Failed to construct kafka consumer

And response like:

OMAG-ADMIN-400-011 The OMAG server omas has been passed an invalid admin services class name org.odpi.openmetadata.accessservices.assetlineage.admin.AssetLineageAdmin for access service Asset Lineage

which are not very useful to identify what technical issue is even if you set the logger on debug, because the original exceptions are lost. In the example could be kafka down or missing topic or wrong ssl connection etc.

For the cases of catch (Throwable e) and catch(Exception e) e needs to be logged with logger and or included in the new throw.

About this issue

Original URL
State: closed
Created 5 years ago
Comments: 29 (28 by maintainers)

Most upvoted comments

I see the point that relying on the logging during development may lead to neglect of proper audit logging. However, if the AuditLog is not perfect yet for all components and the system has to go in production anyway, there would not be any mechanism for the production team at all to properly diagnose errors. I agree with Bogdan that the capacity for the production team to properly diagnose should take priority over this incentive mechanism for developers to properly write auditlogging. The concern that developers will become “lazy” with their auditlogging could better be solved with explicit peer reviewing of their auditlogs instead.

GuyAckermans on Dec 6, 2019

Hi Mandy,

Thanks for the fast answer. Logging and AuditLog are not exclude each other. I agree that AuditLog should be also improved, and I agree that logging is useful only for people like us that are able to see the code. This is exactly what I mean. In this phase we are trying to deploy the app in prod and we are able to see the code also. I can’t see any reason not to have both. Simply adding one logging line and improve also the AuditLog to be more useful, are making app more powerful and give options in usage. This will allow us to move faster. Later on, when env will be more stable, and handed over, debugging can be turned off.

catch (OMAGConfigurationErrorException error)
{
    throw error;
}
catch (Throwable error)
{
    log.error(error);
    auditCode = AssetOwnerAuditCode.SERVICE_INSTANCE_FAILURE;
    auditLog.logRecord(actionDescription,
                       auditCode.getLogMessageId(),
                       auditCode.getSeverity(),
                       auditCode.getFormattedLogMessage(error.getMessage()),
                       null,
                       auditCode.getSystemAction(),
                       auditCode.getUserAction());
}

bogdan-sava on Dec 6, 2019

I disagree. Logging is harmless and it can be switched on and off. Logging is also offering the versatility to setup the verbosity through the severity level, package and class level.

Let’s fix the audit log but also adding logging.

bogdan-sava on Dec 6, 2019

There seems to be two levels of problem here. If I look at the example above, the final catch of Throwable should only be reached for a non-checked exception. Errors such as Kafka config errors should be handled by the called class and returned as OMAGConfigurationErrorException.

If we are not seeing that behaviour then it needs to be addressed.

For the audit log message above, the code should be changed to call logException() on the audit log and pass the exception. The call to log.error(error); should be removed.

mandy-chessell on Dec 6, 2019