Db/system both rebase by derekwbrown · Pull Request #351 · DataDog/datadog-agent

derekwbrown · 2017-06-27T02:00:00Z

What does this PR do?

Implements the IO/disk check for windows. This version uses WMI, to match agent5. Have an eventual PR that uses PDH.

Moves platform specific code to conditionally compiled rather than runtime evaluation.

Note that this does NOT detect insertion of drives, at least as tested. Is same behavior as agent5.

(Tested using virtualbox, and using Disk Manager to mount & unmount drives).

…compatible.

…up disk changes, at least in the manner tested.

date

truthbk

This looks absolutely fine overall. Left a few comments/questions, small things that might make it a little more idiomatic or nitpicks. Also left a couple of notes to myself - as this PR is a bit of a joint effort..

truthbk · 2017-07-04T09:33:02Z

 	Datadog.SetDefault("check_runners", int64(4))
 	Datadog.SetDefault("forwarder_timeout", 20)
 	// Dogstatsd
+	Datadog.SetDefault("device_blacklist_re", "")


We decided device_blacklist_re was meant to be a check level (init_conf as I implemented it) configuration element. So we should remove it from here.

truthbk · 2017-07-04T09:40:09Z

    if !ENV["USE_SYSTEM_PY"]
      env["PKG_CONFIG_LIBDIR"] = "#{PKG_CONFIG_LIBDIR}"
-      libdir = `PKG_CONFIG_LIBDIR="#{PKG_CONFIG_LIBDIR}" pkg-config --variable=libdir python-2.7`.strip
+      ENV["PKG_CONFIG_LIBDIR"] = "#{PKG_CONFIG_LIBDIR}"


Do we really need to add PKG_CONFIG_LIBDIR to the global environment? I take it prepending the environment variable won't work on windows...

yes, it doesn't work on windows. This solution is definitely sub-optimal, and I'd be open to anything else, but the existing rakefile won't build on windows.

truthbk · 2017-07-04T09:42:17Z

+	core.RegisterCheck("iowin", wmiioFactory)
+}
+
+func wmiioFactory() check.Check {


nit: should probably be wmiIoFactory

Do we really need two different factories? They both do the same thing:

func ioFactory() check.Check { c := &IOCheck{} return c }

Similarly, I think we can probably have a single init block for the two implementations (and leave the initialization of hz in its own separate init block):

init() { core.RegisterCheck("io", ioFactory) }

truthbk · 2017-07-04T09:48:01Z

+}
+
+func init() {
+	core.RegisterCheck("iowin", wmiioFactory)


nit: the check can probably be called io - not sure we need to differentiate between platforms in the check name.

You're right. This is supposed to be "io". But, I have an associated problem on windows which is that when I call it "io", it fails to load, because the python loader fails loading the module "io"; this was supposed to be a temporary workaround. Will fix it.

truthbk · 2017-07-04T09:50:00Z

+		return err
+	}
+
+	blacklistRe := config.Datadog.GetString("device_blacklist_re")


As mentioned in pkg/config we decided to not have this as a global configuration element, so you can probably remove this entire block. We already initialize the regexp expression in commonConfigure().

truthbk · 2017-07-04T09:52:04Z

