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

cmd/scollector: Replace external AD collector with built in collector #1074

Merged
merged 1 commit into from
Jun 18, 2015

Conversation

gbrayut
Copy link
Contributor

@gbrayut gbrayut commented Jun 17, 2015

This replaces the PS1 script that IT wrote for monitoring AD replication with a built in collector. The old metric names were activedirectory.replication.age and activedirectory.replication.failures, but I had to rename them due to how the WMI classes present the data (all sources for a host, not all destinations).

I have it running as a second instance on devbosun right now. See http://goo.gl/HY4rjr and http://goo.gl/jQIXyB for examples (remove host filter to see or-dc issues due to move)

Review on Reviewable

@captncraig
Copy link
Contributor

Review status: 0 of 2 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.


cmd/scollector/collectors/activedirectory_windows.go, line 14 [r1] (raw file):
delete commented code.


cmd/scollector/collectors/activedirectory_windows.go, line 39 [r1] (raw file):
The name here indicates this is a timestamp, but it is actually a duration. Perhaps either rename to "sync_age" or else just emit the raw timestamp. Which are we doing more often, absolute or relative times? We should try to be consistent.


cmd/scollector/collectors/wmi_windows.go, line 1 [r1] (raw file):
I believe there is a gofmt isssue in this file.


cmd/scollector/collectors/wmi_windows.go, line 42 [r1] (raw file):
is the time.Unix really necessary? Why not just 0? If this is necessary make it a var and use it in all three error returns.


Comments from the review on Reviewable.io

@gbrayut
Copy link
Contributor Author

gbrayut commented Jun 17, 2015

Review status: 0 of 2 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.


cmd/scollector/collectors/activedirectory_windows.go, line 14 [r1] (raw file):
I commented it out for testing... Forgot to uncomment it. Replicatikn only occurs every 15-60mins so no need for high resolution data.


cmd/scollector/collectors/activedirectory_windows.go, line 39 [r1] (raw file):
Previously was just age... So sync_age works for me. Time since is preferred as it is better for alerts. We do the same thing for iis app pools and server uptime


cmd/scollector/collectors/wmi_windows.go, line 1 [r1] (raw file):
Travic-ci agrees with you. Will fix


cmd/scollector/collectors/wmi_windows.go, line 42 [r1] (raw file):
It was the easiest way i could find to get the epoch date. Am open to alternatives but cant return nil unless changed to pointer


Comments from the review on Reviewable.io

@captncraig
Copy link
Contributor

Reviewed 2 of 2 files at r2.
Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from the review on Reviewable.io

gbrayut added a commit that referenced this pull request Jun 18, 2015
cmd/scollector: Replace external AD collector with built in collector
@gbrayut gbrayut merged commit a1a1e70 into master Jun 18, 2015
@gbrayut gbrayut deleted the activedirectory branch June 18, 2015 03:14
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