-
Notifications
You must be signed in to change notification settings - Fork 32
WIP, auditloganalyzer: implement a basic audit log analyzer #1016
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
base: main
Are you sure you want to change the base?
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1016 +/- ##
=======================================
Coverage ? 66.93%
=======================================
Files ? 143
Lines ? 14503
Branches ? 0
=======================================
Hits ? 9708
Misses ? 4114
Partials ? 681
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
/hold We might add more functions to it. |
6e27865 to
49f6092
Compare
cmd/auditloganalyzer/main.go
Outdated
| record := []string{ | ||
| sql, | ||
| fmt.Sprintf("%d", group.ExecutionCount), | ||
| group.TotalCostTime.String(), |
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.
Why not output a number? It's easier to order and calculate avg.
| w := csv.NewWriter(f) | ||
| for sql, group := range result { | ||
| record := []string{ | ||
| sql, |
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.
Also output the digests so that it's easier to filter a specific SQL.
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.
And stmt type
4a19a43 to
2083882
Compare
Signed-off-by: Yang Keao <yangkeao@chunibyo.icu>
| ExecutionCount int | ||
| TotalCostTime time.Duration | ||
| TotalAffectedRows int64 | ||
| StmtTypes stmtTypesSet |
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.
Why multiple statement types?
| if err != nil { | ||
| return result, errors.Errorf("parsing cost time failed: %s", costTimeStr) | ||
| } | ||
| costTime = time.Duration(millis) * (time.Millisecond) |
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.
No need to convert int to Duration and then convert back
| connInfo.lastCmd.PreparedStmt = sql | ||
| } | ||
| connInfo.lastCmd.StmtType = kvs[auditPluginKeyStmtType] | ||
| connInfo.lastCmd.kvs = kvs |
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.
kvs is cleared before each loop, so saving kvs in it is wrong.
What problem does this PR solve?
Issue Number: close #1014
Problem Summary:
What is changed and how it works:
Check List
Tests
I used the following script to fetch analyze result for upstream and downstream cluster:
Run it with
The
100dayis the offset between upstream and downstream. The10minis the duration.Notable changes
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.