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 2022/07/03 06:07:22 UTC
[GitHub] [airflow] rino0601 opened a new pull request, #24811: Add %z for %(asctime)s to fix timezone for logs on UI
rino0601 opened a new pull request, #24811:
URL: https://github.com/apache/airflow/pull/24811
follow up of #24373
related:
original issue #23796
previous pr #24373
revert previous pr #24810
reference #21942
As already addressed in #24810, #24373 has git conflict problem. This PR solves that problem and makes it able to release as a bugfix.
Also, thanks to @millin 's [comment](https://github.com/apache/airflow/pull/24373#issuecomment-1171985380), by applying ISO 8601 format, it can drop complicated and duplicated custom parse code on UI.
This pr must be merged after #24810
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code changes, an 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 a newsfragment file, named `{pr_number}.significant.rst`, in [newsfragments](https://github.com/apache/airflow/tree/main/newsfragments).
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #24811: Add %z for %(asctime)s to fix timezone for logs on UI
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #24811:
URL: https://github.com/apache/airflow/pull/24811#issuecomment-1181392738
Should we possibly add a warning if someone does not use TimezoneAware formatter? I think it is no harm to add it (with clear instruction on what to do). I think writing it in the logs of tasks, would make it much more discoverable - you could simply print a warning "Beware, this log might contain wrong timezone and you should use this and that to fix it".
There are two kinds of users - those who read the "migration guides and changelog" and those who don't and the latter kind of user will generally open an issue if they see logs with wrong timestamp. Producing a warning in the very logs that the user might complain about is as good as we can do to make the user self-serviced, especially if we are very explicit that "timestamps here might have wrong timezone" and add "this is how you fix it".
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] bbovenzi commented on a diff in pull request #24811: Add %z for %(asctime)s to fix timezone for logs on UI
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on code in PR #24811:
URL: https://github.com/apache/airflow/pull/24811#discussion_r913939830
##########
airflow/www/static/js/ti_log.js:
##########
@@ -102,9 +102,8 @@ function autoTailingLog(tryNumber, metadata = null, autoTailing = false) {
// Detect urls and log timestamps
const urlRegex = /http(s)?:\/\/[\w.-]+(\.?:[\w.-]+)*([/?#][\w\-._~:/?#[\]@!$&'()*+,;=.%]+)?/g;
- const dateRegex = /(\d{4}[./-]\d{2}[./-]\d{2} \d{2}:\d{2}:\d{2})((,\d{3})|([+-]\d{4} \d{3}ms))/g;
- // above regex is a kind of duplication of 'dateRegex'
- // in airflow/www/static/js/grid/details/content/taskinstance/Logs/utils.js
+ const dateRegex = /\d{4}[./-]\d{2}[./-]\d{2} \d{2}:\d{2}:\d{2},\d{3}/g;
+ const iso8601Regex = /\d{4}[./-]\d{2}[./-]\d{2}T\d{2}:\d{2}:\d{2}.\d{3}[+-]\d{4}/g;
Review Comment:
Should we not do both of these checks in `utils.ts` too?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr merged pull request #24811: Add %z for %(asctime)s to fix timezone for logs on UI
Posted by GitBox <gi...@apache.org>.
uranusjr merged PR #24811:
URL: https://github.com/apache/airflow/pull/24811
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] rino0601 commented on pull request #24811: Add %z for %(asctime)s to fix timezone for logs on UI
Posted by GitBox <gi...@apache.org>.
rino0601 commented on PR #24811:
URL: https://github.com/apache/airflow/pull/24811#issuecomment-1181294641
Could you please give this PR a `type:bug-fix` label and an `Airflow 2.3.4` milestone? thank you
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] rino0601 commented on pull request #24811: Add %z for %(asctime)s to fix timezone for logs on UI
Posted by GitBox <gi...@apache.org>.
rino0601 commented on PR #24811:
URL: https://github.com/apache/airflow/pull/24811#issuecomment-1179482384
By `git rebase apache/main && git push --force-with-lease` solved conflicts.
also, `git cherry-pick ${THIS_PR_HASH}` onto v2-3-test branch doesn't make any conflicts as intended.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] rino0601 commented on a diff in pull request #24811: Add %z for %(asctime)s to fix timezone for logs on UI
Posted by GitBox <gi...@apache.org>.
rino0601 commented on code in PR #24811:
URL: https://github.com/apache/airflow/pull/24811#discussion_r912438277
##########
airflow/config_templates/config.yml:
##########
@@ -635,7 +635,7 @@
default: "%%(asctime)s %%(levelname)s - %%(message)s"
- name: log_formatter_class
description: ~
- version_added: 2.3.3
+ version_added: 2.3.4
Review Comment:
bump up.
#24373 didn't show up on https://github.com/apache/airflow/pull/24762/files#diff-9b07fd28838e759e05bdaa5293886d860374fe291bdc6cc43c20f951fc77eab4 and it's commits
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] bbovenzi commented on pull request #24811: Add %z for %(asctime)s to fix timezone for logs on UI
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on PR #24811:
URL: https://github.com/apache/airflow/pull/24811#issuecomment-1179103591
@rino0601 getting some conflicts now after https://github.com/apache/airflow/pull/24810
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] rino0601 commented on a diff in pull request #24811: Add %z for %(asctime)s to fix timezone for logs on UI
Posted by GitBox <gi...@apache.org>.
rino0601 commented on code in PR #24811:
URL: https://github.com/apache/airflow/pull/24811#discussion_r915431542
##########
airflow/www/static/js/ti_log.js:
##########
@@ -102,9 +102,8 @@ function autoTailingLog(tryNumber, metadata = null, autoTailing = false) {
// Detect urls and log timestamps
const urlRegex = /http(s)?:\/\/[\w.-]+(\.?:[\w.-]+)*([/?#][\w\-._~:/?#[\]@!$&'()*+,;=.%]+)?/g;
- const dateRegex = /(\d{4}[./-]\d{2}[./-]\d{2} \d{2}:\d{2}:\d{2})((,\d{3})|([+-]\d{4} \d{3}ms))/g;
- // above regex is a kind of duplication of 'dateRegex'
- // in airflow/www/static/js/grid/details/content/taskinstance/Logs/utils.js
+ const dateRegex = /\d{4}[./-]\d{2}[./-]\d{2} \d{2}:\d{2}:\d{2},\d{3}/g;
+ const iso8601Regex = /\d{4}[./-]\d{2}[./-]\d{2}T\d{2}:\d{2}:\d{2}.\d{3}[+-]\d{4}/g;
Review Comment:
The regular expression on the `utils.ts` side is `const regExp = /\[(.*?)\] \{(.*?)\}/;`, which finds the string between square brackets instead of finding the time string.
moments.js understands the ISO 8601 format, so we don't have to worry about this.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] rino0601 commented on pull request #24811: Add %z for %(asctime)s to fix timezone for logs on UI
Posted by GitBox <gi...@apache.org>.
rino0601 commented on PR #24811:
URL: https://github.com/apache/airflow/pull/24811#issuecomment-1183269270
<img width="943" alt="airflow_–_taskinstance_py" src="https://user-images.githubusercontent.com/1422445/178752953-a57052d2-6f75-49eb-b925-01ee418189ed.png">
I've tried to add a warnings but it didn't work at the first try.
I'm still digging how to it work. any advice will helpful.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org