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

feat: add basic metrics for sharky #2759

Merged
merged 1 commit into from
Jan 17, 2022
Merged

feat: add basic metrics for sharky #2759

merged 1 commit into from
Jan 17, 2022

Conversation

mrekucci
Copy link
Contributor

@mrekucci mrekucci commented Jan 13, 2022

Introduces basic metrics for sharky as read, write counters and their error counters, shard size counter.


This change is Reviewable

Copy link
Member

@acud acud left a comment

Choose a reason for hiding this comment

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

nice! I have some suggestions

}
return sh, nil
store.metrics.ShardSize.Set(float64(len(store.shards)))
Copy link
Member

Choose a reason for hiding this comment

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

just to mention, this is not shard size but number of shard. shard size would be the number of the items in the shard, which would be the filesize/datasize and should be updated when the shard grows

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@mrekucci mrekucci requested a review from zelig January 13, 2022 15:28
@mrekucci mrekucci added the ready for review The PR is ready to be reviewed label Jan 13, 2022
@acud
Copy link
Member

acud commented Jan 13, 2022

also missing - you need to register the metrics in the node.go file somehow (either localstore should throw the collector when being called from node.go, or leak the registration to node.go

@mrekucci
Copy link
Contributor Author

also missing - you need to register the metrics in the node.go file somehow (either localstore should throw the collector when being called from node.go, or leak the registration to node.go

Yes, but that would be done in the #2682 PR, since the sharky is instantiated there.

@mrekucci mrekucci requested a review from aloknerurkar January 13, 2022 15:35
Copy link
Member

@zelig zelig left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @acud and @aloknerurkar)


pkg/sharky/store.go, line 68 at r1 (raw file):

Previously, mrekucci wrote…

Done.

this is simply shardCnt variable

@aloknerurkar aloknerurkar force-pushed the sharky-pkg branch 2 times, most recently from 060da23 to 26f14b7 Compare January 17, 2022 08:11
@mrekucci mrekucci requested a review from acud January 17, 2022 09:54
Copy link
Contributor Author

@mrekucci mrekucci left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @acud and @aloknerurkar)


pkg/sharky/store.go, line 68 at r1 (raw file):

Previously, zelig (Viktor Trón) wrote…

this is simply shardCnt variable

Done.

@mrekucci mrekucci merged commit 8987e10 into sharky-pkg Jan 17, 2022
@mrekucci mrekucci deleted the sharky-metrics branch January 17, 2022 10:29
acud pushed a commit that referenced this pull request Jan 24, 2022
aloknerurkar pushed a commit that referenced this pull request Jan 27, 2022
aloknerurkar pushed a commit that referenced this pull request Feb 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pull-request ready for review The PR is ready to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants