Skip to content

Conversation

@ultrabug
Copy link
Contributor

One of the most frustrating topic for users is usually related
to their understanding on the scheduler decisions about running
a DAG or not.

It would be wise to add more logs in the jobs creation decision
so that it gets more clear whether a DAG is run or not and why.

Dear Airflow maintainers,

Please accept this PR. I understand that it will not be reviewed until I have checked off all the steps below!

JIRA

Description

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

One of the most frustrating topic for users is usually related to their understanding on the scheduler decisions about running a DAG or not.

It would be wise to add more logs in the jobs creation decision so that it gets more clear whether a DAG is run or not and why.

This patch adds such simple and useful logs.

Tests

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

Logs only

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
    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"

@codecov-io
Copy link

codecov-io commented Jul 18, 2017

Codecov Report

Merging #2455 into master will increase coverage by 0.04%.
The diff coverage is 81.81%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2455      +/-   ##
==========================================
+ Coverage    69.4%   69.44%   +0.04%     
==========================================
  Files         146      146              
  Lines       11289    11298       +9     
==========================================
+ Hits         7835     7846      +11     
+ Misses       3454     3452       -2
Impacted Files Coverage Δ
airflow/jobs.py 76.73% <81.81%> (+0.39%) ⬆️

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 751e936...b68b8f2. Read the comment docs.

airflow/jobs.py Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

info level plz

Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as above everywhere ( put dag_id here)

airflow/jobs.py Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

debug level plz

airflow/jobs.py Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

debug level plz

airflow/jobs.py Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

debug level plz

airflow/jobs.py Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

debug lvl plz

airflow/jobs.py Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

debug lvl please

Copy link
Contributor

Choose a reason for hiding this comment

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

Put dag name and runs in the message too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@saguziel I did not put the dag_id in any of the logs to remain in line with the other logs and also because they are already split by their dag name.

Do you still want me to do this? If so, would you like all logs to include the dag_id to remain consistent?

@ultrabug
Copy link
Contributor Author

Thanks for your reply @bolkedebruin but I really think this is worth an INFO log level since it's crucial to understand the decision making of the scheduler in production environment.

airflow/jobs.py Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

probably should be is not None

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right, good point

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry, actually it's is None instead, committing

@ultrabug
Copy link
Contributor Author

@bolkedebruin @saguziel I've updated the PR with requested changes.

@bolkedebruin
Copy link
Contributor

@ultrabug I disagree with your assessment of the requirement for the high log levels. The scheduler is already quite chatty. Please lower the levels.

@ultrabug
Copy link
Contributor Author

@bolkedebruin okay, can we settle to at least keep the last one (next scheduled execution time) to info? This one is very valuable.

Thanks in advance

@bolkedebruin
Copy link
Contributor

Sure no problem.

One of the most frustrating topic for users is usually related
to their understanding on the scheduler decisions about running
a DAG or not.

It would be wise to add more logs in the jobs creation decision
so that it gets more clear whether a DAG is run or not and why.
@ultrabug
Copy link
Contributor Author

@bolkedebruin done, thanks.

@stale
Copy link

stale bot commented Dec 10, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Dec 10, 2018
@ron819
Copy link
Contributor

ron819 commented Dec 11, 2018

@ultrabug was there something else needs to be done in this PR ?

@stale stale bot removed the stale Stale PRs per the .github/workflows/stale.yml policy file label Dec 11, 2018
@ultrabug
Copy link
Contributor Author

@ron819 well AFAIK I've done what has been asked for me but ofc now there are conflicts..

I'd be more happy to see #2460 getting attention tho, since it gives a better user experience over this kind of information

# return if already reached maximum active runs and no timeout setting
if len(active_runs) >= dag.max_active_runs and not dag.dagrun_timeout:
self.logger.info(
"Dag reached maximum of {} active runs (no timeout)".
Copy link
Member

Choose a reason for hiding this comment

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

Such formatting of messages causes that a string object is created, which then may not be used anywhere, when the login level will be too low.
Formatting parameters should be passed as arguments to the info method.
Example:

self.log.info("The Table '%s' does not exists already.", self.table_id)
```

@stale
Copy link

stale bot commented Sep 3, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Sep 3, 2019
@OmerJog
Copy link
Contributor

OmerJog commented Sep 4, 2019

@mik-laj Is the format the only issue here?
This PR is standing for a while and it's just about logging :/
I'm willing to re-PR if @ultrabug has no time but lets first decide if we want this change in.
No reason to hang PRs if the functionality is disputed

@stale stale bot removed the stale Stale PRs per the .github/workflows/stale.yml policy file label Sep 4, 2019
@ultrabug
Copy link
Contributor Author

ultrabug commented Sep 4, 2019

@OmerJog I'll happily update this PR shall it be acknowledged first indeed.

@stale
Copy link

stale bot commented Nov 20, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Nov 20, 2019
@ultrabug
Copy link
Contributor Author

@mik-laj @bolkedebruin @saguziel will be happy to correct anything if there's a chance it gets some attention, answer would be appreciated thanks.

If not, please close this PR yourself so we at least know where we stand.

Cheers mates.

@stale stale bot removed the stale Stale PRs per the .github/workflows/stale.yml policy file label Nov 20, 2019
@stale
Copy link

stale bot commented Jan 4, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Jan 4, 2020
@stale stale bot closed this Jan 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stale Stale PRs per the .github/workflows/stale.yml policy file

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants