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

metrics extended #9

Merged
merged 4 commits into from
Jul 7, 2024
Merged

Conversation

synarete
Copy link
Collaborator

@synarete synarete commented Jun 6, 2024

Provide extra-fine metrics information. In particular,. parse the open_files section in smbstatus --json and provide more metrics on open files.

@synarete synarete requested a review from anoopcs9 June 6, 2024 11:22
OpenedAt string `json:"opened_at"`
ShareMode SMBStatusOpenShareMode `json:"sharemode"`
AccessMask SMBStatusOpenAccessMask `json:"access_mask"`
OpLock SMBStatusOpenOpLock `json:"oplock"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you have a plan to include leases section?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, absolutely. It is part of my TODO list, but for now I want a minimal viable set of metrics. Do you think leases should be part of it or may I defer it to next cycle?

Copy link
Contributor

Choose a reason for hiding this comment

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

leases can be considered as an upgrade to oplocks. I could see oplocks(but not yet used) and thus the question. You may choose to drop oplocks or add leases alongside.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fair enough. I will add leases as part of this PR

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@anoopcs9 Added Lease entry (after OpLock). Please re-review.

Copy link
Contributor

@anoopcs9 anoopcs9 left a comment

Choose a reason for hiding this comment

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

See below for a small query.

Copy link
Contributor

@anoopcs9 anoopcs9 left a comment

Choose a reason for hiding this comment

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

lgtm, thanks.

@@ -602,3 +616,16 @@ func TestParseSMBStatusLocks(t *testing.T) {
assert.Equal(t, lock2.FileID.Inode, int64(52))
assert.Equal(t, lock2.NumPendingDeletes, 2)
}

func TestParseSMBStatusLease(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

May be add a test for oplocks too?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You are right -- I should have done so in the first place..

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

@anoopcs9 anoopcs9 left a comment

Choose a reason for hiding this comment

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

See below for a small nit.

Comment on lines 628 to 637
assert.Equal(t, lease.Handle, false)
assert.Equal(t, lease.Read, false)
assert.Equal(t, lease.Write, false)
assert.Equal(t, lease.Text, "")
Copy link
Contributor

Choose a reason for hiding this comment

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

To cover other SMBStatusOpenOpLock types:

assert.Equal(t, oplock.Batch, true)
assert.Equal(t, oplock.LevelII, false)
assert.Equal(t, oplock.Text, "BATCH")

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

@anoopcs9 anoopcs9 Jul 1, 2024

Choose a reason for hiding this comment

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

If you meant to include the additional checks in the current PR itself I don't see it in the updated version.

Add extra layer of JSON parsing for more file-grained details on
currently open-files. In particular, parse access-modes.

Signed-off-by: Shachar Sharon <[email protected]>
Provides two new metrics: number of total open-file in RO/RW modes. Uses
the detailed SMB information provided by 'open_files' section per file.

Signed-off-by: Shachar Sharon <[email protected]>
Define two distinct share counter-metrics: number of remote machines
currently using a specific share and the number of distinct shares used
by a specific remote machine (identified by IP-address).

Signed-off-by: Shachar Sharon <[email protected]>
Ignore the special value of "IPC$" when collecting and exporting shares'
metrics.

Signed-off-by: Shachar Sharon <[email protected]>
Copy link
Contributor

@anoopcs9 anoopcs9 left a comment

Choose a reason for hiding this comment

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

lgtm, thanks

@synarete synarete disabled auto-merge July 7, 2024 08:21
@synarete synarete merged commit e60bcb6 into samba-in-kubernetes:main Jul 7, 2024
7 checks passed
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.

2 participants