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

[AIRFLOW-4069] Add Opsgenie Alert Hook and Operator #4903

Merged
merged 1 commit into from
Apr 5, 2019

Conversation

nritholtz
Copy link
Contributor

@nritholtz nritholtz commented Mar 11, 2019

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"
    • In case you are fixing a typo in the documentation you can prepend your commit with [AIRFLOW-XXX], code changes always need a Jira issue.
    • In case you are proposing a fundamental code change, you need to create an Airflow Improvement Proposal(AIP).

Description

  • Here are some details about my PR, including screenshots of any UI changes:
    Add a hook and operator for the Opsgenie Alert API to create alerts.

Tests

  • My PR adds the following unit tests OR does not need testing for this extremely good reason:
    tests/contrib/hooks/test_opsgenie_alert_hook.py and tests/contrib/operators/test_opsgenie_alert_operator.py

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
    • If you implement backwards incompatible changes, please leave a note in the Updating.md so we can assign it to a appropriate release

Code Quality

  • Passes flake8

@nritholtz nritholtz changed the title [WIP] - AIRFLOW-4069: Add Opsgenie Alert Hook [WIP] - [AIRFLOW-4069] Add Opsgenie Alert Hook Mar 11, 2019
@nritholtz nritholtz changed the title [WIP] - [AIRFLOW-4069] Add Opsgenie Alert Hook [AIRFLOW-4069] Add Opsgenie Alert Hook Mar 11, 2019
@nritholtz nritholtz force-pushed the AIRFLOW-4069 branch 3 times, most recently from 924610a to a911721 Compare March 11, 2019 22:39
@codecov-io
Copy link

codecov-io commented Mar 11, 2019

Codecov Report

Merging #4903 into master will increase coverage by 0.05%.
The diff coverage is 96.49%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4903      +/-   ##
==========================================
+ Coverage   76.23%   76.29%   +0.05%     
==========================================
  Files         466      468       +2     
  Lines       30101    30188      +87     
==========================================
+ Hits        22949    23031      +82     
- Misses       7152     7157       +5
Impacted Files Coverage Δ
airflow/contrib/hooks/opsgenie_alert_hook.py 100% <100%> (ø)
airflow/utils/db.py 90.38% <100%> (+0.09%) ⬆️
...rflow/contrib/operators/opsgenie_alert_operator.py 93.93% <93.93%> (ø)
airflow/utils/log/file_processor_handler.py 87.25% <0%> (+1.14%) ⬆️

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 dbed51e...778c919. Read the comment docs.

@nritholtz
Copy link
Contributor Author

nritholtz commented Mar 12, 2019

@zhongjiajie Fair point, I removed the ability to explicitly pass the api_key to the hook.

@zhongjiajie
Copy link
Member

@nritholtz Well I notice that you add new hook without operator. should we add operator to this new hook?

@nritholtz
Copy link
Contributor Author

nritholtz commented Mar 12, 2019

@zhongjiajie On second thought, Ill add an operator as well since I'm already in this code.

@nritholtz nritholtz changed the title [AIRFLOW-4069] Add Opsgenie Alert Hook [WIP] [AIRFLOW-4069] Add Opsgenie Alert Hook Mar 12, 2019
@nritholtz nritholtz changed the title [WIP] [AIRFLOW-4069] Add Opsgenie Alert Hook [WIP] [AIRFLOW-4069] Add Opsgenie Alert Hook and Operator Mar 12, 2019
@nritholtz
Copy link
Contributor Author

Added operator for the hook, and also converted the api_key field to a connection password. This way, the BaseHook debug_info call will obfuscate the key in the logs.

@nritholtz nritholtz force-pushed the AIRFLOW-4069 branch 2 times, most recently from 9aa25da to 886f8be Compare March 13, 2019 01:34
Copy link
Member

@zhongjiajie zhongjiajie left a comment

Choose a reason for hiding this comment

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

My 2 cents.