+
+	drivebuf := make([]byte, 256)
+
+	r, _, err := ProcGetLogicalDriveStringsW.Call(


maybe a comment here with a link to: https://msdn.microsoft.com/en-us/library/windows/desktop/aa364975(v=vs.85).aspx ?

truthbk · 2017-07-04T10:02:30Z

+	}
+	for _, v := range drivebuf {
+		// between 'A' & 'Z'
+		if v >= 65 && v <= 90 {


Any scenario on windows where the drive would not be a single letter? Network drives, for instance? Maybe something like this might be safer?

drives := bytes.Split(drivebuff, []byte{0}) for _, v := range drives { drive := string(v) // do whatever we need to do with the drive string. }

truthbk · 2017-07-04T10:09:26Z

+		tagbuff.WriteString("device:")
+		tagbuff.WriteString(drive)
+		tags := []string{tagbuff.String()}
+		if len(c.drivemap[d.Name].Name) != 0 {


Are we just checking if the value was already in the drivemap? The more idiomatic way of doing this would be:

if prev, ok := m["route"]; ok { metrics, err := computeValue(prev, &d) }

truthbk · 2017-07-04T10:15:10Z

+	err := c.commonConfigure(data, initConfig)
+	return err
+}
+func (c *IOCheck) nixIO() error {


Note to self (and totally on me): We may be assuming way too many things about how this will work out on Darwin...

truthbk · 2017-07-04T10:18:39Z

+}
+
+// Run executes the check
+func (c *IOCheck) Run() error {


Although right now this it's probably unnecessary to have Run() just call nixIO(), it might actually come in handy in case we need to do different things for Darwin or other GOOS variants.

My intent was, once we get to doing darwin, it should be split out at compile time (the way I did with windows), not at runtime. That's more a preference thing, though one we probably ought to standardize on one way or the other.

* adds upstart file to centos6 * adds upstart file to rpm * changes conditional * enables alternative python

truthbk

Logic looks good to me. There's a few things I commented on, but I'll let you decide if/when you want to address those.

truthbk · 2017-07-12T08:19:45Z


 variables:
-  DOCKER_REGISTRY: gitlab.datad0g.com:4567
+  DOCKER_REGISTRY: 727006795293.dkr.ecr.us-east-1.amazonaws.com


Could this be a problem once the repo goes public? Or is it only accessible from within a VPC or whatever?

I took this from master, to get it to pass ci: https://github.com/DataDog/datadog-agent/blob/master/.gitlab-ci.yml#L9. TBH, I'm not sure of the impact

truthbk · 2017-07-12T08:27:43Z

 # Linux
 if linux?
-  if debian?
+  if debian? || redhat?


If we don't do anything different for redhat vs debian we can maybe remove this if? Or are you accounting here for SuSE?

This is also on master; I had to pull in this change to get CI to function
2a22841#diff-6510f031d77235eab2fc2392e0be89abR110

truthbk · 2017-07-12T08:30:55Z


-    if debian?
+    mkdir "/etc/init/"
+    if debian? || redhat?


Similarly, do we need this if condition, we're doing the same thing for both major distros... Is it because of SuSE?

ditto here (same change as above)

truthbk · 2017-07-12T08:41:35Z

+			retstrings = append(retstrings, "")
+			continue
+		}
+		retstrings[rsindex] += string(rune(winput[i]))


Unfortunately this is the most inefficient way to concatenate strings in go (by far)... This is going to get called pretty often, and although it's going to work on a short list, with short strings - it might be preferable to use a string buffer instead. I'll let you decide if you want to implement this differently.

byte buffer to do the original conversion, rather than appending a character on to the string for each pass.

truthbk and others added 30 commits May 24, 2017 14:54

[load] adding system check.

80c4ebe

[system][iostats] adding iostats check + tests.

9635688

[iostats] more precise comment..

22af8c7

[uptime] adding check + tests.

040dcf2

[load] removing logging statement.

fbaa0ae

[iostats][test] amending expected number of asserted calls.

5406b20

[iostats] improve windows support.

df9b96b

[iostats] refactor unix specific io submission, for clarity.

c5c4421

[iostats] refactor, windows numbers differ greatly. Currently not BW …

64e1e44

…compatible.

[iostats] removing unnecessary C(go) header.

79b747a

[iostats] implement device blacklist.

be362cb

[iostats] fixing blacklist management + adding test.

fa9f507

[windows] implement windows IO check

c4ffef7

Initial checkin of code using both WMI & PDH

4e4635a

Modified APM request

efcbd49

touch up logging. Remove calls to APM

2340b03

[load] adding system check.

612d390

[system][iostats] adding iostats check + tests.

f853aab

[iostats] more precise comment..

2487b34

[load] removing logging statement.

33e1aa0

[iostats] improve windows support.

4eed358

[iostats] refactor unix specific io submission, for clarity.

a54c4d4

[iostats] refactor, windows numbers differ greatly. Currently not BW …

6aef62c

…compatible.

[iostats] removing unnecessary C(go) header.

bb3dfe4

[iostats] implement device blacklist.

4161153

[iostats] fixing blacklist management + adding test.

6e62927

[windows] implement windows IO check

b507ec5

Initial checkin of code using both WMI & PDH

6b21bff

Modified APM request

bad63ac

touch up logging. Remove calls to APM

100edda

derekwbrown added 3 commits June 26, 2017 11:26

Switch back to wmi, at least for now. Neither PDH nor WMI is picking …

c213a4c

…up disk changes, at least in the manner tested.

Merge branch 'db/system-both' into db/system-both-rebase

feaeb19

Fix problems with merge

bc2227b

derekwbrown requested review from olivielpeau and truthbk June 27, 2017 02:00

derekwbrown force-pushed the db/system-both-rebase branch 3 times, most recently from fe36c70 to c7b4989 Compare June 27, 2017 04:31

Fix merge. Currently uses WMI; will re-add pdh based IO check at a later

b58e66c

date

derekwbrown force-pushed the db/system-both-rebase branch from c7b4989 to b58e66c Compare June 29, 2017 16:20

truthbk reviewed Jul 4, 2017

View reviewed changes

derekwbrown and others added 3 commits July 5, 2017 14:26

Changes to reflect review feedback

1c46e53

centos6 changes (#354)

1dec683

* adds upstart file to centos6 * adds upstart file to rpm * changes conditional * enables alternative python

Fix gitlab ci so ci testing can continue

90a2612

truthbk approved these changes Jul 12, 2017

View reviewed changes

derekwbrown added 2 commits July 12, 2017 09:27

Final review feedback. Make string conversion more efficient by using a

027d679

byte buffer to do the original conversion, rather than appending a character on to the string for each pass.

Final review feedback. Make string conversion more efficient by using a

521dcd1

byte buffer to do the original conversion, rather than appending a character on to the string for each pass.

derekwbrown merged commit 8b10aa1 into master Jul 12, 2017

masci deleted the db/system-both-rebase branch July 26, 2017 16:45


		drivebuf := make([]byte, 256)

		r, _, err := ProcGetLogicalDriveStringsW.Call(

Conversation

derekwbrown commented Jun 27, 2017

What does this PR do?

Uh oh!

truthbk left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

truthbk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

truthbk left a comment •

edited

Loading