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.

Sign up for GitHub

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

Jump to bottom

[AIRFLOW-3559] Add missing options to DatadogHook. #4362

Merged
merged 1 commit into from
Jan 17, 2019
Merged

[AIRFLOW-3559] Add missing options to DatadogHook. #4362

merged 1 commit into from
Jan 17, 2019

Conversation

jmcarp
Copy link
Contributor

@jmcarp jmcarp commented Dec 22, 2018

Make sure you have checked all steps below.

Jira

  • My PR addresses the following Airflow Jira issues and references them in the PR title. For example, "[AIRFLOW-XXX] My Airflow PR"

Description

  • Here are some details about my PR, including screenshots of any UI changes:

Adds missing arguments to DatadogHook.

Tests

  • My PR adds the following unit tests OR does not need testing for this extremely good reason:

Backfills missing tests for DatadogHook

Commits

  • My commits all reference Jira issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message":
    1. Subject is separated from body by a blank line
    2. Subject is limited to 50 characters (not including Jira issue reference)
    3. Subject does not end with a period
    4. Subject uses the imperative mood ("add", not "adding")
    5. Body wraps at 72 characters
    6. Body explains "what" and "why", not "how"

Documentation

  • In case of new functionality, my PR adds documentation that describes how to use it.
    • When adding new operators/hooks/sensors, the autoclass documentation generation needs to be added.
    • All the public functions and the classes in the PR contain docstrings that explain what it does

Code Quality

  • Passes flake8

@jmcarp jmcarp force-pushed the add-datadog-arguments branch 2 times, most recently from c4bc3eb to 359f778 Compare December 22, 2018 19:44
@codecov-io
Copy link

codecov-io commented Dec 22, 2018 •
edited
Loading

Codecov Report

Merging #4362 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4362   +/-   ##
=======================================
  Coverage   79.45%   79.45%           
=======================================
  Files         204      204           
  Lines       16499    16499           
=======================================
  Hits        13109    13109           
  Misses       3390     3390

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0e112d3...2815e58. Read the comment docs.

kaxil
kaxil requested changes Dec 27, 2018
View reviewed changes
airflow/contrib/hooks/datadog_hook.py

def validate_response(self, response):
if response['status'] != 'ok':
self.log.error("Datadog returned: %s", response)
raise AirflowException("Error status received from Datadog")

def send_metric(self, metric_name, datapoint, tags=None):
def send_metric(self, metric_name, datapoint, tags=None, type_=None, interval=None):
Copy link
Member

Choose a reason for hiding this comment

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

typo .. type_ -> type

Copy link
Member

Choose a reason for hiding this comment

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

or may be change the name to metric_type

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The option is called type in the datadog docs, and I appended an underscore to avoid shadowing the type builtin in python.

airflow/contrib/hooks/datadog_hook.py Outdated
@@ -76,12 +65,18 @@ def send_metric(self, metric_name, datapoint, tags=None):
:type datapoint: int or float
:param tags: A list of tags associated with the metric
:type tags: list
:param type: Type of your metric either: gauge, rate, or count
:type type: string
Copy link
Member

Choose a reason for hiding this comment

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

string -> str

airflow/contrib/hooks/datadog_hook.py Outdated
:param alert_type: The alert type for the event, one of
["error", "warning", "info", "success"]
:type alert_type: str
:param aggregation_key: Key that can be used to aggregate this event in a stream
:type aggregation_key: str
:date_happened: POSIX timestamp of the event; defaults to now
Copy link
Member

Choose a reason for hiding this comment

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

:type date_happened:

@jmcarp
Copy link
Contributor Author

jmcarp commented Dec 27, 2018

Updated

@jmcarp
Copy link
Contributor Author

jmcarp commented Jan 6, 2019

PTAL when you have time @kaxil

kaxil
kaxil reviewed Jan 6, 2019
View reviewed changes
airflow/contrib/hooks/datadog_hook.py
"Datadog connection details")
if self.app_key is None:
raise AirflowException("app_key must be specified in the "
"Datadog connection details")
Copy link
Member

Choose a reason for hiding this comment

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

In absence of this code block when a user forgets to have api_key in Connection extras, it won't give a useful error. Any reasoning behind removing this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In some cases, the app key isn't required (from https://docs.datadoghq.com/api/?lang=python#authentication):

Requests that write data require reporting access and require an API key. Requests that read data require full access and also require an application key.

I'll update so that the api key is required and the app key isn't.

@jmcarp
Copy link
Contributor Author

jmcarp commented Jan 10, 2019

Updated.

@jmcarp
Copy link
Contributor Author

jmcarp commented Jan 16, 2019

Ready for review when you have time @kaxil

@kaxil kaxil merged commit 1770bf5 into apache:master Jan 17, 2019
@kaxil
Copy link
Member

kaxil commented Jan 17, 2019

Thanks @jmcarp

ashb pushed a commit that referenced this pull request Mar 26, 2019
@jmcarp @ashb
[AIRFLOW-3559] Add missing options to DatadogHook. (#4362)
9dcf1f6
wmorris75 pushed a commit to modmed/incubator-airflow that referenced this pull request Jul 29, 2019
@jmcarp
[AIRFLOW-3559] Add missing options to DatadogHook. (apache#4362)
3f944e5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kaxil kaxil kaxil approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jmcarp @codecov-io @kaxil