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

chore: Display token information in partition ring status page #631

Merged
merged 5 commits into from
Jan 3, 2025

Conversation

alexweav
Copy link
Contributor

@alexweav alexweav commented Jan 2, 2025

What this PR does:

The partition ring has a concept of tokens, but its tokens aren't visible in the partition ring status UI.

This PR ports the token-related affordances (count, ownership%, view all) from the ingester ring status page to the partition ring status page.

Screenshot of new columns:
2025-01-02-165849_2553x822_scrot

When "Show Tokens" at the bottom is clicked:
2025-01-02-165908_2553x819_scrot

Which issue(s) this PR fixes:

Contrib https://github.com/grafana/mimir-squad/issues/2350

Checklist

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

@alexweav alexweav marked this pull request as ready for review January 2, 2025 23:13
@@ -15,6 +15,8 @@
<th>State</th>
<th>State updated at</th>
<th>Owners</th>
<th>Tokens</th>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since we usually only show the Partition Ring page in ingest-storage mode, I chose not to add any extra toggle to show or hide these values. In contexts where these values don't make sense, the entire page is usually inaccessible.

@@ -94,6 +94,34 @@ func (m *PartitionRingDesc) partitionByToken() map[Token]int32 {
return out
}

// CountTokens returns the summed token distance of all tokens in each partition.
func (m *PartitionRingDesc) countTokens() map[int32]int64 {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Chose this name for consistency with its corresponding operation on the instance ring:

func (r *Desc) CountTokens() map[string]int64 {

Copy link
Contributor

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

Nice work, thanks!

@alexweav
Copy link
Contributor Author

alexweav commented Jan 3, 2025

I noticed in the second screenshot the header was misleading, it reads Instance 0 when it would be better to read Partition 0.
2025-01-03-095825_95x78_scrot

Pushed a commit changing that. It's tiny, so i don't think that warrants resetting reviews. I'll leave this open for a bit longer in case of any objections, then merge.

@alexweav alexweav merged commit b54518d into main Jan 3, 2025
5 checks passed
@alexweav alexweav deleted the alexweav/show-tokens-partition-ring branch January 3, 2025 19:25
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