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/06/10 11:11:48 UTC
[GitHub] [airflow] rino0601 opened a new pull request, #24373: Fix timezone display for logs on UI by adding %z for %(asctime)s (#23796)
rino0601 opened a new pull request, #24373:
URL: https://github.com/apache/airflow/pull/24373
In this PR I have fixed the bug with logs display in UI.
For those who not using UTC on their system, time conversion doesn't work correctly.
Because previous fix(PR #23075) consider UTC only.
This PR add `%z` to `logging.Formatter.default_time_format` so that moments.js could understand the time properly.
closes: #23796
---
**^ 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 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 a newsfragement 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 #24373: Add %z for %(asctime)s to fix timezone for logs on UI
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #24373:
URL: https://github.com/apache/airflow/pull/24373#issuecomment-1165609482
The failure was because oit was not rebased BTW
--
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 #24373: Fix timezone display for logs on UI by adding %z for %(asctime)s (#23796)
Posted by GitBox <gi...@apache.org>.
rino0601 commented on code in PR #24373:
URL: https://github.com/apache/airflow/pull/24373#discussion_r903430423
##########
newsfragments/24373.improvement.rst:
##########
@@ -0,0 +1 @@
+Support config ``[logging]log_formatter_class`` for overriding logging.Formatter
Review Comment:
@uranusjr
I misinterpreted significant as breaking change.
Referring to the following paragraph of the contribution guide, it seems that 'body' to explain the procedure is supported only in the 'significant' type. is that right?
>In general newsfragments must be one line. For newsfragment type `significant`, you may include summary and body separated by a blank line, similar to `git` commit messages.
If so, I think you should change it to a significant type and put the explanation you suggested. thanks for the comment
--
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 #24373: Fix timezone display for logs on UI by adding %z for %(asctime)s (#23796)
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on code in PR #24373:
URL: https://github.com/apache/airflow/pull/24373#discussion_r898070795
##########
airflow/utils/log/colored_log.py:
##########
@@ -43,6 +43,9 @@ class CustomTTYColoredFormatter(TTYColoredFormatter):
traceback.
"""
+ default_time_format = '%Y-%m-%d %H:%M:%S%z'
+ default_msec_format = '%s %03dms'
Review Comment:
Why are we adding milliseconds to this PR? I don't think the logs showed ms before...
--
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 commented on pull request #24373: Fix timezone display for logs on UI by adding %z for %(asctime)s (#23796)
Posted by GitBox <gi...@apache.org>.
uranusjr commented on PR #24373:
URL: https://github.com/apache/airflow/pull/24373#issuecomment-1165565198
Failure unrelated (and this doesn’t change the providers).
--
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 #24373: Add %z for %(asctime)s to fix timezone for logs on UI
Posted by GitBox <gi...@apache.org>.
rino0601 commented on PR #24373:
URL: https://github.com/apache/airflow/pull/24373#issuecomment-1172295867
@millin Thanks for the suggestion.
I was shocked that I did not realize this simple way on my own. 😱
I need to fix this embarrassing PR right now 🤕
--
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] millin commented on pull request #24373: Add %z for %(asctime)s to fix timezone for logs on UI
Posted by GitBox <gi...@apache.org>.
millin commented on PR #24373:
URL: https://github.com/apache/airflow/pull/24373#issuecomment-1171985380
Dear @rino0601 @uranusjr
Why not just apply ISO 8601 format (e.g 2022-06-12T13:00:00.123+0000)?
For what you complicate and come up with a new non-standard format?
I already suggested in [my PR](https://github.com/apache/airflow/pull/21942) to use ISO 8601 for ES Logger because it is native for it.
https://github.com/apache/airflow/blob/4af6779867691e04e24dbb59f8e3cb75170968ec/airflow/providers/elasticsearch/log/es_json_formatter.py#L29-L48
--
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 commented on a diff in pull request #24373: Fix timezone display for logs on UI by adding %z for %(asctime)s (#23796)
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #24373:
URL: https://github.com/apache/airflow/pull/24373#discussion_r903311532
##########
newsfragments/24373.improvement.rst:
##########
@@ -0,0 +1 @@
+Support config ``[logging]log_formatter_class`` for overriding logging.Formatter
Review Comment:
We probably only need one of these. Also, this should add a sentence saying something like
_If you are using a custom Formatter subclass in your [`logging_config_class`](https://airflow.apache.org/docs/apache-airflow/stable/logging-monitoring/logging-tasks.html#advanced-configuration), please inherit from `airflow.utils.log.timezone_aware.TimezoneAware` instead of `logging.Formatter`_.
--
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 #24373: Fix timezone display for logs on UI by adding %z for %(asctime)s (#23796)
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on PR #24373:
URL: https://github.com/apache/airflow/pull/24373#issuecomment-1154159174
cc: @uranusjr
--
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 commented on pull request #24373: Fix timezone display for logs on UI by adding %z for %(asctime)s (#23796)
Posted by GitBox <gi...@apache.org>.
uranusjr commented on PR #24373:
URL: https://github.com/apache/airflow/pull/24373#issuecomment-1154666856
The logging side (Python) looks reasonable to me, thanks for the effort! On the JavaScript side though, I think we need some compatibiliy code to be able to deal with existing log files, which lack the timezone part.
--
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 commented on pull request #24373: Fix timezone display for logs on UI by adding %z for %(asctime)s (#23796)
Posted by GitBox <gi...@apache.org>.
uranusjr commented on PR #24373:
URL: https://github.com/apache/airflow/pull/24373#issuecomment-1161934514
Just realised, we should add a news entry to tell users using custom loggers to update their configuration so they can benefit from this enhancement.
--
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 #24373: Fix timezone display for logs on UI by adding %z for %(asctime)s (#23796)
Posted by GitBox <gi...@apache.org>.
rino0601 commented on code in PR #24373:
URL: https://github.com/apache/airflow/pull/24373#discussion_r898614509
##########
airflow/utils/log/colored_log.py:
##########
@@ -43,6 +43,9 @@ class CustomTTYColoredFormatter(TTYColoredFormatter):
traceback.
"""
+ default_time_format = '%Y-%m-%d %H:%M:%S%z'
+ default_msec_format = '%s %03dms'
Review Comment:
@bbovenzi
The original value before override in this part is `%s,%03d`.
If left as is, the file will have something like `2022-06-14 06:00:00-0600,123` which is a bit awkward. Also, it's in a format that moments.js doesn't understand.
It would be ideal to have a format like `2022-06-14 06:00:00,123-0600`, but that would be a very difficult because it would have to modify the standard library.
There are 3 ways to solve this problem:
- Avoid specifying milliseconds in the first place. by doing that we can leave the same level of information both in the UI and in the file (which is currently not. file is verbose than UI). It is possible by assigning `default_msec_format` to `None` , but this requires python3.9 or higher. Dropping 3.7 and 3.8 because of this small change ridiculous. so this is not an option. (if there is a way to consume an argument in the %-format but not print it, we can do this without dropping 3.7 and 3.8. but I don't think such a method exists.)
- In order to stick current behavior, modify the regex to match including the rear part of datetime, and then not use the rear part in replaceAll. In this case, the file side has more detailed information, anyway I think `-0600 123ms` is easier to understand than `-0600,123`.
- (which I chose) To provide the same level of information to both the UI and the file, the regex didn't match the milliseconds part. so they could exposed as they is. By doing this, UI shows more information that wasn't there before, but I didn't think that would be a problem.
Previously, milliseconds was not shown because it was discarded from the format of moments.js added on August 3, 2018. While searching for this, I found that the most recently added format (presumably used in grid views) uses milliseconds. Based on this, I guessed that there is a demand for milliseconds.
If you think displaying milliseconds is too verbose, I'll edit the code to use the second method. However, the regular expression become a bit more complicated than it is now, and code of replaceAll become getting the milliseconds value and then ignoring it. This can be awkward code for anyone who hasn't read the conversation in this PR.
--
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 #24373: Fix timezone display for logs on UI by adding %z for %(asctime)s (#23796)
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on PR #24373:
URL: https://github.com/apache/airflow/pull/24373#issuecomment-1156563733
We've started to add logs into the grid view too. I think a useful thing is that we can have a single log parser function that both pages use and we write tests for. I can play around with that after this PR.
--
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 #24373: Fix timezone display for logs on UI by adding %z for %(asctime)s (#23796)
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on code in PR #24373:
URL: https://github.com/apache/airflow/pull/24373#discussion_r900111430
##########
airflow/utils/log/colored_log.py:
##########
@@ -43,6 +43,9 @@ class CustomTTYColoredFormatter(TTYColoredFormatter):
traceback.
"""
+ default_time_format = '%Y-%m-%d %H:%M:%S%z'
+ default_msec_format = '%s %03dms'
Review Comment:
Go ahead and do the simple fix for now. I'm happy to help out combine them into a single parser in a subsequent PR. I think for this change we can target 2.3.3, so it shouldn't be months.
--
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 #24373: Fix timezone display for logs on UI by adding %z for %(asctime)s (#23796)
Posted by GitBox <gi...@apache.org>.
rino0601 commented on PR #24373:
URL: https://github.com/apache/airflow/pull/24373#issuecomment-1159728875
@bbovenzi I updated PR (579b9c92bf749783a888eb6d2eb5b9644a6ff929):
--
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 commented on a diff in pull request #24373: Fix timezone display for logs on UI by adding %z for %(asctime)s (#23796)
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #24373:
URL: https://github.com/apache/airflow/pull/24373#discussion_r903311532
##########
newsfragments/24373.improvement.rst:
##########
@@ -0,0 +1 @@
+Support config ``[logging]log_formatter_class`` for overriding logging.Formatter
Review Comment:
We probably only need one of these. Also, this should add a sentence saying something like _If you are using a custom Formatter subclass in your [`logging_config_class`](https://airflow.apache.org/docs/apache-airflow/stable/logging-monitoring/logging-tasks.html#advanced-configuration), please inherit from `airflow.utils.log.timezone_aware.TimezoneAware` instead of `logging.Formatter`.
##########
newsfragments/24373.improvement.rst:
##########
@@ -0,0 +1 @@
+Support config ``[logging]log_formatter_class`` for overriding logging.Formatter
Review Comment:
We probably only need one of these. Also, this should add a sentence saying something like _If you are using a custom Formatter subclass in your [`logging_config_class`](https://airflow.apache.org/docs/apache-airflow/stable/logging-monitoring/logging-tasks.html#advanced-configuration), please inherit from `airflow.utils.log.timezone_aware.TimezoneAware` instead of `logging.Formatter`_.
--
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 #24373: Fix timezone display for logs on UI by adding %z for %(asctime)s (#23796)
Posted by GitBox <gi...@apache.org>.
rino0601 commented on code in PR #24373:
URL: https://github.com/apache/airflow/pull/24373#discussion_r897653116
##########
airflow/config_templates/airflow_local_settings.py:
##########
@@ -60,10 +59,15 @@
'version': 1,
'disable_existing_loggers': False,
'formatters': {
- 'airflow': {'format': LOG_FORMAT},
+ 'airflow': {
+ 'format': LOG_FORMAT,
+ 'class': 'airflow.utils.log.timezone_aware.TimezoneAware',
Review Comment:
@uranusjr
Like `COLORED_FORMATTER_CLASS`, I changed it to be user configurable. This is a bugfix, and backwards compatibility was adjusted in js, so a new class was designated as fallback.
Could you check it out?
--
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 #24373: Fix timezone display for logs on UI by adding %z for %(asctime)s (#23796)
Posted by GitBox <gi...@apache.org>.
rino0601 commented on code in PR #24373:
URL: https://github.com/apache/airflow/pull/24373#discussion_r899729793
##########
airflow/utils/log/colored_log.py:
##########
@@ -43,6 +43,9 @@ class CustomTTYColoredFormatter(TTYColoredFormatter):
traceback.
"""
+ default_time_format = '%Y-%m-%d %H:%M:%S%z'
+ default_msec_format = '%s %03dms'
Review Comment:
@bbovenzi
After I git rebase onto main:
> A lot of thoughts went through my mind when I saw this screen. 😢
It would be desirable to fix it in this PR because it takes more effort to find and process it later. so I've spend some times try to fix it. it seems that if I fix the regular expression in `grid/details/content/taskInstance/Logs/utils.js` I can fix it for now, but if I do, as you concerned, we will have two log parsers.
Then I stopped trying, to ask you comment. Also, my front-end knowledge is frozen in the days of react15, so I need some time to review if I understand the grid code properly.
---
Anyway,
I wonder if logs in the grid view feature will be released in 2.3 or 2.4.
If it's going to be released in 2.4, so if this PR also has to go out in 2.4 too, I'd like to know in advance.
I'm using 1.10.15 at work, and dozens of instances are stuck with that version. I recently migrated one instance to 2.3.2 as a pilot. Then I discovered this time zone problem. Therefore, other instances are waiting for the result of this PR.
My co-worker's previous contribution was classified as a minor release, it took 5 months to release. If this PR is going to be classified as minor release, I would like to say "migrate now (with workaround config)" to my colleagues because I can guess that it will be released in a few months.
--
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 commented on a diff in pull request #24373: Fix timezone display for logs on UI by adding %z for %(asctime)s (#23796)
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #24373:
URL: https://github.com/apache/airflow/pull/24373#discussion_r897574091
##########
airflow/config_templates/airflow_local_settings.py:
##########
@@ -60,10 +59,15 @@
'version': 1,
'disable_existing_loggers': False,
'formatters': {
- 'airflow': {'format': LOG_FORMAT},
+ 'airflow': {
+ 'format': LOG_FORMAT,
+ 'class': 'airflow.utils.log.timezone_aware.TimezoneAware',
Review Comment:
Let’s make this string a constant like `COLORED_FORMATTER_CLASS`.
--
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] boring-cyborg[bot] commented on pull request #24373: Add %z for %(asctime)s to fix timezone for logs on UI
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on PR #24373:
URL: https://github.com/apache/airflow/pull/24373#issuecomment-1165566139
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.
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 #24373: Fix timezone display for logs on UI by adding %z for %(asctime)s (#23796)
Posted by GitBox <gi...@apache.org>.
rino0601 commented on PR #24373:
URL: https://github.com/apache/airflow/pull/24373#issuecomment-1162669497
Since the UI side code maintains backwards compatibility, I thought it would not be a significant addition, so I wrote it as an improvement and a bugfix. Would it be appropriate?
--
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 #24373: Fix timezone display for logs on UI by adding %z for %(asctime)s (#23796)
Posted by GitBox <gi...@apache.org>.
rino0601 commented on code in PR #24373:
URL: https://github.com/apache/airflow/pull/24373#discussion_r903430423
##########
newsfragments/24373.improvement.rst:
##########
@@ -0,0 +1 @@
+Support config ``[logging]log_formatter_class`` for overriding logging.Formatter
Review Comment:
@uranusjr
I misinterpreted significant as breaking change.
Referring to the following paragraph of the contribution guide, it seems that 'body' to explain the procedure is supported only in the 'significant' type. is that right?
>In general newsfragments must be one line. For newsfragment type `significant`, you may include summary and body separated by a blank line, similar to `git` commit messages.
If so, I think you should change it to a significant type and put the explanation you suggested. thanks for the comment
----
I don't want to cause a full ci by changing the documentation, so I'd like to suggest in the comments and get feedback.
I have changed it as follows to reflect your comments. Would it be appropriate?
> 24373.significant.rst
```
Added new config ``[logging]log_formatter_class`` to fix timezone display for logs on UI
If you are using a custom Formatter subclass in your ``[logging]logging_config_class``, please inherit from ``airflow.utils.log.timezone_aware.TimezoneAware`` instead of ``logging.Formatter``.
For example, in your ``custom_config.py``:
.. code-block:: python
from airflow.utils.log.timezone_aware import TimezoneAware
class YourOldCustomFormatter(logging.Formatter):
...
class YourCustomFormatter(TimezoneAware):
...
LOGGING_CONFIG["formatters"]["airflow"][
"class"
] = "somewhere.your.custom_config.YourNewCustomFormatter"
# or use TimezoneAware class directly. If you use don't need custom Formatter.
LOGGING_CONFIG["formatters"]["airflow"][
"class"
] = "airflow.utils.log.timezone_aware.TimezoneAware"
```
line-endings are looks weird but it forced by black during pre-commit
--
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] github-actions[bot] commented on pull request #24373: Fix timezone display for logs on UI by adding %z for %(asctime)s (#23796)
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #24373:
URL: https://github.com/apache/airflow/pull/24373#issuecomment-1162678561
The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.
--
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 #24373: Add %z for %(asctime)s to fix timezone for logs on UI
Posted by GitBox <gi...@apache.org>.
uranusjr merged PR #24373:
URL: https://github.com/apache/airflow/pull/24373
--
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] boring-cyborg[bot] commented on pull request #24373: Fix timezone display for logs on UI by adding %z for %(asctime)s (#23796)
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on PR #24373:
URL: https://github.com/apache/airflow/pull/24373#issuecomment-1152249461
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/main/CONTRIBUTING.rst)
Here are some useful points:
- Pay attention to the quality of your code (flake8, mypy and type annotations). Our [pre-commits]( https://github.com/apache/airflow/blob/main/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/main/docs/apache-airflow/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/main/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.
- Please follow [ASF Code of Conduct](https://www.apache.org/foundation/policies/conduct) for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
- Be sure to read the [Airflow Coding style]( https://github.com/apache/airflow/blob/main/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://s.apache.org/airflow-slack
--
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 #24373: Fix timezone display for logs on UI by adding %z for %(asctime)s (#23796)
Posted by GitBox <gi...@apache.org>.
rino0601 commented on code in PR #24373:
URL: https://github.com/apache/airflow/pull/24373#discussion_r898614509
##########
airflow/utils/log/colored_log.py:
##########
@@ -43,6 +43,9 @@ class CustomTTYColoredFormatter(TTYColoredFormatter):
traceback.
"""
+ default_time_format = '%Y-%m-%d %H:%M:%S%z'
+ default_msec_format = '%s %03dms'
Review Comment:
@bbovenzi
The original value before override in this part is `%s,%03d`.
If left as is, the file will have something like `2022-06-14 06:00:00-0600,123` which is a bit awkward. Also, it's in a format that moments.js doesn't understand.
It would be ideal to have a format like `2022-06-14 06:00:00,123-0600`, but that would be a very difficult because it would have to modify the standard library.
There are 3 ways to solve this problem:
- Avoid specifying milliseconds in the first place. by doing that we can leave the same level of information both in the UI and in the file (which is currently not. file is verbose than UI). It is possible by assigning `default_msec_format` to `None` , but this requires python3.9 or higher. Dropping 3.7 and 3.8 because of this small change is ridiculous. so this is not an option. (if there is a way to consume an argument in the %-format but not print it, we can do this without dropping 3.7 and 3.8. but I don't think such a method exists.)
- In order to stick current behavior, modify the regex to match including the rear part of datetime, and then not use the rear part in replaceAll. In this case, the file side has more detailed information, anyway I think `-0600 123ms` is easier to understand than `-0600,123`.
- (which I chose) To provide the same level of information to both the UI and the file, the regex didn't match the milliseconds part. so they could exposed as they is. By doing this, UI shows more information that wasn't there before, but I didn't think that would be a problem.
Previously, milliseconds was not shown because it was discarded from the format of moments.js added on August 3, 2018. While searching for this, I found that the most recently added format (presumably used in grid views) uses milliseconds. Based on this, I guessed that there is a demand for milliseconds.
If you think displaying milliseconds is too verbose, I'll edit the code to use the second method. However, the regular expression become a bit more complicated than it is now, and code of replaceAll become getting the milliseconds value and then ignoring it. This can be awkward code for anyone who hasn't read the conversation in this PR.
--
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 #24373: Fix timezone display for logs on UI by adding %z for %(asctime)s (#23796)
Posted by GitBox <gi...@apache.org>.
rino0601 commented on code in PR #24373:
URL: https://github.com/apache/airflow/pull/24373#discussion_r898614509
##########
airflow/utils/log/colored_log.py:
##########
@@ -43,6 +43,9 @@ class CustomTTYColoredFormatter(TTYColoredFormatter):
traceback.
"""
+ default_time_format = '%Y-%m-%d %H:%M:%S%z'
+ default_msec_format = '%s %03dms'
Review Comment:
@bbovenzi
The original value before override in this part is `%s,%03d`.
If left as is, the file will have something like `2022-06-14 06:00:00-0600,123` which is a bit awkward. Also, it's in a format that moments.js doesn't understand.
It would be ideal to have a format like `2022-06-14 06:00:00,123-0600`, but that would be a very difficult because it would have to modify the standard library.
There are 3 ways to solve this problem:
- Avoid specifying milliseconds in the first place. by doing that we can leave the same level of information both in the UI and in the file (which is currently not. file is verbose than UI). It is possible by assigning `default_msec_format` to `None` , but this requires python3.9 or higher. Dropping 3.7 and 3.8 because of this small change is ridiculous. so this is not an option. (if there is a way to consume an argument in the %-format but not print it, we can do this without dropping 3.7 and 3.8. but I don't think such a method exists.)
- In order to stick current behavior, modify the regex to match including the rear part of datetime, and then not use the rear part in replaceAll. In this case, the file side has more detailed information, anyway I think `-0600 123ms` is easier to understand than `-0600,123`.
- (which I chose) To provide the same level of information to both the UI and the file, the regex didn't match the milliseconds part. so they could exposed as they is. By doing this, UI shows more information that wasn't there before, but I didn't think that would be a problem.
Previously, milliseconds was not shown because it was discarded from the format of moments.js added on August 3, 2018. While searching for this, I found that the most recently added format (presumably used in grid views) uses milliseconds. Based on this, I guessed that there is a demand for milliseconds.
If you think displaying milliseconds is too verbose, I'll edit the code to use the second method. However, the regular expression become a bit more complicated than it is now, and code of replaceAll become getting the milliseconds value and then ignoring it. This can be awkward code for anyone who hasn't read the conversation in this PR.
--
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 #24373: Fix timezone display for logs on UI by adding %z for %(asctime)s (#23796)
Posted by GitBox <gi...@apache.org>.
rino0601 commented on PR #24373:
URL: https://github.com/apache/airflow/pull/24373#issuecomment-1155970779
@bbovenzi
I did a local rebase and force push to update the branch. I didn't know this would cancel workflow approval. Could you do it again? thank you
@uranusjr
By specifying a group in the regular expression, the code has been changed to perform the previous behavior when matched with the old format. b09da7a5d4c17190ef0cd3410e9976bb3fad6df7
And, speaking of ui test, although the jest configuration file is seen in the `www` directory, it seems to be for grid view only. I'm not familiar with jquery, webpack, I have no idea of a way to test the jquery code. Even if it were possible, I guess it would be impossible without major structural changes.
If someone makes a test procedure for the files under `www/static/js`, I'd be happy to contribute to maintaining the test code related to this PR.
--
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 #24373: Fix timezone display for logs on UI by adding %z for %(asctime)s (#23796)
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on code in PR #24373:
URL: https://github.com/apache/airflow/pull/24373#discussion_r899494685
##########
airflow/utils/log/colored_log.py:
##########
@@ -43,6 +43,9 @@ class CustomTTYColoredFormatter(TTYColoredFormatter):
traceback.
"""
+ default_time_format = '%Y-%m-%d %H:%M:%S%z'
+ default_msec_format = '%s %03dms'
Review Comment:
We _should_ send the date to the UI that moment can parse. But that is annoying if it's non-trivial. We can go with this for now. But we should get that single log parsing function soon so the gird view keeps this in consideration 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