Skip to content

Db/win pychecks merged#406

Merged
derekwbrown merged 11 commits intomasterfrom
db/win_pychecks_merged
Jul 27, 2017
Merged

Db/win pychecks merged#406
derekwbrown merged 11 commits intomasterfrom
db/win_pychecks_merged

Conversation

@derekwbrown
Copy link
Copy Markdown
Contributor

Agent6 can now load python checks, specifically the WMI check.

Copy link
Copy Markdown
Contributor

@masci masci left a comment

Choose a reason for hiding this comment

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

left few comments but it looks good

Comment thread cmd/agent/common/helpers.go Outdated
PyChecksPath,
filepath.Join(GetDistPath(), "checks"),
config.Datadog.GetString("additional_checksd"))
//filepath.Join(_here, "lib"),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

can we remove the commented lines?

@@ -0,0 +1,344 @@
# (C) Datadog, Inc. 2010-2016
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why don't we use the check from integrations-core?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Probably should be reviewed, outside the scope of this PR. I did the same thing we have in agent5, which is a module called wmi_check in agent/checks, and a slightly different module called wmi_check in agent/checks.d (via integrations-core)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

My bad, if we wipe this, we break a bunch of Windows check 😄

Comment thread pkg/collector/py/check.go Outdated
defer gstate.unlock()

// call run function, it takes no args so we pass an empty tuple
log.Debugf("Running python check %s", c.ModuleName)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

we might add the check ID here, otherwise if we have multiple instances there's no way to see which one is producing the log message

@derekwbrown derekwbrown merged commit cce7a20 into master Jul 27, 2017
@masci masci deleted the db/win_pychecks_merged branch July 27, 2017 17:01
// NOTICE: this will also setup the Python environment
coll := collector.NewCollector(GetDistPath(), filepath.Join(GetDistPath(), "checks"),
config.Datadog.GetString("additional_checksd"), PyChecksPath)
os.Setenv("PYTHONHOME", _here)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This breaks the agent on unix, since we set this environment variable in the agent shell script (see

exec env PYTHONHOME=$EMBEDDEDPATH $DIRPATH/agent.bin "$@"
).

If this is required on windows, could we only set it if the platform is windows or if PYTHONHOME is already set?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

that's actually a mistake; that's leftover from trial/error getting stuff to work. I'll remove it.

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