-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Add external_host_tags wrapper to checks_base #3316
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔥
Codecov Report
@@ Coverage Diff @@
## master #3316 +/- ##
==========================================
+ Coverage 79.54% 84.25% +4.71%
==========================================
Files 152 60 -92
Lines 7669 4661 -3008
Branches 929 569 -360
==========================================
- Hits 6100 3927 -2173
+ Misses 1346 613 -733
+ Partials 223 121 -102 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, not good !! set_external_tags
expects a list of tuple like this: ( <hostname>, {<source_type>: tags})
@@ -290,6 +290,19 @@ def service_metadata(self, meta_name, value): | |||
def check(self, instance): | |||
raise NotImplementedError | |||
|
|||
def set_external_tags(self, external_tags): | |||
from datadog_agent import set_external_tags as _set_external_tags |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the lazy import?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I figured that we wouldn't need the try/catch at the top of the file if we imported directly in this function. If you're using the AgentCheck class(es) it means you're running on A6, so its gauranteed to be there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall 😄
@@ -218,6 +220,21 @@ def test_none_value(self): | |||
normalized_tags = check._normalize_tags_type(tags, None) | |||
assert normalized_tags == ['tag:foo'] | |||
|
|||
def test_external_host_tag_normalization(self): | |||
""" | |||
Tests that th external_host_tag modifies in place the list of tags in the provided object |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤦♂️
@@ -701,6 +714,19 @@ def convert_to_underscore_separated(self, name): | |||
metric_name = self.METRIC_REPLACEMENT.sub(br'_', metric_name) | |||
return self.DOT_UNDERSCORE_CLEANUP.sub(br'.', metric_name).strip(b'_') | |||
|
|||
def set_external_tags(self, external_tags): | |||
from datadog_agent import set_external_tags as _set_external_tags | |||
# Example of external_tags format |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 for this comments!!
# ('hostname2', {'src2_name': ['test2:t3']}) | ||
# ] | ||
for x in external_tags: | ||
for src_name, tags in iteritems(x[1]): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we validate the structure of x
just in case it is not set properly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll wrap in a try/catch around an indexError, what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, discussed this with @zippolyte The Agent currently emits an error if the format of these tags isn't correct. In that spirit, I've set this to catch an IndexError and log an exception (and then raise that exception) This keeps the same functionality that the Agent was currently performing. (Agent code that does validation - https://github.com/DataDog/datadog-agent/blob/master/pkg/collector/py/datadog_agent.c#L147)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one nit, feel free to merge once addressed!
In general, exposing wrapper functions to checks (instead of the builtin datadog_agent
module directly) sounds more flexible to me as well 👍
@@ -3,6 +3,8 @@ | |||
# (C) Datadog, Inc. 2018 | |||
# All rights reserved | |||
# Licensed under a 3-clause BSD style license (see LICENSE) | |||
import sys |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this import needed? (don't see it used anywhere)
Surprised a linter didn't catch this 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was originally there when I was doing more mocking than necessary, will remove thanks for catching!
5139b20
What does this PR do?
Adds a wrapper to the Datadog Agent's (A6's) set_external_tags function.
This wrapper makes sure the tags are in the expected format (depending on PY2/3) when being sent to the Datadog Agent.
Imports datadog_agent inside the function because it means that we aren't on A5 if we're using the AgentCheck class from here.
Having this allows us to remove some tag normalization from the specific checks. For example, vsphere uses set_external_tags and has to attempt to normalize tags in specific formats depending on the Agent being used (Py2/3) Having this wrapper allows us to let the AgentCheck class do this directly and implicitly, making it less error prone for future checks.
Also added a test
Motivation
Attempting to port a check such as vsphere was proving difficult, as the tags needed to be in different formats depending on whether the check was running in Py 2 or Py 3.
Additional Notes
Anything else we should know when reviewing?
Review checklist (to be filled by reviewers)
changelog/
andintegration/
labels attached