Skip to content

HDDS-10658. audit log message having transaction id#6863

Closed
sumitagrawl wants to merge 4 commits intoapache:masterfrom
sumitagrawl:HDDS-10658
Closed

HDDS-10658. audit log message having transaction id#6863
sumitagrawl wants to merge 4 commits intoapache:masterfrom
sumitagrawl:HDDS-10658

Conversation

@sumitagrawl
Copy link
Contributor

What changes were proposed in this pull request?

For debuggability of om transaction and audit log mapping, audit log is included with transaction Id. And additionally unknown failure case, also having audit log with exception message and transaction id.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-10658

How was this patch tested?

  • Unit test is added

@sumitagrawl sumitagrawl requested a review from szetszwo June 25, 2024 09:00
Copy link
Contributor

@adoroszlai adoroszlai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @sumitagrawl for the patch.

Comment on lines +485 to +488
OzoneManagerProtocolProtos.UserInfo userInfo, TermIndex termIndex) {
if (null != auditMap) {
auditMap.put("Transaction", "" + termIndex.getIndex());
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method is for building the audit message based on the input map. termIndex should not be added as a parameter. It should be added to the map by the caller.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Chances of missing adding termIndex at every caller is high, so its done at common place. This is to avoid coding mistake, as this should be mandatory parameter for logging, and required for logging. Other parameter can be optional and based on specific request type.

Copy link
Contributor

@adoroszlai adoroszlai Jun 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This piece of code (passing -1):

buildAuditMessage(OMAction.GET_DELEGATION_TOKEN,
new LinkedHashMap<>(), ioe, request.getUserInfo(), TransactionInfo.getTermIndex(-1)));

suggests that it is not mandatory for all requests. On closer look this call is from preExecute, which doesn't have termIndex. Only requests related to delegation tokens do audit failures in preExecute. We may want to fix that.

I think we should change the place where audit logging happens for OM requests:

  • start building AuditMessage.Builder in requests, but do not build() and log it yet
  • add the in-progress AuditMessage.Builder to the OMClientResponse returned from validateAndUpdateCache
  • central server-side request/response handler (OzoneManagerProtocolServerSideTranslatorPB OzoneManagerRequestHandler) can add common information like termIndex, finish building AuditMessage and log it
  • OzoneManagerProtocolServerSideTranslatorPB can also log failures that occur in preExecute

@errose28
Copy link
Contributor

Can you include some before and after log message examples in the PR description for others to quickly reference?

@sumitagrawl
Copy link
Contributor Author

Changes are udpated in new pull request #6949

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants