Highlight log lines by keyword. #47507
Conversation
tirkarthi
requested review from
bbovenzi,
ephraimbuddy,
jscheffl,
pierrejeambrun and
ryanahamilton
as code owners
boring-cyborg
bot
added
area:API
|
Yeah, we shoudl adjust those light mode colors. First, want to try a light Code background before playign with the text color? We should also make sure this plays well with #47469. Maybe log source shouldn't be by color then? |
ashb
left a comment
ashb
left a comment
There was a problem hiding this comment.
Sorry, this is -1 for me right now for the reasons below.
| "audit_view_included_events": conf.get("webserver", "audit_view_included_events", fallback=""), | ||
| "audit_view_excluded_events": conf.get("webserver", "audit_view_excluded_events", fallback=""), | ||
| "test_connection": conf.get("core", "test_connection", fallback="Disabled"), | ||
| "color_log_error_keywords": conf.get("logging", "color_log_error_keywords", fallback="").split(","), |
There was a problem hiding this comment.
- there's a
conf.getlistfunction - This is a UI setting, not a logging setting
- Now we have structured logging (where the log level comes form the "source" directly without needing to pase , I don't think we should be looking for keywords.
- I really really really REALLY don't want us to add more config options!
Also "Task succeded without error" would get highlighted as an error. Very prone to false positives.
There was a problem hiding this comment.
This is not a new configuration. This was already present in Airflow 2.10. This is a port of the implementation from the old UI to the new UI. We have use cases where we show logs from external systems like Spark, HDFS etc that are not structured from the triggerer as a log group like "::group::stdout". The keywords help in highlighting the relevant line and improves debugging.
Also "Task succeded without error" would get highlighted as an error. Very prone to false positives.
Agreed, this is like a simple ctrl+f in the browser and has been more useful than a hindrance,
Config docs : https://airflow.apache.org/docs/apache-airflow/stable/configurations-ref.html#color-log-error-keywords
Airflow 2.10 implementation : #37443
There was a problem hiding this comment.
3.0 is the opportunity to decide which config to remove too.
There was a problem hiding this comment.
I am mostly with Ash. I had mostly false-positives with the highlighting so far which ended-up that we disabled all keywords from default as users called because being confused.
For Error/Warning I think the highlighting should be really oriented from the log sub system and log level as we have structured data.
I'd be OK to have the highlighting with keyword, if not "Exception", "Error" and "Warning" are used per default in config (as this would be over-lapping with the log level). It might be use-ful for cases where custom words (other than error...) are to be highlighted.
|
Thanks everyone for the comments. I am closing the PR. This can be revisited if there are requests from users in Airflow 3.x . |
4 participants
Update API to return
color_log_error_keywordsandcolor_log_warning_keywordsfrom airflow.cfg through config API. Then in frontend parse the log message to add color attribute to span tag based on the keyword.Notes to self and review :
airflow.cfg values to test as per the screenshot