[cdc-connector][mysql]Mysql add numRecordsOutBySnapshot and numRecordsOutByIncremental metrics #3456
+94
−2
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…sOutByIncremental metrics
SML0127
reviewed
Jul 12, 2024
Comment on lines
40
to
47
| public static final String IO_NUM_RECORDS_OUT_INCREMENTAL_INSERT = | ||
| ".numRecordsOutByIncrementalInsert"; | ||
|
|
||
| public static final String IO_NUM_RECORDS_OUT_INCREMENTAL_DELETE = | ||
| ".numRecordsOutByIncrementalDelete"; | ||
|
|
||
| public static final String IO_NUM_RECORDS_OUT_INCREMENTAL_UPDATE = | ||
| ".numRecordsOutByIncrementalUpdate"; |
There was a problem hiding this comment.
Thx for contirbution @ChengJie1053
Nit: How about changing INCREMENTAL to BINLOG or CHANGE_EVENT? INCREMENTAL can be understood as a Incremental Snapshot step.
There was a problem hiding this comment.
I think your suggestion is great, and I will CHANGE it to CHANGE EVENT
SML0127
reviewed
Jul 12, 2024
Comment on lines
131
to
149
| if (RecordUtils.isDataChangeRecord(element)) { | ||
| Struct value = (Struct) element.value(); | ||
| if (value != null) { | ||
| Envelope.Operation operation = | ||
| Envelope.Operation.forCode( | ||
| value.getString(Envelope.FieldName.OPERATION)); | ||
| switch (operation) { | ||
| case CREATE: | ||
| sourceReaderMetrics.numRecordsOutInsertIncrease(tableId); | ||
| break; | ||
| case UPDATE: | ||
| sourceReaderMetrics.numRecordsOutUpdateIncrease(tableId); | ||
| break; | ||
| case DELETE: | ||
| sourceReaderMetrics.numRecordsOutDeleteIncrease(tableId); | ||
| break; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
We checked whether element is data change record or not in line 101.
How about removing this condition?
Suggested change
| if (RecordUtils.isDataChangeRecord(element)) { | |
| Struct value = (Struct) element.value(); | |
| if (value != null) { | |
| Envelope.Operation operation = | |
| Envelope.Operation.forCode( | |
| value.getString(Envelope.FieldName.OPERATION)); | |
| switch (operation) { | |
| case CREATE: | |
| sourceReaderMetrics.numRecordsOutInsertIncrease(tableId); | |
| break; | |
| case UPDATE: | |
| sourceReaderMetrics.numRecordsOutUpdateIncrease(tableId); | |
| break; | |
| case DELETE: | |
| sourceReaderMetrics.numRecordsOutDeleteIncrease(tableId); | |
| break; | |
| } | |
| } | |
| } | |
| Struct value = (Struct) element.value(); | |
| if (value != null) { | |
| Envelope.Operation operation = | |
| Envelope.Operation.forCode( | |
| value.getString(Envelope.FieldName.OPERATION)); | |
| switch (operation) { | |
| case CREATE: | |
| sourceReaderMetrics.numRecordsOutInsertIncrease(tableId); | |
| break; | |
| case UPDATE: | |
| sourceReaderMetrics.numRecordsOutUpdateIncrease(tableId); | |
| break; | |
| case DELETE: | |
| sourceReaderMetrics.numRecordsOutDeleteIncrease(tableId); | |
| break; | |
| } | |
| } |
There was a problem hiding this comment.
Thank you very much for helping me review the code.
I will delete the isDataChangeRecord judgment
SML0127
suggested changes
Jul 15, 2024
Comment on lines
+72
to
+114
| public void numRecordsOutSnapshotIncrease(TableId tableId) { | ||
| Counter counter = | ||
| snapshotNumRecordsOutMap.computeIfAbsent( | ||
| tableId, | ||
| k -> | ||
| metricGroup.counter( | ||
| tableId.identifier() + IO_NUM_RECORDS_OUT_SNAPSHOT)); | ||
| counter.inc(); | ||
| } | ||
|
|
||
| public void numRecordsOutInsertIncrease(TableId tableId) { | ||
| Counter counter = | ||
| insertNumRecordsOutMap.computeIfAbsent( | ||
| tableId, | ||
| k -> | ||
| metricGroup.counter( | ||
| tableId.identifier() | ||
| + IO_NUM_RECORDS_OUT_DATA_CHANGE_EVENT_INSERT)); | ||
| counter.inc(); | ||
| } | ||
|
|
||
| public void numRecordsOutUpdateIncrease(TableId tableId) { | ||
| Counter counter = | ||
| updateNumRecordsOutMap.computeIfAbsent( | ||
| tableId, | ||
| k -> | ||
| metricGroup.counter( | ||
| tableId.identifier() | ||
| + IO_NUM_RECORDS_OUT_DATA_CHANGE_EVENT_UPDATE)); | ||
| counter.inc(); | ||
| } | ||
|
|
||
| public void numRecordsOutDeleteIncrease(TableId tableId) { | ||
| Counter counter = | ||
| deleteNumRecordsOutMap.computeIfAbsent( | ||
| tableId, | ||
| k -> | ||
| metricGroup.counter( | ||
| tableId.identifier() | ||
| + IO_NUM_RECORDS_OUT_DATA_CHANGE_EVENT_DELETE)); | ||
| counter.inc(); | ||
| } | ||
|
|
There was a problem hiding this comment.
I applied this metrics to my code, and left some comments.
- Metric names should be general.
- In your case, It generate uniqe metrics for each tables. But in monitorig tool such as grafana, it distingush tables by property such as job name in metric, not by metric name
So I suggest to removetableId.identifier()in metric name.
Suggested change
| public void numRecordsOutSnapshotIncrease(TableId tableId) { | |
| Counter counter = | |
| snapshotNumRecordsOutMap.computeIfAbsent( | |
| tableId, | |
| k -> | |
| metricGroup.counter( | |
| tableId.identifier() + IO_NUM_RECORDS_OUT_SNAPSHOT)); | |
| counter.inc(); | |
| } | |
| public void numRecordsOutInsertIncrease(TableId tableId) { | |
| Counter counter = | |
| insertNumRecordsOutMap.computeIfAbsent( | |
| tableId, | |
| k -> | |
| metricGroup.counter( | |
| tableId.identifier() | |
| + IO_NUM_RECORDS_OUT_DATA_CHANGE_EVENT_INSERT)); | |
| counter.inc(); | |
| } | |
| public void numRecordsOutUpdateIncrease(TableId tableId) { | |
| Counter counter = | |
| updateNumRecordsOutMap.computeIfAbsent( | |
| tableId, | |
| k -> | |
| metricGroup.counter( | |
| tableId.identifier() | |
| + IO_NUM_RECORDS_OUT_DATA_CHANGE_EVENT_UPDATE)); | |
| counter.inc(); | |
| } | |
| public void numRecordsOutDeleteIncrease(TableId tableId) { | |
| Counter counter = | |
| deleteNumRecordsOutMap.computeIfAbsent( | |
| tableId, | |
| k -> | |
| metricGroup.counter( | |
| tableId.identifier() | |
| + IO_NUM_RECORDS_OUT_DATA_CHANGE_EVENT_DELETE)); | |
| counter.inc(); | |
| } | |
| public void numRecordsOutSnapshotIncrease(TableId tableId) { | |
| Counter counter = | |
| snapshotNumRecordsOutMap.computeIfAbsent( | |
| tableId, | |
| k -> | |
| metricGroup.counter(IO_NUM_RECORDS_OUT_SNAPSHOT)); | |
| counter.inc(); | |
| } | |
| public void numRecordsOutInsertIncrease(TableId tableId) { | |
| Counter counter = | |
| insertNumRecordsOutMap.computeIfAbsent( | |
| tableId, | |
| k -> | |
| metricGroup.counter(IO_NUM_RECORDS_OUT_DATA_CHANGE_EVENT_INSERT)); | |
| counter.inc(); | |
| } | |
| public void numRecordsOutUpdateIncrease(TableId tableId) { | |
| Counter counter = | |
| updateNumRecordsOutMap.computeIfAbsent( | |
| tableId, | |
| k -> | |
| metricGroup.counter(IO_NUM_RECORDS_OUT_DATA_CHANGE_EVENT_UPDATE)); | |
| counter.inc(); | |
| } | |
| public void numRecordsOutDeleteIncrease(TableId tableId) { | |
| Counter counter = | |
| deleteNumRecordsOutMap.computeIfAbsent( | |
| tableId, | |
| k -> | |
| metricGroup.counter(IO_NUM_RECORDS_OUT_DATA_CHANGE_EVENT_DELETE)); | |
| counter.inc(); | |
| } |
There was a problem hiding this comment.
Ok, thanks for reviewing the code for me, but the original purpose of this metric is to see each table at a granular level, if you need to see the total, I will add other metrics
Comment on lines
+38
to
+47
| public static final String IO_NUM_RECORDS_OUT_SNAPSHOT = ".numRecordsOutBySnapshot"; | ||
|
|
||
| public static final String IO_NUM_RECORDS_OUT_DATA_CHANGE_EVENT_INSERT = | ||
| ".numRecordsOutByDataChangeEventInsert"; | ||
|
|
||
| public static final String IO_NUM_RECORDS_OUT_DATA_CHANGE_EVENT_DELETE = | ||
| ".numRecordsOutByDataChangeEventDelete"; | ||
|
|
||
| public static final String IO_NUM_RECORDS_OUT_DATA_CHANGE_EVENT_UPDATE = | ||
| ".numRecordsOutByDataChangeEventUpdate"; |
There was a problem hiding this comment.
I suggest to remove dot(.) from metric name.
Suggested change
| public static final String IO_NUM_RECORDS_OUT_SNAPSHOT = ".numRecordsOutBySnapshot"; | |
| public static final String IO_NUM_RECORDS_OUT_DATA_CHANGE_EVENT_INSERT = | |
| ".numRecordsOutByDataChangeEventInsert"; | |
| public static final String IO_NUM_RECORDS_OUT_DATA_CHANGE_EVENT_DELETE = | |
| ".numRecordsOutByDataChangeEventDelete"; | |
| public static final String IO_NUM_RECORDS_OUT_DATA_CHANGE_EVENT_UPDATE = | |
| ".numRecordsOutByDataChangeEventUpdate"; | |
| public static final String IO_NUM_RECORDS_OUT_SNAPSHOT = "numRecordsOutBySnapshot"; | |
| public static final String IO_NUM_RECORDS_OUT_DATA_CHANGE_EVENT_INSERT = | |
| "numRecordsOutByDataChangeEventInsert"; | |
| public static final String IO_NUM_RECORDS_OUT_DATA_CHANGE_EVENT_DELETE = | |
| "numRecordsOutByDataChangeEventDelete"; | |
| public static final String IO_NUM_RECORDS_OUT_DATA_CHANGE_EVENT_UPDATE = | |
| "numRecordsOutByDataChangeEventUpdate"; |
There was a problem hiding this comment.
Ok, thank you for helping me review the code, I will remove (.)
|
Seems we can't come up with a "best" metric design for everyone: Some may want a task-level unified record counter, while some may want fine-grained counters for each table. Or one may want to disable it entirely to avoid any extra cost. I wonder if we should add an option to control metric behavior? cc @ruanhang1993 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
numRecordsOutBySnapshotmetrics collects snapshot statisticsnumRecordsOutByIncrementalmetrics collects incremental statisticsnumRecordsOutByIncrementalhas three metrics, respectively isnumRecordsOutByIncrementalInsert,numRecordsOutByIncrementalUpdate,numRecordsOutByIncrementalDelete2024.7.12 change metrics name，change
numRecordsOutByIncrementalInserttonumRecordsOutByDataChangeEventInsert