airflow/contrib/hooks/opsgenie_alert_hook.py Outdated Show resolved Hide resolved
airflow/contrib/hooks/opsgenie_alert_hook.py Outdated Show resolved Hide resolved
airflow/contrib/hooks/opsgenie_alert_hook.py Outdated Show resolved Hide resolved
airflow/contrib/operators/opsgenie_alert_operator.py Outdated Show resolved Hide resolved
airflow/contrib/hooks/opsgenie_alert_hook.py Outdated Show resolved Hide resolved
docs/code.rst Outdated Show resolved Hide resolved
@nritholtz nritholtz force-pushed the AIRFLOW-4069 branch 2 times, most recently from f7057ae to 1adaa4f Compare March 13, 2019 02:23
@nritholtz nritholtz changed the title [WIP] [AIRFLOW-4069] Add Opsgenie Alert Hook and Operator [AIRFLOW-4069] Add Opsgenie Alert Hook and Operator Mar 13, 2019
@zhongjiajie
Copy link
Member

cc @mik-laj we still have discuss in #4903 (comment) and #4903 (comment).
after we finish we could ask committer to review this PR.

@nritholtz
Copy link
Contributor Author

I've updated the PR as per the suggestions and also added additional test coverage for the hook.

@nritholtz nritholtz force-pushed the AIRFLOW-4069 branch 2 times, most recently from d7853a3 to 3c9bb51 Compare March 18, 2019 10:50
@nritholtz
Copy link
Contributor Author

Made suggested changes.

Just an FYI, while reviewing the Dingding code as per your suggestion, I found an issue with the doc generation on https://github.com/apache/airflow/blob/master/airflow/contrib/hooks/dingding_hook.py#L104 where there needs to be a newline for the param/type to be parsed correctly.

@ashb
Copy link
Member

ashb commented Apr 5, 2019

Yes it does, good catch.

@nritholtz nritholtz force-pushed the AIRFLOW-4069 branch 2 times, most recently from c230a47 to 39ed1a6 Compare April 5, 2019 16:59
@nritholtz
Copy link
Contributor Author

@ashb Only one job in the build failed due to unrelated tests:

======================================================================
1) ERROR: test_integration_run_dag (tests.contrib.minikube.test_kubernetes_executor.KubernetesExecutorTest)
----------------------------------------------------------------------
   Traceback (most recent call last):
    tests/contrib/minikube/test_kubernetes_executor.py line 173 in test_integration_run_dag
      result_json = self.start_dag(dag_id=dag_id, host=host)
    tests/contrib/minikube/test_kubernetes_executor.py line 134 in start_dag
      'dags/{dag_id}/paused/false'.format(host=host, dag_id=dag_id)
    .tox/py35-backend_postgres-env_kubernetes/lib/python3.5/site-packages/requests/api.py line 75 in get
      return request('get', url, params=params, **kwargs)
    .tox/py35-backend_postgres-env_kubernetes/lib/python3.5/site-packages/requests/api.py line 60 in request
      return session.request(method=method, url=url, **kwargs)
    .tox/py35-backend_postgres-env_kubernetes/lib/python3.5/site-packages/requests/sessions.py line 533 in request
      resp = self.send(prep, **send_kwargs)
    .tox/py35-backend_postgres-env_kubernetes/lib/python3.5/site-packages/requests/sessions.py line 646 in send
      r = adapter.send(request, **kwargs)
    .tox/py35-backend_postgres-env_kubernetes/lib/python3.5/site-packages/requests/adapters.py line 516 in send
      raise ConnectionError(e, request=request)
   ConnectionError: HTTPConnectionPool(host='10.20.1.120', port=30809): Max retries exceeded with url: /api/experimental/dags/example_kubernetes_executor_config/paused/false (Caused by NewConnectionError('<urllib3.connection.HTTPConnection object at 0x7f3181a65cc0>: Failed to establish a new connection: [Errno 111] Connection refused',))
   -------------------- >> begin captured stdout << ---------------------
   [2019-04-05 17:34:09,772] {connectionpool.py:205} DEBUG - Starting new HTTP connection (1): 10.20.1.120:30809
   
   --------------------- >> end captured stdout << ----------------------
   -------------------- >> begin captured logging << --------------------
   airflow.utils.log.logging_mixin.LoggingMixin: INFO: Creating new Airflow config file in: /home/airflow/airflow.cfg
   airflow.utils.log.logging_mixin.LoggingMixin: INFO: Reading the config from /home/airflow/airflow.cfg
   airflow.utils.log.logging_mixin.LoggingMixin: INFO: Creating new FAB webserver config file in: /home/airflow/webserver_config.py
   airflow.settings: INFO: Configured default timezone <Timezone [UTC]>
   root: INFO: Generating grammar tables from /usr/lib/python3.5/lib2to3/Grammar.txt
   root: INFO: Generating grammar tables from /usr/lib/python3.5/lib2to3/PatternGrammar.txt
   airflow.utils.log.logging_mixin.LoggingMixin: DEBUG: Cannot import  due to  doesn't look like a module path
   airflow.logging_config: DEBUG: Unable to load custom logging, using default config instead
   urllib3.connectionpool: DEBUG: Starting new HTTP connection (1): 10.20.1.120:30809
   --------------------- >> end captured logging << ---------------------
