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

copy/paste towards [4352] SMB Server Shares #501

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

zeph
Copy link

@zeph zeph commented Apr 10, 2020

  • violent copy/paste of metrics
  • long but readable names
  • copy pasting in the for loop
  • 1st round of copy/paste errors
  • we shall have them all
  • Counter & Gauge assigned correctly
  • naming
  • focus on the OpenFiles
  • our own white/black Lists
  • just a word
  • we go nowhere with the wrong class mapping
  • that was not a counter

violent copy/paste of metrics

long but readable names

copy pasting in the for loop

1st round of copy/paste errors

we shall have them all

Counter & Gauge assigned correctly

naming

focus on the OpenFiles

our own white/black Lists

just a word

we go nowhere with the wrong class mapping

that was not a counter
Copy link
Collaborator

@carlpett carlpett left a comment

Hi @zeph,
Thanks for your contribution!
Some comments inline, would also like some documentation and a mention in the main README :)


ch <- prometheus.MustNewConstMetric(
c.TotalFileOpenCount,
prometheus.GaugeValue,
Copy link
Collaborator

@carlpett carlpett Apr 13, 2020

Choose a reason for hiding this comment

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

This would be a counter, right?

Copy link
Author

@zeph zeph Apr 14, 2020

Choose a reason for hiding this comment

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

not according to the dump http endpoint from the perflib tool

)

func init() {
registerCollector("smb_server_shares", NewSmbServerSharesCollector, "SMB Server Shares")
Copy link
Collaborator

@carlpett carlpett Apr 13, 2020

Choose a reason for hiding this comment

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

I think maybe we'd like to keep other SMB-related metrics in the same collector, so generalise this to just being the smb collector
(Unless there's something I'm missing which makes that a bad idea)

Copy link
Author

@zeph zeph Apr 14, 2020

Choose a reason for hiding this comment

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

sure, I just reflected the naming structure I saw so far
...I'll change it to smb

nil,
),
TotalFileOpenCount: prometheus.NewDesc(
prometheus.BuildFQName(Namespace, subsystem, "file_open_count"),
Copy link
Collaborator

@carlpett carlpett Apr 13, 2020

Choose a reason for hiding this comment

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

This should have a _total suffix

Copy link
Author

@zeph zeph Apr 14, 2020

Choose a reason for hiding this comment

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

ack

nil,
),
FilesOpenedPerSec: prometheus.NewDesc(
prometheus.BuildFQName(Namespace, subsystem, "files_opened_total"),
Copy link
Collaborator

@carlpett carlpett Apr 13, 2020

Choose a reason for hiding this comment

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

Isn't this the same as the above metric?

Copy link
Author

@zeph zeph Apr 14, 2020

Choose a reason for hiding this comment

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

hah, no idea... I did grab what I could out of perflib

// :9432/dump?query=4352#4352
type SmbServerShares struct {
Name string
/*
Copy link
Collaborator

@carlpett carlpett Apr 13, 2020

Choose a reason for hiding this comment

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

Do you intend to add the commented out metrics later, or are they documentation for what else exists?

Copy link
Author

@zeph zeph Apr 14, 2020

Choose a reason for hiding this comment

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

i wanted to implement ALL i could see from perflib, but then I was running out of time
...this code was developed over the weekend during an outage at a customer as a tool to triage

return nil
}

// :9432/dump?query=4352#4352
Copy link
Collaborator

@carlpett carlpett Apr 13, 2020

Choose a reason for hiding this comment

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

What does this mean? :)

Copy link
Author

@zeph zeph Apr 14, 2020

Choose a reason for hiding this comment

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

it's the URL query to perflib (I had not found the microsoft documentation reference as u had in there from the copy&paste source file I used... the logic_drivers one)... I believe it is the class id in the array/list in perflib memory

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.

None yet

2 participants