Re: [PR] Add color to log lines in UI for error and warnings based on keywords [airflow]
bbovenzi merged PR #39006: URL: https://github.com/apache/airflow/pull/39006 -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Add color to log lines in UI for error and warnings based on keywords [airflow]
tirkarthi commented on PR #39006: URL: https://github.com/apache/airflow/pull/39006#issuecomment-2067480912 The legacy log view doesn't have code changes to process every line. It just has the whole log as a single string so doing splitting and then adding ansi codes to join them back was more work that I left off since there is an issue to remove legacy log view especially with full-screen mode and other improvements being made to new UI. -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Add color to log lines in UI for error and warnings based on keywords [airflow]
jscheffl commented on code in PR #39006:
URL: https://github.com/apache/airflow/pull/39006#discussion_r1572806934
##
airflow/www/static/js/utils/index.ts:
##
@@ -185,6 +185,34 @@ const toSentenceCase = (camelCase: string): string => {
return "";
};
+const addColorKeyword = (
+ parsedLine: string,
+ errorKeywords: string[],
+ warningKeywords: string[]
+): string => {
+ const lowerParsedLine = parsedLine.toLowerCase();
+ const containsError = errorKeywords.some((keyword) =>
+lowerParsedLine.includes(keyword)
+ );
+ const bold = (line: string) => `\x1b[1m${line}\x1b[0m`;
+ const red = (line: string) => `\x1b[31m${line}\x1b[39m`;
Review Comment:
okay, but you are measuring in ms still... most time probably is needed by
browser then anyway.
--
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-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Add color to log lines in UI for error and warnings based on keywords [airflow]
tirkarthi commented on code in PR #39006:
URL: https://github.com/apache/airflow/pull/39006#discussion_r1572653543
##
airflow/www/static/js/utils/index.ts:
##
@@ -185,6 +185,34 @@ const toSentenceCase = (camelCase: string): string => {
return "";
};
+const addColorKeyword = (
+ parsedLine: string,
+ errorKeywords: string[],
+ warningKeywords: string[]
+): string => {
+ const lowerParsedLine = parsedLine.toLowerCase();
+ const containsError = errorKeywords.some((keyword) =>
+lowerParsedLine.includes(keyword)
+ );
+ const bold = (line: string) => `\x1b[1m${line}\x1b[0m`;
+ const red = (line: string) => `\x1b[31m${line}\x1b[39m`;
+ const yellow = (line: string) => `\x1b[33m${line}\x1b[39m`;
+
+ if (containsError) {
+return bold(red(parsedLine));
+ }
+
+ const containsWarning = warningKeywords.some((keyword) =>
Review Comment:
I kept them away since if the error keyword is found I wanted to return the
line red colored and don't want to search the line for warning keyword as an
optimisation. Keeping them next to each other will make 2 searches for every
line though only one is used in case of error keyword being present.
--
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-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Add color to log lines in UI for error and warnings based on keywords [airflow]
tirkarthi commented on PR #39006:
URL: https://github.com/apache/airflow/pull/39006#issuecomment-2066933356
@dirrao @jscheffl Used below dag which generated around 30MB log file and
the function call added around 15ms with total render time taking 9-10 seconds.
I did it with dev builds using "yarn dev" since the function name was not
searchable in production mode possibly due to no source mapping. Please let me
know if I missed anything. Thanks
```pythonfrom __future__ import annotations
from datetime import datetime
from airflow import DAG
from airflow.decorators import task
with DAG(
dag_id="perf_39006",
start_date=datetime(2024, 1, 1),
catchup=False,
schedule_interval=None,
) as dag:
@task
def log_line_generator():
import random
lorem_ipsum_words = "Lorem ipsum dolor sit amet, consectetur
adipiscing elit, sed do eiusmod tempor".split()
for _ in range(200_000):
random.shuffle(lorem_ipsum_words)
line = " ".join(lorem_ipsum_words)
rand = random.random()
if rand > 0.9:
print(f"{line} error")
elif rand > 0.8:
print(f"{line} warn")
else:
print(line)
log_line_generator()
```
Without patch
With patch :
--
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-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Add color to log lines in UI for error and warnings based on keywords [airflow]
tirkarthi commented on code in PR #39006:
URL: https://github.com/apache/airflow/pull/39006#discussion_r1572643904
##
airflow/www/static/js/utils/index.ts:
##
@@ -185,6 +185,34 @@ const toSentenceCase = (camelCase: string): string => {
return "";
};
+const addColorKeyword = (
+ parsedLine: string,
+ errorKeywords: string[],
+ warningKeywords: string[]
+): string => {
+ const lowerParsedLine = parsedLine.toLowerCase();
+ const containsError = errorKeywords.some((keyword) =>
+lowerParsedLine.includes(keyword)
+ );
+ const bold = (line: string) => `\x1b[1m${line}\x1b[0m`;
+ const red = (line: string) => `\x1b[31m${line}\x1b[39m`;
Review Comment:
Thanks, it helped performance to go from 15ms to 2ms. I tried inlining by
formatting and returning the string itself but for some reason the time went
back to 15ms though it was my assumption inlining the code would increase
performance which didn't happen on directly returning the string.
--
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-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Add color to log lines in UI for error and warnings based on keywords [airflow]
tirkarthi commented on code in PR #39006: URL: https://github.com/apache/airflow/pull/39006#discussion_r1572642067 ## airflow/config_templates/config.yml: ## @@ -979,6 +979,22 @@ logging: type: boolean example: ~ default: "True" +color_log_error_keywords: Review Comment: I don't want to add color names of red to the config option in case it changes before release or so. I guess for me color_log_error_keywords is good enough. -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Add color to log lines in UI for error and warnings based on keywords [airflow]
tirkarthi commented on code in PR #39006:
URL: https://github.com/apache/airflow/pull/39006#discussion_r1572640444
##
airflow/www/static/js/utils/index.ts:
##
@@ -185,6 +185,34 @@ const toSentenceCase = (camelCase: string): string => {
return "";
};
+const addColorKeyword = (
Review Comment:
Thanks added tests and renamed function to `highlightByKeywords` .
--
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-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Add color to log lines in UI for error and warnings based on keywords [airflow]
jscheffl commented on code in PR #39006:
URL: https://github.com/apache/airflow/pull/39006#discussion_r1567847175
##
airflow/www/static/js/utils/index.ts:
##
@@ -185,6 +185,34 @@ const toSentenceCase = (camelCase: string): string => {
return "";
};
+const addColorKeyword = (
Review Comment:
If the function is separated-out into a utility, can you use this
opportunity to add a unit test to airflow/www/static/js/utils/index.test.ts as
well?
##
airflow/config_templates/config.yml:
##
@@ -979,6 +979,22 @@ logging:
type: boolean
example: ~
default: "True"
+color_log_error_keywords:
Review Comment:
Not to be too "picky" but you name the config arguments "error" and
"warning" - mainly because you apply typical red/yellow colors as highlight. So
in most cases this really apply to errors and warnings. And this is common. But
functional wise it is not linked to this. Did you consider naming the config
keys `color_log_red_highlight` and `color_log_yellow_highlight`? (and of course
keeping error/warn as default values) It would be a bit closer to what actually
is configured.
```suggestion
color_log_red_highlight_keywords:
```
##
airflow/www/static/js/utils/index.ts:
##
@@ -185,6 +185,34 @@ const toSentenceCase = (camelCase: string): string => {
return "";
};
+const addColorKeyword = (
+ parsedLine: string,
+ errorKeywords: string[],
+ warningKeywords: string[]
+): string => {
+ const lowerParsedLine = parsedLine.toLowerCase();
+ const containsError = errorKeywords.some((keyword) =>
+lowerParsedLine.includes(keyword)
+ );
+ const bold = (line: string) => `\x1b[1m${line}\x1b[0m`;
+ const red = (line: string) => `\x1b[31m${line}\x1b[39m`;
Review Comment:
As the log might be long and this operation is potentially long-running -
could you merge the bold+[red|yellow] operations into one to save some CPU?
##
airflow/www/static/js/utils/index.ts:
##
@@ -185,6 +185,34 @@ const toSentenceCase = (camelCase: string): string => {
return "";
};
+const addColorKeyword = (
Review Comment:
Alongside, the name of the function might be mis-leading for first-time
readers. Could you rename it to `coloByKeywords()` or `highlightByKeywords()`?
--
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-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Add color to log lines in UI for error and warnings based on keywords [airflow]
bbovenzi commented on code in PR #39006:
URL: https://github.com/apache/airflow/pull/39006#discussion_r1567452482
##
airflow/www/static/js/utils/index.ts:
##
@@ -185,6 +185,34 @@ const toSentenceCase = (camelCase: string): string => {
return "";
};
+const addColorKeyword = (
+ parsedLine: string,
+ errorKeywords: string[],
+ warningKeywords: string[]
+): string => {
+ const lowerParsedLine = parsedLine.toLowerCase();
+ const containsError = errorKeywords.some((keyword) =>
+lowerParsedLine.includes(keyword)
+ );
+ const bold = (line: string) => `\x1b[1m${line}\x1b[0m`;
+ const red = (line: string) => `\x1b[31m${line}\x1b[39m`;
+ const yellow = (line: string) => `\x1b[33m${line}\x1b[39m`;
+
+ if (containsError) {
+return bold(red(parsedLine));
+ }
+
+ const containsWarning = warningKeywords.some((keyword) =>
Review Comment:
Do you mind rearranging these variables a little bit? It was hard to read at
first. Let's keep the containsWarning and containsError declarations near each
other.
--
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-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Add color to log lines in UI for error and warnings based on keywords [airflow]
bbovenzi commented on PR #39006: URL: https://github.com/apache/airflow/pull/39006#issuecomment-2059217203 cc: @jscheffl -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Add color to log lines in UI for error and warnings based on keywords [airflow]
dirrao commented on PR #39006: URL: https://github.com/apache/airflow/pull/39006#issuecomment-2059174505 > @dirrao This is to do string search and it has to process each line to apply color ansi codes. This is already done on whole log file to apply log level class based on info, error, warning etc and to build filters. I haven't benchmarked it but I don't see this adding much overhead. In case of data workloads the logs are too heavy on the client side. I believe test it once with bulky logs to see the obvious issues. -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Add color to log lines in UI for error and warnings based on keywords [airflow]
tirkarthi commented on PR #39006: URL: https://github.com/apache/airflow/pull/39006#issuecomment-2055862566 @dirrao This is to do string search and it has to process each line to apply color ansi codes. This is already done on whole log file to apply log level class based on info, error, warning etc and to build filters. I haven't benchmarked it but I don't see this adding much overhead. -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[PR] Add color to log lines in UI for error and warnings based on keywords [airflow]
tirkarthi opened a new pull request, #39006: URL: https://github.com/apache/airflow/pull/39006 closes: #37443 related: #37443 We have been using this feature in Airflow 2.3.4 in the legacy log page by applying regex against lines to apply color for them. It received positive feedback from users since it's helpful in quickly identifying lines causing error while reading large log fies. This adds the feature to the new log tab in the grid. Users can set additional keywords besides common keywords like error and exception to highlight them since we found cases like "fail", "access denied", "missing" etc from trigger logs emitted by systems outside our Airflow that also were helpful in debugging but maybe not applicable to everyone. This also handles case where the log line already has an ansi code to wrap remaining part of the line with color to avoid conflict. Error and exception lines in red color with bold style  Case where warn keyword is present but it has it's own ansi code to handle nested ansi codes  -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
