-
Notifications
You must be signed in to change notification settings - Fork 31
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
Log creaper commands in JSON format in debug #153
Conversation
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.
Generally LGTM, just see my comment above and provide your opinion on it.
@@ -149,7 +149,8 @@ public void apply(Iterable<OnlineCommand> commands) throws CommandFailedExceptio | |||
public ModelNodeResult execute(ModelNode operation) throws IOException { | |||
checkClosed(); | |||
operation = adjustOperationForDomain.adjust(operation); | |||
log.debugf("Executing operation %s", ModelNodeOperationToCliString.convert(operation)); | |||
log.debugf("Executing operation %s\nJSON format:\n%s", ModelNodeOperationToCliString.convert(operation), | |||
operation.toJSONString(false)); |
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.
I believe we should be consistent with other execute
methods, I suggest to do similar update mainly for execute
on Operation
. Not sure whether it would make sense to have it also for execute
on String
for some debugging purposes, I guess not.
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.
This should absolutely be added to execute(Operation)
too, yes. There's no way to do that for execute(String)
, though.
One idea: I'm not terribly happy about newlines in a log message, how about adding it as a second log, possibly to the TRACE level?
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.
Thanks for noticing. I have overlooked operation.getOperation()
returns ModelNode
.
Regarding newlines. I was considering both options. I have intentionally decided for this option, because of selecting JSON string. It is ensured than no unintentional character will be copied.
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.
BTW, Operation.getOperation
gives you a ModelNode
, but the entire purpose of the Operation
class is to have attachments, the ModelNode
doesn't contain everything. Just a notice.
I'm still not exactly fond of expanding a single-line log message to 3 lines, but I guess I can be convinced. Keep on pushing :-)
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.
See above.
Resolves #151
It producelog output as:
INFO: Executing operation /core-service=management/security-realm=creaperSecRealm:add
JSON format:
{
"operation" : "add",
"address" : [
{
"core-service" : "management"
},
{
"security-realm" : "creaperSecRealm"
}
]
}