======================================================================
2) ERROR: test_integration_run_dag_with_scheduler_failure (tests.contrib.minikube.test_kubernetes_executor.KubernetesExecutorTest)
----------------------------------------------------------------------
   Traceback (most recent call last):
    tests/contrib/minikube/test_kubernetes_executor.py line 196 in test_integration_run_dag_with_scheduler_failure
      result_json = self.start_dag(dag_id=dag_id, host=host)
    tests/contrib/minikube/test_kubernetes_executor.py line 134 in start_dag
      'dags/{dag_id}/paused/false'.format(host=host, dag_id=dag_id)
    .tox/py35-backend_postgres-env_kubernetes/lib/python3.5/site-packages/requests/api.py line 75 in get
      return request('get', url, params=params, **kwargs)
    .tox/py35-backend_postgres-env_kubernetes/lib/python3.5/site-packages/requests/api.py line 60 in request
      return session.request(method=method, url=url, **kwargs)
    .tox/py35-backend_postgres-env_kubernetes/lib/python3.5/site-packages/requests/sessions.py line 533 in request
      resp = self.send(prep, **send_kwargs)
    .tox/py35-backend_postgres-env_kubernetes/lib/python3.5/site-packages/requests/sessions.py line 646 in send
      r = adapter.send(request, **kwargs)
    .tox/py35-backend_postgres-env_kubernetes/lib/python3.5/site-packages/requests/adapters.py line 516 in send
      raise ConnectionError(e, request=request)
   ConnectionError: HTTPConnectionPool(host='10.20.1.120', port=30809): Max retries exceeded with url: /api/experimental/dags/example_kubernetes_executor_config/paused/false (Caused by NewConnectionError('<urllib3.connection.HTTPConnection object at 0x7f3181a65208>: Failed to establish a new connection: [Errno 111] Connection refused',))

All suggested changes have been made. PTAL

@ashb
Copy link
Member

ashb commented Apr 5, 2019

Taking a look. Yeah, the kube tests timeout sometimes :(

@ashb
Copy link
Member

ashb commented Apr 5, 2019

@nritholtz Two small changes - if you can make them soon I can get this in to 1.10.3

@nritholtz
Copy link
Contributor Author

@ashb looks like unrelated test failures again on 2 of the jobs. Otherwise made all suggested changes.

Thanks.

@nritholtz
Copy link
Contributor Author

@ashb Thanks for the rebuild, looks all green now. 🎉

@ashb ashb merged commit 958f8ce into apache:master Apr 5, 2019
@nritholtz nritholtz deleted the AIRFLOW-4069 branch April 5, 2019 21:41
@zhongjiajie
Copy link
Member

zhongjiajie commented Apr 6, 2019

Made suggested changes.

Just an FYI, while reviewing the Dingding code as per your suggestion, I found an issue with the doc generation on https://github.com/apache/airflow/blob/master/airflow/contrib/hooks/dingding_hook.py#L104 where there needs to be a newline for the param/type to be parsed correctly.

Good catch, I will add new line tomorrow, Thanks. @nritholtz

cthenderson pushed a commit to cthenderson/apache-airflow that referenced this pull request Apr 16, 2019
andriisoldatenko pushed a commit to andriisoldatenko/airflow that referenced this pull request Jul 26, 2019
wmorris75 pushed a commit to modmed/incubator-airflow that referenced this pull request Jul 29, 2019
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.

5 participants