Skip to content

Containerized network check reports the host metrics#994

Merged
JulienBalestra merged 2 commits into
masterfrom
JulienBalestra/network-check-a6-support
Jan 10, 2018
Merged

Containerized network check reports the host metrics#994
JulienBalestra merged 2 commits into
masterfrom
JulienBalestra/network-check-a6-support

Conversation

@JulienBalestra

Copy link
Copy Markdown
Contributor

What does this PR do?

This PR allows to get some host network stats when the agent is running inside a container and not in the host network namespace.

To do so, you need to take advantage of the following entry in the agent datadog.yaml:

procfs_path: "/host/proc"

Then mount the host /proc inside the container to /host/proc.

However in this setup, the following network configuration is always ignored:

instances:
  - collect_connection_state: true

This is because the metrics associated with the option collect_connection_state are collected by ss / netstat.
Theses binaries don't support a custom /proc path.

Additional Notes

I refactor (pep8, 257, ...) some parts of the check in the first commit.
I'll comment inline to help the review.

@JulienBalestra
JulienBalestra requested review from a team January 5, 2018 10:51
Comment thread network/check.py
@@ -237,13 +236,10 @@ def _submit_devicemetrics(self, iface, vals_by_metric):
self.log.debug("tracked %s network metrics for interface %s" % (count, iface))

def _parse_value(self, v):

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.

Calling long("-") will raise ValueError

v = "-"
long(v)
Traceback (most recent call last):
  File "<input>", line 1, in <module>
ValueError: invalid literal for long() with base 10: '-'

Comment thread network/check.py
if self._collect_cx_state is False:
return False

if proc_location != "/proc":

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.

I'm not sure if it's interesting to add a condition over Platform.is_containerized()

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.

I think that's not necessary indeed

@olivielpeau olivielpeau left a comment

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.

LGTM 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants