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 metrics for shuffle sharding #4432

Closed

Conversation

ac1214
Copy link
Contributor

@ac1214 ac1214 commented Aug 18, 2021

What this PR does:

Adds a metric for tracking the number of compactions which are planned but yet to be completed. Depends on #4357. Once #4357 is merged the diff for this PR will be reduced.

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

Copy link
Contributor

@bboreham bboreham left a comment

Choose a reason for hiding this comment

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

Code seems broadly ok, but I have a question about the metric value if scraped at a bad time.

@@ -109,6 +115,7 @@ func (g *ShuffleShardingGrouper) Groups(blocks map[ulid.ULID]*metadata.Meta) (re
// TODO: Use the group's hash to determine whether a compactor should be responsible for compacting that group
groupHash := hashGroup(group.blocks[0].Thanos.Labels["__org_id__"], group.rangeStart, group.rangeEnd)

g.remainingPlannedCompactions.Inc()
Copy link
Contributor

Choose a reason for hiding this comment

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

If the metric is scraped in the middle of this function, it looks like it will have some value between 0 and the final value. Is that useful?
Would it be better to count up then Set() once?

@stale
Copy link

stale bot commented Dec 14, 2021

This issue has been automatically marked as stale because it has not had any activity in the past 60 days. It will be closed in 15 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Dec 14, 2021
@bboreham
Copy link
Contributor

@ac1214 will you be coming back to this?

@stale stale bot removed the stale label Dec 16, 2021
@ac1214
Copy link
Contributor Author

ac1214 commented Dec 17, 2021

@ac1214 will you be coming back to this?

I haven't had much time to work on these changes, but I believe that @roystchiang is taking over this work.

There is some discussion about this work in #4433

ac1214 and others added 4 commits January 14, 2022 17:10
Signed-off-by: Albert <ac1214@users.noreply.github.com>
Signed-off-by: Albert <ac1214@users.noreply.github.com>
Signed-off-by: Albert <ac1214@users.noreply.github.com>
Signed-off-by: Alvin Lin <alvinlin@amazon.com>
@alvinlin123
Copy link
Member

I don't think we need this PR, since it looks like #4433 includes changes in this PR. I will double check in more depth before closing this PR.

@ac1214
Copy link
Contributor Author

ac1214 commented Jan 15, 2022

I don't think we need this PR, since it looks like #4433 includes changes in this PR. I will double check in more depth before closing this PR.

#4433 will contain or overwrite all of the changes in this PR, so this one should be okay to close. This PR was originally created to split up the code to make reviewing easier.

@alvinlin123
Copy link
Member

Yes, this PR is a subset of #4433. We will just use #4433. Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants