Skip to content
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

[WFCORE-6476-fix]: Reducing logging level for failed internal read-on… #5733

Merged
merged 1 commit into from Oct 23, 2023

Conversation

ehsavoie
Copy link
Contributor

@ehsavoie ehsavoie commented Oct 20, 2023

…ly operation steps.

Jira issue: https://issues.redhat.com/browse/WFCORE-6476

  • Fixing missing case

@github-actions github-actions bot added the deps-ok Dependencies have been checked, and there are no significant changes label Oct 20, 2023
@@ -1072,7 +1072,7 @@ private void executeStep(final Step step) {
}
} catch (Throwable t) {
// Handling for throwables that don't implement OperationClientException marker interface
MGMT_OP_LOGGER.operationFailed(t, step.operation.get(OP), step.operation.get(OP_ADDR));
logStepFailure(step, false);
Copy link
Collaborator

@yersan yersan Oct 20, 2023

Choose a reason for hiding this comment

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

I am not sure why this case was left from the previous initial work. However, it makes sense to me.

I can only see two consequences:

  1. Errors arriving at this point when the stage is Stage.DOMAIN, will no longer logged, but @bstansberry added this comment on his previous change
    // Currently Stage.DOMAIN failure handling involves message manipulation before sending the
    // failure data to the client; logging stuff before that is done is liable to just produce a log mess.
    , so probably ignoring them would be fine.

If even with that we are not sure and want to be conservative, we could ask for the stage here and log them in such a case. For example

if (currentStage != Stage.DOMAIN) {
   logStepFailure(step, false);
} else {
MGMT_OP_LOGGER.operationFailed(t, step.operation.get(OP), 
step.operation.get(OP_ADDR));
}
  1. In the worst case, this change will change some specific logs from ERROR to DEBUG and will not change specific server behavior.

If there are no other thoughts, I would approve it as it is.

@pferraro , do you have any thoughts about this?

Copy link
Collaborator

@yersan yersan Oct 20, 2023

Choose a reason for hiding this comment

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

Another possible more conservative alternative is to use the new method only for the internal callers and left anything else as it was before:

if (!isExternalClient()) {
    logStepFailure(step, false);
} else {
    MGMT_OP_LOGGER.operationFailed(t, step.operation.get(OP), step.operation.get(OP_ADDR));
}

@ehsavoie Would the above resolve your issue too? It seems it could be a more secure option to avoid miss or change any log-level

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would also ensure that the op is read-only

…ly operation steps.

 * Fixing missing case

Signed-off-by: Emmanuel Hugonnet <ehugonne@redhat.com>
@yersan yersan added the ready-for-merge This PR is ready to be merged and fulfills all requirements label Oct 23, 2023
@yersan yersan merged commit 9073dc8 into wildfly:main Oct 23, 2023
12 checks passed
@yersan
Copy link
Collaborator

yersan commented Oct 23, 2023

Thanks @ehsavoie

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deps-ok Dependencies have been checked, and there are no significant changes ready-for-merge This PR is ready to be merged and fulfills all requirements
Projects
None yet
2 participants