PR-2 Plugin configuration from airflow.cfg #32
Conversation
Multiple dimensions Local docker setup
|
@newrelic/python I would suggest removing the test on python27 |
|
Python 2.7 is unfortunately something we're going to be stuck supporting for quite some time. A lot of high profile customers for Python still use 2.7, so we're not willing to drop support yet. It looks like the error is a dependency issue, probably should be addressed separately. What's the purpose of the |
|
Hi @TimPansino, sorry for the late reply. I have been on vacation. The "static dimensions"-feature is a request from the field. They would like to be able to send more information for a given airflow instance, which helps identify, separate, or aggregate the data of multiple airflow instances. As this is now possible via |
pschrimpf
force-pushed
the
feature/airflow_cfg
branch
from
0767855
to
48260cd
Compare
pschrimpf
force-pushed
the
feature/airflow_cfg
branch
from
99566f4
to
017fcf3
Compare
|
@newrelic/python I fixed the dependency issue for python2.7 - how are we going to proceed? Any other remarks on that PR? |
There was a problem hiding this comment.
This PR needs a bit more work, I may step in and do some of the edits to ensure we have stable code with no potential crashes for customers.
This doesn't include any additional tests to test the changes made here, it could use a test for dimensional metrics at the very least if not also config file vs environment testing.
| @@ -24,5 +24,5 @@ | |||
| version = "{version!s}" | |||
| """, | |||
| }, | |||
| install_requires=("apache-airflow>=1.8", "newrelic-telemetry-sdk>=0.4.0,<0.5"), | |||
| install_requires=("email-validator==1.2.1", "apache-airflow>=1.8", "newrelic-telemetry-sdk>=0.4.0,<0.5"), | |||
There was a problem hiding this comment.
| install_requires=("email-validator==1.2.1", "apache-airflow>=1.8", "newrelic-telemetry-sdk>=0.4.0,<0.5"), | |
| install_requires=("apache-airflow>=1.8", "newrelic-telemetry-sdk>=0.4.0,<0.5"), |
This seems like a mistake. Why are we adding another dependency?
There was a problem hiding this comment.
HI @TimPansino, the email validator is a sub-dependency coming with apache-airflow. Unfortunately, not pinned to a version compatible with Python2.7. I thought it would be better to pin that particular dependency instead of apache-airflow.
| # Set default configs | ||
| if PROP_INSERT_KEY not in nr_config and ENV_INSERT_KEY in os.environ: | ||
| nr_config[PROP_INSERT_KEY] = os.environ.get(ENV_INSERT_KEY) | ||
|
|
||
| if PROP_SERVICE_NAME not in nr_config: | ||
| nr_config[PROP_SERVICE_NAME] = os.environ.get(ENV_SERVICE_NAME, "Airflow") | ||
|
|
||
| if PROP_HOST not in nr_config: | ||
| nr_config[PROP_HOST] = os.environ.get(ENV_HOST, None) | ||
|
|
||
| if PROP_HARVESTER_INTERVAL not in nr_config: | ||
| nr_config[PROP_HARVESTER_INTERVAL] = 5 | ||
|
|
||
| return nr_config |
There was a problem hiding this comment.
We probably shouldn't mutate the user's config, and instead do this the other way around. Retrieve our config section with a function and check for environment variables if the key isn't present. If neither is present use a default.
There was a problem hiding this comment.
I am not really sure what you mean. For me it looks like your described approach is how it is already implemented. 😅
| if "NEW_RELIC_INSERT_KEY" in os.environ and not cls.patched: | ||
| if PROP_INSERT_KEY in config and not cls.patched: |
There was a problem hiding this comment.
This is not equivalent if we swap config strategies. Will need to check both conditions.
There was a problem hiding this comment.
At this moment the config should already have the value from the environment variable (if not directly defined in airflow.cfg)
pschrimpf
force-pushed
the
feature/airflow_cfg
branch
9 times, most recently
from
bcd21e9
to
b558b61
Compare
pschrimpf
force-pushed
the
feature/airflow_cfg
branch
from
b558b61
to
9093841
Compare
pschrimpf
force-pushed
the
feature/airflow_cfg
branch
from
4f327a2
to
1a4e363
Compare
|
|
Multiple dimensions
Local docker setup