-
Notifications
You must be signed in to change notification settings - Fork 430
[metric] metrics aggregation #1531
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
Conversation
70203d9 to
d536b18
Compare
swuferhong
left a comment
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. Left some comments.
|
Thanks for reviewing @swuferhong . I have adjusted the position and description. PLAT~ |
ccc3990 to
aeffd00
Compare
|
@zcoo could you rebase and resolve the conflicts? |
a8afc1c to
3d0604e
Compare
|
@wuchong Sure. Now conflicts are resolved. |
d88c76c to
434a4f4
Compare
| public static final String LOG_NUM_SEGMENTS = "numSegments"; | ||
| public static final String LOG_END_OFFSET = "endOffset"; | ||
| public static final String LOG_SIZE = "size"; | ||
| public static final String LOG_FLUSH_RATE = "flushPerSecond"; |
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.
Considering this will be moved from bucket-level to server-level, it's better to add "log" prefix to distinguish with the metric of kv. E.g., logFlushPerSecond and logFlushLatencyMs
| public static final String KV_PRE_WRITE_BUFFER_FLUSH_RATE = "preWriteBufferFlushPerSecond"; | ||
| public static final String KV_PRE_WRITE_BUFFER_FLUSH_LATENCY_MS = | ||
| "preWriteBufferFlushLatencyMs"; |
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.
It's better to name them kvFlushPerSecond and kvFlushLatencyMs to keep align with the log metrics.
|
|
||
| // for kv tablet | ||
| public static final String KV_LATEST_SNAPSHOT_SIZE = "latestSnapshotSize"; | ||
| public static final String KV_PRE_WRITE_BUFFER_TRUNCATE_AS_DUPLICATED_RATE = |
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.
We should remove the void registerMetrics(BucketMetricGroup bucketMetricGroup) of KvTablet, because we don't need to report bucket-level pre-write-buffer metrics.
|
@zcoo @swuferhong I appended a commit to address above comments, please take a look, especially the metric renaming. |
3d0604e to
39fa282
Compare
swuferhong
left a comment
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 +1. I will rebase #1548 after this pr merged.
Purpose
Linked issue: close #1525
Linked issue: close #1526
Brief change log
Tests
API and Format
Documentation