Skip to content

Conversation

synarete
Copy link
Collaborator

With per-share profile metrics now on Samba's vfs_ceph_new master, support this mode also in Prometheus metrics using additional label.

@synarete synarete requested a review from avanthakkar June 24, 2025 12:23
Copy link
Collaborator

@phlogistonjohn phlogistonjohn left a comment

Choose a reason for hiding this comment

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

Looking at it from largely a golang perspecitive only it looks generally good, I have a couple of minor nitpicks but I'm also ok with approving any time.

@synarete synarete force-pushed the ss-per-share-metrics branch from 0eae003 to f7b7a69 Compare June 25, 2025 06:26
Copy link

@avanthakkar avanthakkar left a comment

Choose a reason for hiding this comment

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

LGTM overall
Just one note—not directly related to this PR, but something worth tracking so we don’t forget: the netbios label is currently only present on the smb_metrics_status metric. I think we’ll eventually need this label on all SMB related metrics (including per-share), not just status. When Ceph Dashboard builds Grafana dashboards, alerts, etc.. they’ll require a unique identifier like netbios to join exposed metrics with related metadata metric (to be exposed in Ceph prometheus module), which will have this label too.

Copy link

@avanthakkar avanthakkar left a comment

Choose a reason for hiding this comment

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

Might be worth updating the README to include examples of per-share labeled metrics too?

@synarete
Copy link
Collaborator Author

LGTM overall Just one note—not directly related to this PR, but something worth tracking so we don’t forget: the netbios label is currently only present on the smb_metrics_status metric. I think we’ll eventually need this label on all SMB related metrics (including per-share), not just status. When Ceph Dashboard builds Grafana dashboards, alerts, etc.. they’ll require a unique identifier like netbios to join exposed metrics with related metadata metric (to be exposed in Ceph prometheus module), which will have this label too.

Good point. Will fix it in a follow-up PR.

@synarete
Copy link
Collaborator Author

Might be worth updating the README to include examples of per-share labeled metrics too?

Indeed. Will fix.

@synarete
Copy link
Collaborator Author

LGTM overall Just one note—not directly related to this PR, but something worth tracking so we don’t forget: the netbios label is currently only present on the smb_metrics_status metric. I think we’ll eventually need this label on all SMB related metrics (including per-share), not just status. When Ceph Dashboard builds Grafana dashboards, alerts, etc.. they’ll require a unique identifier like netbios to join exposed metrics with related metadata metric (to be exposed in Ceph prometheus module), which will have this label too.

Good point. Will fix it in a follow-up PR.

Digging a bit more into it, I realized that we should probably also add the client-ip as label. Although it is not a requirement for now, we have it now (with this PR), so why not add it? adding another commit soon, will ask for another review.

synarete added 4 commits June 25, 2025 14:29
When per-share metrics mode is enabled, smbstatus dumps fine-grained
profile counters. Parse it.

Signed-off-by: Shachar Sharon <[email protected]>
Parse per-share profile key to extract share-name and connected client
IP as a pair of strings.

Signed-off-by: Shachar Sharon <[email protected]>
When exporting per-share metrics provide net-bios name, share-name and
client-ip as labels.

Signed-off-by: Shachar Sharon <[email protected]>
Added a (subset) of additional metrics when per-share profile counters
are enabled.

Signed-off-by: Shachar Sharon <[email protected]>
@synarete synarete force-pushed the ss-per-share-metrics branch from f7b7a69 to edc14f8 Compare June 25, 2025 11:56
@synarete
Copy link
Collaborator Author

Added netbios and client as labels + updated README. @avanthakkar Please take another look.

Copy link

@avanthakkar avanthakkar left a comment

Choose a reason for hiding this comment

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

lgtm

@synarete synarete merged commit 347c08d into samba-in-kubernetes:main Jun 26, 2025
1 check passed
@synarete synarete deleted the ss-per-share-metrics branch June 26, 2025 06:11
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