Skip to content
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

Add more labels to VReplicationSecondsBehindMaster #6279

Conversation

zmagg
Copy link
Contributor

@zmagg zmagg commented Jun 7, 2020

Description

Adding more labels to VReplicationSecondsBehindMaster.

@sougou I know we talked about how you're refactoring tabletmanager, but after taking a closer look at this metric, it was pretty simple to emit the extra labels now. 😁 Let me know if this gets in the way of the refactor somehow.

Testing

Tested in a dev env, and the emitted metrics now look like:

(debug/vars version)

"VReplicationSecondsBehindMaster": {"test_keyspace.-80.test_workflow.3": 0, "test_keyspace.80-.test_workflow.4": 0},

(/metrics version)

vttablet_v_replication_seconds_behind_master{counts="3",source_keyspace="test_keyspace",source_shard="-80",workflow="test_workflow"} 0

zmagg added 2 commits June 6, 2020 19:26
Signed-off-by: Maggie Zhou <mzhou@slack-corp.com>
Signed-off-by: Maggie Zhou <mzhou@slack-corp.com>
@zmagg zmagg requested a review from sougou as a code owner June 7, 2020 02:34
Copy link
Contributor

@sougou sougou left a comment

Choose a reason for hiding this comment

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

I misunderstood what you were trying to tell me (confused with vttablet labels). This is a great idea, and shouldn't interfere with labels. LMK if you want me to merge this as is or if you plan to do more work.

@sougou
Copy link
Contributor

sougou commented Jun 7, 2020

One suggestion: Since id is the most specific of the labels, I'd go with workflow.keyspace.shard.id.

Signed-off-by: Maggie Zhou <mzhou@slack-corp.com>
@zmagg
Copy link
Contributor Author

zmagg commented Jun 7, 2020

Oh good idea, updated.

Fine to merge as is now, I just wanted to check to make sure the labels were emitted as expected in dev where we have a stream running, and they are! Updated the description accordingly.

@sougou sougou merged commit 8daaaac into vitessio:master Jun 7, 2020
@deepthi deepthi added this to the v7.0 milestone Jul 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants