-
Notifications
You must be signed in to change notification settings - Fork 5
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
Add audit json logging design page #14
Conversation
ca330d3
to
deb9db7
Compare
Friendly reminder this needs to be reviewed, thanks! |
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.
LGTM
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.
As discussed in the comments, if there are plans to make a date format configurable, I think it will be nice to describe the "configuration" part here, too.
It's a minor issue, though. The rest, LGTM.
Sorry I haven't pushed my recent changes. I'll do that shortly |
deb9db7
to
2ef51f2
Compare
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.
Looks good
client: IP_ADDRESS, | ||
conn_id: ####, | ||
op_id: ####, | ||
result: ##, |
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 wonder if the changetype is really usefull (it is a bit redundant with the operation parameter field name )
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 wonder if the changetype is really usefull (it is a bit redundant with the operation parameter field name )
Well I thought it might make parsing "easier" as you wouldn't have to check all the operation keys to see if they are set to determine what the operation is, but I agree it is redundant. I removed it.
I did some more JSON object cleanup around the modify operation (removed redundant "delete_value" and will just use "value" for all op types).
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 wonder if the changetype is really usefull (it is a bit redundant with the operation parameter field name )
Well I thought it might make parsing "easier" as you wouldn't have to check all the operation keys to see if they are set to determine what the operation is, but I agree it is redundant. I removed it.
Ok I know I just removed the changetype key, but I'm having second thoughts about it. I think there are benefits to making it easier to find the op key (especially for a parser writer). Thoughts?
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.
Can you describe a case where having the changetype would really help ?
IMHO most application should use a json decoder library to avoid some nasty surprise with escaped characters
and such libraries provide way access/check the fields, (and checking if the field exist is always a good idea for robustness point of view)
BTW in python using if 'modify' in parsed_entry_log:
is as simple as if parsed_entry_log['changetype'] == "modify':
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.
Can you describe a case where having the changetype would really help ? IMHO most application should use a json decoder library to avoid some nasty surprise with escaped characters and such libraries provide way access/check the fields, (and checking if the field exist is always a good idea for robustness point of view) BTW in python using
if 'modify' in parsed_entry_log:
is as simple asif parsed_entry_log['changetype'] == "modify':
I guess I'm looking at this from a REST perspective. When we set the "changetype" we are telling the parser the key. Otherwise the parser needs to know all the possible keys in advance. I know changetypes are not going to change, but I was thinking it would "slightly" simplify what a parser would need to do. But, this is not a REST API object so we can leave changetype out...
Reviewed by: progier389, vashirov, spichugi (Thanks!!)
2ef51f2
to
acd8dcf
Compare
Design doc created to continue discussion on a new JSON format for the audit log