percona
AUDITLOG - use log_timestamps
Use log_timestamps global variable to format audit log timestamp record. This will make it easier to analyse when we have servers in non-UTC and we use log_timestamps = SYSTEM Reuse existing mysql timestamps functions, this will also add usec to the audit.
I don't see any problem caused by this code, the failed builds look like they are failing by non-related issues in the pipeline
First off, thank you for your contribution!
I do have a concern of changing user interface, even though the behavior would be controlled by log_timestamp, I believe it is more appropriate to add a new controlling variable like audit_log_timestamps with the default behavior being the current. We must be 100% sure not to break any existing parsers or consumers of this data when a simple minor version upgrade is performed, no exceptions can be made for this.
There is/are also no accompanying mtr test case/s to cover the default/existing behavior and new behaviors which are required for each change set.
This PR also seems to have many spurious unrelated formatting changes which should not be there.
Thanks, please let us know if you need any further assistance!
-- George O. Lorch III Director of Server Engineering, Percona Server for MySQL, Percona XtraDB Cluster, and Percona XtraBackup
Hi George,
Fair point. I have added a dynamic plugin variable called audit_log_timestamps with valid values of UTC, SYSTEM and default value of UTC.
I have added a basic mtr test case for this variable, copied over the test case for log_timestamps. I'm not sure that we can create a test case to validate the timestamp in the audit output.
I formatted the file I'm modified using clang-format and the standard options defined in MySQL 8.0. Those spurious changes are caused by the file not complying with MySQL coding standard in the first place.
Hello again @fmbiete, Thank you very much for the changes. Typically we require 1 commit per PR (with exceptions). So you would need to squash these two together (or we can). We typically do not allow blanket formatting changes as it causes merge conflicts when we merge from Oracle MySQL. In this case, we are only within Percona specific code so we can accept blanket file reformattig, but, it would be required to be in a commit of its own so that formatting and functional changes are in two different commits in the same PR. Formatting changes tend to cause extra hassle with git blame and regression tracing. So, if I could ask you to do two more small things before I pass this on to one of our developers for full review, 1) introduce one commit that first re-formats the code with NO functional changes, 2) squash the functional changes, including mtr test into a single second commit. After that we can review it, send it through our ci/cd farm, create proper JIRA issue and edit commit message to comply with our automation requirements, etc...
@percona-ysorokin ^^
-- George O. Lorch III Director of Server Engineering, Percona Server for MySQL, Percona XtraDB Cluster, and Percona XtraBackup
ok, first commit with clang-format file, second commit with functional changes.
Excellent @fmbiete, thank you for this. I will run this through our internal CI/CD system and get a further review done. We will need to create JIRA issues for this and for documentation updates as well as will need to touch up commit messages so this will get ported over into another pull request but you will retain 'Author' status. Thank you again for your contribution.
-- George O. Lorch III Director of Server Engineering, Percona Server for MySQL, Percona XtraDB Cluster, and Percona XtraBackup
Hello again @fmbiete. I missed the part where you added usec to all timestamps. this is also not going to be allowed by default as it would/could break existing parsers and it in fact broke the full audit_log test suite as the audit result verifier code here https://github.com/percona/percona-server/blob/1782921be2685530a2922e96c6bf97ec24a4d188/plugin/audit_log/tests/mtr/audit_log_echo.inc#L37 and here https://github.com/percona/percona-server/blob/1782921be2685530a2922e96c6bf97ec24a4d188/plugin/audit_log/tests/mtr/audit_log_echo.inc#L38 are expecting the existing format. So in order t accept the usec portion of the change, it will need to be yet another audit_ variable that explicitly enables it. This is why MySQL has so many seemingly silly variables, but, it is the world we live in to ensure than a minor version upgrade does not break an existing deployment. If the usec is important to you, then I would suggest a new audit_log_timestamps_include_usec variable or something similar, but, then it would also require further changes to the mtr suite/results to account for both on and off. If usec is not important to you, I can simply remove it from my working copy of your work and retest everything. Thanks!
-- George O. Lorch III Director of Server Engineering, Percona Server for MySQL, Percona XtraDB Cluster, and Percona XtraBackup
Hi @georgelorchpercona , we don't need usec, it's just a nice thing to have. They got introduced because I was using MySQL default makedate functions... If Oracle MySQL Enterprise audit plugin doesn't support them, feel free to drop them. Thank you