Skip to content
This repository has been archived by the owner on Nov 24, 2023. It is now read-only.

syncer, loader: add more metrics #1778

Merged
merged 45 commits into from
Aug 27, 2021
Merged

syncer, loader: add more metrics #1778

merged 45 commits into from
Aug 27, 2021

Conversation

Ehco1996
Copy link
Contributor

@Ehco1996 Ehco1996 commented Jun 16, 2021 •
edited
Loading

What problem does this PR solve?

close #1451

What is changed and how it works?

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)

Code changes

  • Has interface methods change

Side effects

  • Possible performance regression
  • Increased code complexity

Related changes

  • Need to cherry-pick to the release branch
  • Need to update the documentation
  • Need to update the dm/dm-ansible
  • Need to be included in the release note

@ti-chi-bot
Copy link
Member

ti-chi-bot commented Jun 16, 2021 •
edited
Loading

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • lance6716
  • lichunzhu

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

@ti-chi-bot ti-chi-bot requested review from GMHDBJD and lance6716 June 16, 2021 08:44
@Ehco1996 Ehco1996 force-pushed the more-metric branch 2 times, most recently from e707050 to b440fe9 Compare June 17, 2021 02:36
@Ehco1996 Ehco1996 force-pushed the more-metric branch 2 times, most recently from 3125b7a to fb21efe Compare June 17, 2021 09:59
@Ehco1996 Ehco1996 mentioned this pull request Jun 21, 2021
Closed
@ti-chi-bot ti-chi-bot added size/XL and removed size/L labels Jun 24, 2021
@Ehco1996 Ehco1996 force-pushed the more-metric branch 2 times, most recently from ceaa828 to 90b2505 Compare June 25, 2021 06:22
lichunzhu
lichunzhu reviewed Aug 26, 2021
View reviewed changes
Copy link
Contributor

@lichunzhu lichunzhu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Besides, can we add some metrics for server info like UpTime, Goroutines and add more graphs for dumpling progress?
We can do these things in next PR

dm/config/subtask.go Outdated Show resolved Hide resolved
syncer/dbconn/db.go Show resolved Hide resolved
syncer/heartbeat.go Show resolved Hide resolved
syncer/syncer.go Outdated
tpCnt := make(map[opType]int64)
// db_schema->db_table->opType
tpCnt := make(map[string]map[string]map[opType]int64)
queueID := strings.Split(workerLagKey, "lag_")[0]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use TrimPrefix here?

syncer/syncer.go Outdated Show resolved Hide resolved
syncer/syncer.go Outdated Show resolved Hide resolved
tests/metrics/run.sh Show resolved Hide resolved
syncer/metrics/metrics.go Outdated Show resolved Hide resolved
@ti-chi-bot ti-chi-bot requested a review from lance6716 August 26, 2021 06:35
@ti-chi-bot
Copy link
Member

@lichunzhu: GitHub didn't allow me to request PR reviews from the following users: WDYT.

Note that only pingcap members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

I think current and historic data both matter 🤔
/cc @lance6716 WDYT

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

lichunzhu
lichunzhu reviewed Aug 26, 2021
View reviewed changes
Copy link
Contributor

@lichunzhu lichunzhu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rest LGTM

dm/worker/worker.go Outdated Show resolved Hide resolved
lichunzhu
lichunzhu approved these changes Aug 26, 2021
View reviewed changes
Copy link
Contributor

@lichunzhu lichunzhu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ti-chi-bot ti-chi-bot added status/LGT2 Two reviewers already commented LGTM, ready for merge and removed status/LGT1 One reviewer already commented LGTM labels Aug 26, 2021
@Ehco1996
Copy link
Contributor Author

/merge

@ti-chi-bot
Copy link
Member

This pull request has been accepted and is ready to merge.

Commit hash: e23edc9

@ti-chi-bot ti-chi-bot merged commit 8325f1b into pingcap:master Aug 27, 2021
ti-chi-bot pushed a commit to ti-chi-bot/dm that referenced this pull request Aug 27, 2021
@Ehco1996 @ti-chi-bot
syncer, loader: add more metrics (pingcap#1778)
c357e8f
@ti-chi-bot ti-chi-bot mentioned this pull request Aug 27, 2021
Merged
@ti-chi-bot
Copy link
Member

In response to a cherrypick label: new pull request created: #2057.

ti-chi-bot added a commit that referenced this pull request Aug 27, 2021
@ti-chi-bot
syncer, loader: add more metrics (#1778) (#2057)
a51905d
@ti-chi-bot ti-chi-bot mentioned this pull request Sep 2, 2021
Merged
8 tasks
@Ehco1996 Ehco1996 mentioned this pull request Sep 9, 2021
Merged
@ti-chi-bot ti-chi-bot mentioned this pull request Sep 9, 2021
Merged
@lance6716 lance6716 mentioned this pull request Dec 28, 2021
Closed
Ehco1996
Ehco1996 commented Dec 28, 2021
View reviewed changes
loader/loader.go
@@ -221,7 +224,13 @@ func (w *Worker) run(ctx context.Context, fileJobQueue chan *fileJob, runFatalCh
hasError = true
continue
}
w.loader.finishedDataSize.Add(job.offset - job.lastOffset)
Copy link
Contributor Author

@Ehco1996 Ehco1996 Dec 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do this change?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@lichunzhu lichunzhu lichunzhu approved these changes

@GMHDBJD GMHDBJD Awaiting requested review from GMHDBJD

@lance6716 lance6716 Awaiting requested review from lance6716

Assignees
No one assigned
Labels
needs-cherry-pick-release-2.0 This PR should be cherry-picked to release-2.0. Remove this label after cherry-picked to release-2.0 needs-update-docs Should update docs after this PR is merged. Remove this label once the docs are updated needs-update-release-note This PR should be added into release notes. Remove this label once the release notes are updated size/XXL status/can-merge status/LGT2 Two reviewers already commented LGTM, ready for merge status/PTAL This PR is ready for review. Add this label back after committing new changes
Projects
None yet
Milestone
v2.1.0
Development

Successfully merging this pull request may close these issues.

improve metrics to help better debug
6 participants
@Ehco1996 @ti-chi-bot @lance6716 @lichunzhu @glorv @GMHDBJD