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

[openstack] added project hypervisor and server tagging to project_running metric #1211

Closed
wants to merge 1 commit into from

Conversation

dbr1993
Copy link

@dbr1993 dbr1993 commented Mar 7, 2018

What does this PR do?

Added a new metric for openstack.nova.project.running. This metric is tagged with the hypervisor_hostname, the project name and server name.

Added a fix for host aggregates - web request to nova returns short names, trimmed the output from get_my_hostname so that the compute nodes would match.

Motivation

Extra functionality for a customer

Testing Guidelines

An overview on testing
is available in our contribution guidelines.

Versioning

  • Bumped the check version in manifest.json
  • Bumped the check version in datadog_checks/{integration}/__init__.py
  • Updated CHANGELOG.md. Please use Unreleased as the date in the title
    for the new section.
  • If PR impacts documentation, docs team has been notified or an issue has been opened on the documentation repo

Additional Notes

Anything else we should know when reviewing?

@masci masci self-assigned this Mar 7, 2018
Copy link
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.

Tests are failing because of the code linter. Left few comments, fixes should make the linter happy.
Other than that, this LGTM

@@ -663,16 +663,36 @@ def get_all_server_ids(self, filter_by_host=None):

return server_ids

def get_project_name_from_id(self, tenant_id):
url = "{0}/{1}/{2}/{3}".format(self.keystone_server_url, DEFAULT_KEYSTONE_API_VERSION, "projects", tenant_id)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit - With Python 2.7 in this case there's no need to specify the numbers for the placeholders

url = "{0}/{1}/{2}/{3}".format(self.keystone_server_url, DEFAULT_KEYSTONE_API_VERSION, "projects", tenant_id)
self.log.debug("Project URL is %s", url)
headers = {'X-Auth-Token': self.get_auth_token()}
self.log.debug("Headers %s", headers)
Copy link
Contributor

Choose a reason for hiding this comment

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

this would leak the auth token in the logs and should be removed

@@ -691,7 +711,8 @@ def _is_valid_metric(label):
for st in server_stats:
if _is_valid_metric(st):
self.gauge("openstack.nova.server.{0}".format(st.replace("-", "_")), server_stats[st], tags=tags, hostname=server_id)

if hypervisor_hostname and project_name and server_name:
self.gauge('openstack.nova.project.running', 1, tags=['hypervisor_hostname:{0}'.format(hypervisor_hostname), 'project_name:{0}'.format(project_name), 'server_name:{0}'.format(server_name)])
Copy link
Contributor

Choose a reason for hiding this comment

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

this line is indented with 3 spaces, it should be 4

@@ -714,10 +735,12 @@ def _is_valid_metric(label):
if _is_valid_metric(st):
metric_key = PROJECT_METRICS[st]
self.gauge("openstack.nova.limits.{0}".format(metric_key), server_stats['limits']['absolute'][st], tags=tags)

Copy link
Contributor

Choose a reason for hiding this comment

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

can you remove this whitespace?

@@ -986,3 +1009,4 @@ def get_external_host_tags(self):

self.log.debug("Sending external_host_tags: %s", external_host_tags)
return external_host_tags

Copy link
Contributor

Choose a reason for hiding this comment

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

this empty line should be removed

def get_stats_for_all_projects(self, projects):
for project in projects:
self.get_stats_for_single_project(project)


Copy link
Contributor

Choose a reason for hiding this comment

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

remove extra blank line

@truthbk truthbk self-assigned this Mar 8, 2018
@masci masci changed the title added project hypervisor and server tagging to project_running metric [openstack] added project hypervisor and server tagging to project_running metric Apr 6, 2018
@nmuesch
Copy link
Collaborator

nmuesch commented May 8, 2018

Hey @dbr1993 thanks for the contribution. As we discussed I'm going to close this in favor of #1276.

@nmuesch nmuesch closed this May 8, 2018
@masci masci deleted the dbr1993/project_tagging_openstack branch June 6, 2018 11:54
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.

4 participants