You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2020/05/19 12:37:58 UTC
[GitHub] [airflow] Moress opened a new pull request #8911: Support extra config options for Sentry
Moress opened a new pull request #8911:
URL: https://github.com/apache/airflow/pull/8911
For now only dsn can be configured through the airflow.cfg. Need support 'http_proxy' option for example (it can't be configured through the environment variables). This change implements solution for supporting all existed (and maybe future) options for sentry configuration.
---
Make sure to mark the boxes below before creating PR: [x]
- [ ] Description above provides context of the change
- [ ] Unit tests coverage for changes (not needed for documentation changes)
- [ ] Target Github ISSUE in description if exists
- [ ] Commits follow "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)"
- [ ] Relevant documentation is updated including usage instructions.
- [ ] I will engage committers as explained in [Contribution Workflow Example](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#contribution-workflow-example).
---
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/master/UPDATING.md).
Read the [Pull Request Guidelines](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#pull-request-guidelines) for more information.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] AbhiPrasad commented on a change in pull request #8911: Support extra config options for Sentry
Posted by GitBox <gi...@apache.org>.
AbhiPrasad commented on a change in pull request #8911:
URL: https://github.com/apache/airflow/pull/8911#discussion_r445491051
##########
File path: docs/errors.rst
##########
@@ -39,6 +39,8 @@ Add your ``SENTRY_DSN`` to your configuration file e.g. ``airflow.cfg`` under ``
.. note::
If this value is not provided, the SDK will try to read it from the ``SENTRY_DSN`` environment variable.
+You can supply `additional configuration options <https://docs.sentry.io/error-reporting/configuration/?platform=python>`__ based on the Python platform via [sentry] section.
Review comment:
Can you also quickly note the unsupported options here, just so everything is clear?
```
UNSUPPORTED_SENTRY_OPTIONS = frozenset(
("integrations", "in_app_include", "in_app_exclude", "ignore_errors",
"before_breadcrumb", "before_send", "transport")
)
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Moress commented on pull request #8911: Support extra config options for Sentry
Posted by GitBox <gi...@apache.org>.
Moress commented on pull request #8911:
URL: https://github.com/apache/airflow/pull/8911#issuecomment-655947676
@mik-laj please approve this :smile:
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Moress edited a comment on pull request #8911: Support extra config options for Sentry
Posted by GitBox <gi...@apache.org>.
Moress edited a comment on pull request #8911:
URL: https://github.com/apache/airflow/pull/8911#issuecomment-630921263
@AbhiPrasad
Add some description for extra options in docs/errors.rst.
Should i add some guard exclusion for some option (ex. transport) or sentry_sdk management way is suitable? We always can see troubles in try/except block on start app.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Moress commented on pull request #8911: Support extra config options for Sentry
Posted by GitBox <gi...@apache.org>.
Moress commented on pull request #8911:
URL: https://github.com/apache/airflow/pull/8911#issuecomment-630921263
Add some description for extra options in docs/errors.rst.
Should i add some guard exclusion for some option (ex. transport) or sentry_sdk management way is suitable? We always can see troubles in try/except block on start app.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] boring-cyborg[bot] commented on pull request #8911: Support extra config options for Sentry
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on pull request #8911:
URL: https://github.com/apache/airflow/pull/8911#issuecomment-630789645
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst)
Here are some useful points:
- Pay attention to the quality of your code (flake8, pylint and type annotations). Our [pre-commits]( https://github.com/apache/airflow/blob/master/STATIC_CODE_CHECKS.rst#prerequisites-for-pre-commit-hooks) will help you with that.
- In case of a new feature add useful documentation (in docstrings or in `docs/` directory). Adding a new operator? Check this short [guide](https://github.com/apache/airflow/blob/master/docs/howto/custom-operator.rst) Consider adding an example DAG that shows how users should use it.
- Consider using [Breeze environment](https://github.com/apache/airflow/blob/master/BREEZE.rst) for testing locally, itโs a heavy docker but it ships with a working Airflow and a lot of integrations.
- Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
- Be sure to read the [Airflow Coding style]( https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#coding-style-and-best-practices).
Apache Airflow is a community-driven project and together we are making it better ๐.
In case of doubts contact the developers at:
Mailing List: dev@airflow.apache.org
Slack: https://apache-airflow-slack.herokuapp.com/
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on pull request #8911: Support extra config options for Sentry
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #8911:
URL: https://github.com/apache/airflow/pull/8911#issuecomment-655949477
@Moress Can you do a rebase? This branch is a bit old and it is worth checking if there are any problems again.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Moress commented on pull request #8911: Support extra config options for Sentry
Posted by GitBox <gi...@apache.org>.
Moress commented on pull request #8911:
URL: https://github.com/apache/airflow/pull/8911#issuecomment-635998075
@mik-laj,
Need i do something else for approving?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj merged pull request #8911: Support extra config options for Sentry
Posted by GitBox <gi...@apache.org>.
mik-laj merged pull request #8911:
URL: https://github.com/apache/airflow/pull/8911
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Moress commented on a change in pull request #8911: Support extra config options for Sentry
Posted by GitBox <gi...@apache.org>.
Moress commented on a change in pull request #8911:
URL: https://github.com/apache/airflow/pull/8911#discussion_r429652975
##########
File path: airflow/sentry.py
##########
@@ -81,13 +81,17 @@ def __init__(self):
sentry_celery = CeleryIntegration()
integrations.append(sentry_celery)
- dsn = conf.get("sentry", "sentry_dsn")
+ dsn = None
+ sentry_config_opts = conf.getsection("sentry") or {}
Review comment:
> getsection method returns dict[str,str]
It's not quite true. Here (https://github.com/apache/airflow/blob/master/airflow/configuration.py#L422) you can see that code is going through `_section` dict and trying to cast strings to proper type.
But fair enough it's not working for these props (list, dict and custom type is not supported by getsection):
```
integrations
in_app_include
in_app_exclude
ignore_errors
before_breadcrumb
before_send
transport
```
I will add some guard exception for these props.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on pull request #8911: Support extra config options for Sentry
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #8911:
URL: https://github.com/apache/airflow/pull/8911#issuecomment-630791621
@tiopi @AbhiPrasad Can I ask for review?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Moress commented on a change in pull request #8911: Support extra config options for Sentry
Posted by GitBox <gi...@apache.org>.
Moress commented on a change in pull request #8911:
URL: https://github.com/apache/airflow/pull/8911#discussion_r429652975
##########
File path: airflow/sentry.py
##########
@@ -81,13 +81,17 @@ def __init__(self):
sentry_celery = CeleryIntegration()
integrations.append(sentry_celery)
- dsn = conf.get("sentry", "sentry_dsn")
+ dsn = None
+ sentry_config_opts = conf.getsection("sentry") or {}
Review comment:
> getsection method returns dict[str,str]
It's not quite true. Here (https://github.com/apache/airflow/blob/master/airflow/configuration.py#L422) you can see that code is going through `_section` dict and trying to cast strings to proper type.
But fair enough it's not working for these props (list, dict and custom type is not supported by getsection):
```
integrations
in_app_include
in_app_exclude
ignore_errors
before_breadcrumb
before_send
transport
```
I will add some guard exception for these props.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Moress commented on a change in pull request #8911: Support extra config options for Sentry
Posted by GitBox <gi...@apache.org>.
Moress commented on a change in pull request #8911:
URL: https://github.com/apache/airflow/pull/8911#discussion_r427807387
##########
File path: airflow/sentry.py
##########
@@ -81,13 +81,17 @@ def __init__(self):
sentry_celery = CeleryIntegration()
integrations.append(sentry_celery)
- dsn = conf.get("sentry", "sentry_dsn")
+ dsn = None
+ sentry_config_opts = conf.getsection("sentry") or {}
Review comment:
This section has already existed in docs. I update section description in airflow/config_templates/config.yml with info of supporting additional configs from sentry docs.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Moress commented on pull request #8911: Support extra config options for Sentry
Posted by GitBox <gi...@apache.org>.
Moress commented on pull request #8911:
URL: https://github.com/apache/airflow/pull/8911#issuecomment-656064062
> @Moress Can you do a rebase? This branch is a bit old and it is worth checking if there are any problems again.
Done.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] AbhiPrasad commented on pull request #8911: Support extra config options for Sentry
Posted by GitBox <gi...@apache.org>.
AbhiPrasad commented on pull request #8911:
URL: https://github.com/apache/airflow/pull/8911#issuecomment-630928121
@Moress
> Should i add some guard exclusion for some option (ex. transport) or sentry_sdk management way is suitable?
This way right now is fine. Thank you for your contribution :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #8911: Support extra config options for Sentry
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #8911:
URL: https://github.com/apache/airflow/pull/8911#discussion_r429560311
##########
File path: airflow/sentry.py
##########
@@ -81,13 +81,17 @@ def __init__(self):
sentry_celery = CeleryIntegration()
integrations.append(sentry_celery)
- dsn = conf.get("sentry", "sentry_dsn")
+ dsn = None
+ sentry_config_opts = conf.getsection("sentry") or {}
Review comment:
getsection method returns `dict[str,str]`. This means that options that are not strings will not work properly.
https://github.com/getsentry/sentry-python/blob/464ca8dda09155fcc43dfbb6fa09cf00313bf5b8/sentry_sdk/consts.py#L37
The following options will works:
```
ca_certs
dist
dsn
environment
http_proxy
https_proxy
ignore_errors
in_app_exclude
in_app_include
release
server_name
request_bodies
```
The following options will NOT works:
```
attach_stacktrace
before_breadcrumb
before_send
debug
default_integrations
integrations
max_breadcrumbs
propagate_traces
sample_rate
send_default_pii
shutdown_timeout
transport
with_locals
```
This can cause very difficult to debug problems. It would be great if we supported more options or if an option is not supported, we would report a user-friendly error message.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Moress commented on a change in pull request #8911: Support extra config options for Sentry
Posted by GitBox <gi...@apache.org>.
Moress commented on a change in pull request #8911:
URL: https://github.com/apache/airflow/pull/8911#discussion_r445997582
##########
File path: docs/errors.rst
##########
@@ -39,6 +39,8 @@ Add your ``SENTRY_DSN`` to your configuration file e.g. ``airflow.cfg`` under ``
.. note::
If this value is not provided, the SDK will try to read it from the ``SENTRY_DSN`` environment variable.
+You can supply `additional configuration options <https://docs.sentry.io/error-reporting/configuration/?platform=python>`__ based on the Python platform via [sentry] section.
Review comment:
Done. Add this info in doc and config templates.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] boring-cyborg[bot] commented on pull request #8911: Support extra config options for Sentry
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on pull request #8911:
URL: https://github.com/apache/airflow/pull/8911#issuecomment-656383460
Awesome work, congrats on your first merged pull request!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #8911: Support extra config options for Sentry
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #8911:
URL: https://github.com/apache/airflow/pull/8911#discussion_r429560311
##########
File path: airflow/sentry.py
##########
@@ -81,13 +81,17 @@ def __init__(self):
sentry_celery = CeleryIntegration()
integrations.append(sentry_celery)
- dsn = conf.get("sentry", "sentry_dsn")
+ dsn = None
+ sentry_config_opts = conf.getsection("sentry") or {}
Review comment:
getsection method returns `dict[str,str]`. This means that options that are not strings will not work properly.
https://github.com/getsentry/sentry-python/blob/464ca8dda09155fcc43dfbb6fa09cf00313bf5b8/sentry_sdk/consts.py#L37
The following options will works:
```
ca_certs
dist
dsn
environment
http_proxy
https_proxy
ignore_errors
in_app_exclude
in_app_include
release
server_name
request_bodies
```
The following options will NOT works due to type mismatch:
```
attach_stacktrace
before_breadcrumb
before_send
debug
default_integrations
integrations
max_breadcrumbs
propagate_traces
sample_rate
send_default_pii
shutdown_timeout
transport
with_locals
```
This can cause very difficult to debug problems. It would be great if we supported more options or if an option is not supported, we would report a user-friendly error message.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #8911: Support extra config options for Sentry
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #8911:
URL: https://github.com/apache/airflow/pull/8911#discussion_r427539144
##########
File path: airflow/sentry.py
##########
@@ -81,13 +81,17 @@ def __init__(self):
sentry_celery = CeleryIntegration()
integrations.append(sentry_celery)
- dsn = conf.get("sentry", "sentry_dsn")
+ dsn = None
+ sentry_config_opts = conf.getsection("sentry") or {}
Review comment:
Can you add this option to reference documentation?
airflow/config_templates/config.yml
https://airflow.readthedocs.io/en/latest/configurations-ref.html
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org