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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
