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

Make the plugin compatible with Airflow >= "v2.6.0" #43

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

shuvaevv
Copy link

@shuvaevv shuvaevv commented Nov 10, 2023

Closes: #40

  1. Fix the DummyStatsLogger import.
    This bug appeared because DummyStatsLogger was renamed to NoStatsLogger in Airflow v2.6.0.
    The import of the DummyStatsLogger is try-excepted, so the DummyStatsLogger class is None. It leads to the AttributeError: 'NoneType' object has no attribute 'incr'.

  2. Fix the unexpected keyword argument error after the "tag" argument was added to StatsLogger in Airflow v2.6.0

@shuvaevv shuvaevv requested a review from a team November 10, 2023 17:47
@shuvaevv shuvaevv force-pushed the fix-DummyStatsLogger-import branch 2 times, most recently from 82b1e24 to c45e990 Compare November 13, 2023 01:03
@TimPansino
Copy link
Contributor

Hello,

This PR looks helpful, but unfortunately the CI pipeline for this project was canned by security during an incident related to Azure. I'll have to rewrite it in GitHub Actions before we can merge anything. I'll get back to this as soon as we have a new CI in place.

@shuvaevv shuvaevv changed the title Fix import of StatsLogger for Airflow >= v2.6.0 Make the plugin compatible with Airflow >= "v2.6.0" Nov 16, 2023
@TimPansino
Copy link
Contributor

As of right now we don't have plans to finish fixing the CI, so this is unlikely to be merged or released. We're in talks of archiving this project as it's nowhere near as stable as the builtin OpenTelemetry offering.

If you'd like to give that a try, I'd highly recommend it over the current state of this plugin.

I put up a new documentation page about it here

@thesuperzapper
Copy link

@TimPansino I can confirm that this works in Airflow 2.9.3, can we please merge this?

Otherwise, people won't be able to use the newrelic-airflow-plugin with Airflow 2.6+

@thesuperzapper
Copy link

@TimPansino I also want to highlight that Airflow has a critical issue in its OpenTelemetry integrations, which will cause tasks to fail if the OpenTelemetry endpoint is down:

The NewRelic needs to fix this issue in airflow if you don't intend to support this plugin anymore.

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.

NEW_RELIC_INSERT_KEY causing plugin import failure
3 participants