-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
mysqld
system metrics, with TabletManager
rpc
#16850
mysqld
system metrics, with TabletManager
rpc
#16850
Conversation
Signed-off-by: Shlomi Noach <[email protected]>
Signed-off-by: Shlomi Noach <[email protected]>
Signed-off-by: Shlomi Noach <[email protected]>
Signed-off-by: Shlomi Noach <[email protected]>
Signed-off-by: Shlomi Noach <[email protected]>
Signed-off-by: Shlomi Noach <[email protected]>
Signed-off-by: Shlomi Noach <[email protected]>
Signed-off-by: Shlomi Noach <[email protected]>
Signed-off-by: Shlomi Noach <[email protected]>
Signed-off-by: Shlomi Noach <[email protected]>
Signed-off-by: Shlomi Noach <[email protected]>
Signed-off-by: Shlomi Noach <[email protected]>
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
cell string | ||
keyspace string | ||
shard string | ||
tabletAlias *topodatapb.TabletAlias |
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 change (and everything in this file) is a small refactor. It is not strictly required for this PR, but will be needed in part 2, so I thought I may as well do it now.
} | ||
defer closer.Close() | ||
|
||
resp, err := c.MysqlSystemMetrics(ctx, req) |
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.
A fully functional TabletManagerClient
implementation. Not used by anyone right now for 1-version backwards support. This is what the throttler will use in the future.
@@ -275,6 +275,18 @@ func (tm *TabletManager) ExecuteFetchAsApp(ctx context.Context, req *tabletmanag | |||
return sqltypes.ResultToProto3(result), err | |||
} | |||
|
|||
// MysqlSystemMetrics gets system metrics from the MySQL deamon | |||
func (tm *TabletManager) MysqlSystemMetrics(ctx context.Context, req *tabletmanagerdatapb.MysqlSystemMetricsRequest) (*tabletmanagerdatapb.MysqlSystemMetricsResponse, error) { | |||
mysqlResp, err := tm.MysqlDaemon.SystemMetrics(ctx, tm.Cnf) |
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 is the bridge between TabletManager
and MysqlDaemon
. It is fully functional. But right now, no one will be calling this function, so there's no backwards-compatibility issue.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16850 +/- ##
=======================================
Coverage 69.43% 69.43%
=======================================
Files 1571 1571
Lines 203086 203173 +87
=======================================
+ Hits 141013 141079 +66
- Misses 62073 62094 +21 ☔ View full report in Codecov by Sentry. |
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! I only had some minor suggestions / questions. Let me know what you think and I'll come back to this quickly.
Signed-off-by: Shlomi Noach <[email protected]>
Signed-off-by: Shlomi Noach <[email protected]>
Signed-off-by: Shlomi Noach <[email protected]>
Signed-off-by: Shlomi Noach <[email protected]>
Signed-off-by: Shlomi Noach <[email protected]>
Signed-off-by: Shlomi Noach <[email protected]>
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, @shlomi-noach ! ❤️
Description
Addresses #16822. This is part of the multi-metrics throttler project.
Part 1, to be included in
v21
:mysqlctl.HostMetrics
RPC, which returns a map[metric-name]metric-value, e.g.metrics["loadavg"]={...Value: 1.234}
tabletmanager.MysqlHostMetrics
, which is an access point to the above.TabletManager
is the entity that holds aMysqlDaemon
, thus is the only one able to callmysqlctl.HostMetrics
RPC.TabletManagerClient
supportsMysqlHostMetrics()
function, so that anyone can query a given tablet for its MySQL OS metrics.This is all wired in this PR, ie if you call
tmclient. TabletManagerClient
, you get the actual OS metrics read by MysqlDaemon.However, for 1-version backwards compatibility, this code path is not used anywhere.
We wish to merge this in
v21
, and immediately follow up with a PR (designed forv22
) where the tablet throttler collects these metrics. It will do so sparsely.Related Issue(s)
Checklist
Deployment Notes