Skip to content

Conversation

@ashb
Copy link
Member

@ashb ashb commented Feb 17, 2025

Closes #46657

There are multiple parts to this PR

First off: the log reader interface was a mess There was some odd+old code
do deal with reading from multiple hosts that made the message confusing. This
was added for smart sensors (which went away in v2.4 or v2.5) but this mess
remained, and reading from multiple hosts is handled differently now.

This PR keeps the current "parse+interleave" behaviour (though it's debatable
if the interleave feature is needed specifically, or if we could get away with
a simpler concat instead. Future work there if anyone wants to think about and
tackle this.) but changes the JSON resposne type from a single string (the
value of which was previoulsy a mess of double encoded JSON and repr of a
python tuple making it impossible to do anything but display at) to a list of
either strings (when it can't be parsed) or a list of
dicts/StructuredLogMessage.

I have also done some cursory rendering/displaying of these structured log
messages in the UI, but they could be greatly improved by adding colors to
various components of the log.

The current rendered HTML looks like this:

<p>
    <span class="event">::group::Log message source details</span>
    <span class="log-key sources">sources=["/root/airflow/logs/dag_id=trigger_test/run_id=manual__2025-02-16T12:23:29.118614+00:00_gOTl0Qub/task_id=waiter/attempt=1.log","/root/airflow/logs/dag_id=trigger_test/run_id=manual__2025-02-16T12:23:29.118614+00:00_gOTl0Qub/task_id=waiter/attempt=1.log.trigger.14.log"]</span>
</p>
<p><span class="event">::endgroup::</span></p>
<p>[<time datetime="2025-02-16T12:23:30.033308">2025-02-16T12:23:30.033308</time>] <span class="log-level debug">DEBUG</span> - <span class="event">Hook impls: []</span> <span class="log-key logger">logger="airflow.listeners.listener"</span></p>

Although not used by the UI, the non-application/json content type is now
updated to a) include the continuation token as a header, and to set the
content type as application/x-ndjson

It's not the prettiest yet, I'll leave that to people more talented at that than me

Screenshot 2025-02-17 at 14 58 44

^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@boring-cyborg boring-cyborg bot added area:logging area:UI Related to UI/UX. For Frontend Developers. labels Feb 17, 2025
@ashb
Copy link
Member Author

ashb commented Feb 17, 2025

This is failing it's static check and I don't know how to decipher the error:

/Users/ash/code/airflow/airflow/airflow/ui/src/pages/TaskInstance/Logs/Logs.tsx
   97:21  error  Unsafe assignment of an error typed value  @typescript-eslint/no-unsafe-assignment
  122:27  error  Unsafe assignment of an error typed value  @typescript-eslint/no-unsafe-assignment

/Users/ash/code/airflow/airflow/airflow/ui/src/queries/useLogs.tsx
  43:59  error  Unsafe call of an `error` type typed value             @typescript-eslint/no-unsafe-call
  50:7   error  Unexpected use of continue statement                   no-continue
  60:18  error  Unsafe assignment of an `any` value                    @typescript-eslint/no-unsafe-assignment
  75:5   error  Unsafe assignment of an error typed value              @typescript-eslint/no-unsafe-assignment
  75:19  error  Unsafe call of an `error` type typed value             @typescript-eslint/no-unsafe-call
  75:24  error  Unsafe member access .map on an `error` typed value    @typescript-eslint/no-unsafe-member-access
  78:41  error  Invalid type "unknown" of template literal expression  @typescript-eslint/restrict-template-expressions
  81:14  error  Unsafe assignment of an error typed value              @typescript-eslint/no-unsafe-assignment
  86:5   error  Unsafe assignment of an error typed value              @typescript-eslint/no-unsafe-assignment

@ashb
Copy link
Member Author

ashb commented Feb 17, 2025

I have not tested this with an old/non-json Log, and I haven't fixed up the unit tests yet either.

@bbovenzi
Copy link
Contributor

This is failing it's static check and I don't know how to decipher the error:

/Users/ash/code/airflow/airflow/airflow/ui/src/pages/TaskInstance/Logs/Logs.tsx
   97:21  error  Unsafe assignment of an error typed value  @typescript-eslint/no-unsafe-assignment
  122:27  error  Unsafe assignment of an error typed value  @typescript-eslint/no-unsafe-assignment

/Users/ash/code/airflow/airflow/airflow/ui/src/queries/useLogs.tsx
  43:59  error  Unsafe call of an `error` type typed value             @typescript-eslint/no-unsafe-call
  50:7   error  Unexpected use of continue statement                   no-continue
  60:18  error  Unsafe assignment of an `any` value                    @typescript-eslint/no-unsafe-assignment
  75:5   error  Unsafe assignment of an error typed value              @typescript-eslint/no-unsafe-assignment
  75:19  error  Unsafe call of an `error` type typed value             @typescript-eslint/no-unsafe-call
  75:24  error  Unsafe member access .map on an `error` typed value    @typescript-eslint/no-unsafe-member-access
  78:41  error  Invalid type "unknown" of template literal expression  @typescript-eslint/restrict-template-expressions
  81:14  error  Unsafe assignment of an error typed value              @typescript-eslint/no-unsafe-assignment
  86:5   error  Unsafe assignment of an error typed value              @typescript-eslint/no-unsafe-assignment

It's a bunch of typescript issues. Usually easier to read inline. We we weren't setting the types of the log parsing functions so that made typescript say every variable assignment in the function was unsafe.

@Lee-W Lee-W force-pushed the render-json-logs-new-ui branch 5 times, most recently from 6171d88 to b609dc3 Compare February 20, 2025 09:36
@Lee-W Lee-W added the legacy api Whether legacy API changes should be allowed in PR label Feb 20, 2025
@Lee-W Lee-W force-pushed the render-json-logs-new-ui branch from 527596d to b609dc3 Compare February 20, 2025 09:51
@Lee-W Lee-W added the legacy ui Whether legacy UI change should be allowed in PR label Feb 20, 2025
@Lee-W Lee-W closed this Feb 20, 2025
@Lee-W Lee-W reopened this Feb 20, 2025
@Lee-W Lee-W force-pushed the render-json-logs-new-ui branch 3 times, most recently from 6c41902 to 4bc68d4 Compare February 20, 2025 13:56
@Lee-W
Copy link
Member

Lee-W commented Feb 20, 2025

quick update on the current progress. core tests should already be fixed. some provider ones remaining.

  • providers/amazon/tests/unit/amazon/aws/log/test_cloudwatch_task_handler.py
  • providers/amazon/tests/unit/amazon/aws/log/test_s3_task_handler.py
  • providers/google/tests/unit/google/cloud/log/test_gcs_task_handler.py
  • providers/elasticsearch/tests/unit/elasticsearch/log/test_es_task_handler.py

@Lee-W Lee-W force-pushed the render-json-logs-new-ui branch 3 times, most recently from dd1c677 to a612ff7 Compare February 21, 2025 09:21
@Lee-W
Copy link
Member

Lee-W commented Feb 21, 2025

just add compat code to providers. The tests should pass now... I think...

@Lee-W
Copy link
Member

Lee-W commented Feb 21, 2025

didn't notice there're some others...

  • providers/microsoft/azure/tests/unit/microsoft/azure/log/test_wasb_task_handler.py::TestWasbTaskHandler::test_wasb_read
  • providers/opensearch/tests/unit/opensearch/log/test_os_task_handler.py::TestOpensearchTaskHandler::test_read
  • providers/opensearch/tests/unit/opensearch/log/test_os_task_handler.py::TestOpensearchTaskHandler::test_read_with_patterns
  • providers/opensearch/tests/unit/opensearch/log/test_os_task_handler.py::TestOpensearchTaskHandler::test_read_with_patterns_no_match
  • providers/opensearch/tests/unit/opensearch/log/test_os_task_handler.py::TestOpensearchTaskHandler::test_read_missing_logs
  • providers/opensearch/tests/unit/opensearch/log/test_os_task_handler.py::TestOpensearchTaskHandler::test_read_with_none_metadata
  • providers/redis/tests/unit/redis/log/test_redis_task_handler.py::TestRedisTaskHandler::test_read

@Lee-W Lee-W force-pushed the render-json-logs-new-ui branch from a612ff7 to a416611 Compare February 21, 2025 10:00
@Lee-W Lee-W force-pushed the render-json-logs-new-ui branch from 1495cfd to ba1049e Compare February 26, 2025 10:35
ashb and others added 19 commits February 27, 2025 14:51
There are multiple parts to this PR;

First off: the log reader interface was _a mess_ There was some odd+old code
do deal with reading from multiple hosts that made the message confusing. This
  was added for smart sensors (which went away in v2.4 or v2.5) but this mess
  remained, and reading from multiple hosts is handled differently now.

This PR keeps the current "parse+interleave" behaviour (though it's debatable
if the interleave feature is needed specifically, or if we could get away with
a simpler concat instead. Future work there if anyone wants to think about and
tackle this.) but changes the JSON resposne type from a single string (the
value of which was previoulsy a mess of double encoded JSON and repr of a
python tuple making it impossible to do anything but display at) to a list of
either strings (when it can't be parsed) or a list of
dicts/StructuredLogMessage.

I have also done some cursory rendering/displaying of these structured log
messages in the UI, but they could be greatly improved by adding colors to
various components of the log.

The current rendered HTML looks like this:

```html
<p><span class="event">::group::Log message source details</span> <span class="log-key sources">sources=["/root/airflow/logs/dag_id=trigger_test/run_id=manual__2025-02-16T12:23:29.118614+00:00_gOTl0Qub/task_id=waiter/attempt=1.log","/root/airflow/logs/dag_id=trigger_test/run_id=manual__2025-02-16T12:23:29.118614+00:00_gOTl0Qub/task_id=waiter/attempt=1.log.trigger.14.log"]</span></p>
<p><span class="event">::endgroup::</span></p>
<p>[<time datetime="2025-02-16T12:23:30.033308">2025-02-16T12:23:30.033308</time>] <span class="log-level debug">DEBUG</span> - <span class="event">Hook impls: []</span> <span class="log-key logger">logger="airflow.listeners.listener"</span></p>
```

Although not used by the UI, the non-application/json content type is now
updated to a) include the continuation token as a header, and to set the
content type as application/x-ndjson
@Lee-W Lee-W force-pushed the render-json-logs-new-ui branch from ba1049e to d3e2dff Compare February 27, 2025 06:52
@Lee-W
Copy link
Member

Lee-W commented Feb 27, 2025

As the comments are resolved, I'll merge this one. Thanks all for your help!

@Lee-W Lee-W merged commit aa61371 into main Feb 27, 2025
89 checks passed
@bbovenzi bbovenzi deleted the render-json-logs-new-ui branch February 27, 2025 14:43
ambika-garg pushed a commit to ambika-garg/airflow that referenced this pull request Feb 28, 2025
…ache#46827)

* Render structured logs in the new UI rather than showing raw JSON

There are multiple parts to this PR;

First off: the log reader interface was _a mess_ There was some odd+old code
do deal with reading from multiple hosts that made the message confusing. This
  was added for smart sensors (which went away in v2.4 or v2.5) but this mess
  remained, and reading from multiple hosts is handled differently now.

This PR keeps the current "parse+interleave" behaviour (though it's debatable
if the interleave feature is needed specifically, or if we could get away with
a simpler concat instead. Future work there if anyone wants to think about and
tackle this.) but changes the JSON resposne type from a single string (the
value of which was previoulsy a mess of double encoded JSON and repr of a
python tuple making it impossible to do anything but display at) to a list of
either strings (when it can't be parsed) or a list of
dicts/StructuredLogMessage.

I have also done some cursory rendering/displaying of these structured log
messages in the UI, but they could be greatly improved by adding colors to
various components of the log.

The current rendered HTML looks like this:

```html
<p><span class="event">::group::Log message source details</span> <span class="log-key sources">sources=["/root/airflow/logs/dag_id=trigger_test/run_id=manual__2025-02-16T12:23:29.118614+00:00_gOTl0Qub/task_id=waiter/attempt=1.log","/root/airflow/logs/dag_id=trigger_test/run_id=manual__2025-02-16T12:23:29.118614+00:00_gOTl0Qub/task_id=waiter/attempt=1.log.trigger.14.log"]</span></p>
<p><span class="event">::endgroup::</span></p>
<p>[<time datetime="2025-02-16T12:23:30.033308">2025-02-16T12:23:30.033308</time>] <span class="log-level debug">DEBUG</span> - <span class="event">Hook impls: []</span> <span class="log-key logger">logger="airflow.listeners.listener"</span></p>
```

Although not used by the UI, the non-application/json content type is now
updated to a) include the continuation token as a header, and to set the
content type as application/x-ndjson

* Fix typescript useLogs

* style: group metadata pop

* style: reduce if-else and directly use bool for assigning metadata["download_logs"]

* style: improve type annotation

* test(test_log_reader): fix existing unit tests

* test(api_fastapi): fix existing test_log unit tests

* feat(api_connexion/log): update v1 api to the latest log format

* test(providers/elasticsearch): fix part of the existing unit test

* test(providers/amazon): fix TestCloudwatchTaskHandler::test_read

* feat(providers/amazon): add airflow 3 compat logic

* feat(providers/google): add airflow 3 task handler log handling logic

* feat(providers/elasticsearch): add airflow 3 task handler log handling logic

* feat(providers/microsoft): add airflow 3 task handler log handling logic

* feat(providers/redis): add airflow 3 task handler log handling logic

* feat(providers/opensearch): add airflow 3 task handler log handling logic

* test: ignore unneeded tests

* test(log_handlers): fix pendulum.tz version imcompat

* feat: force StructuredLogMessage check when initialing

---------

Co-authored-by: Brent Bovenzi <brent.bovenzi@gmail.com>
Co-authored-by: Wei Lee <weilee.rx@gmail.com>
nailo2c pushed a commit to nailo2c/airflow that referenced this pull request Apr 4, 2025
…ache#46827)

* Render structured logs in the new UI rather than showing raw JSON

There are multiple parts to this PR;

First off: the log reader interface was _a mess_ There was some odd+old code
do deal with reading from multiple hosts that made the message confusing. This
  was added for smart sensors (which went away in v2.4 or v2.5) but this mess
  remained, and reading from multiple hosts is handled differently now.

This PR keeps the current "parse+interleave" behaviour (though it's debatable
if the interleave feature is needed specifically, or if we could get away with
a simpler concat instead. Future work there if anyone wants to think about and
tackle this.) but changes the JSON resposne type from a single string (the
value of which was previoulsy a mess of double encoded JSON and repr of a
python tuple making it impossible to do anything but display at) to a list of
either strings (when it can't be parsed) or a list of
dicts/StructuredLogMessage.

I have also done some cursory rendering/displaying of these structured log
messages in the UI, but they could be greatly improved by adding colors to
various components of the log.

The current rendered HTML looks like this:

```html
<p><span class="event">::group::Log message source details</span> <span class="log-key sources">sources=["/root/airflow/logs/dag_id=trigger_test/run_id=manual__2025-02-16T12:23:29.118614+00:00_gOTl0Qub/task_id=waiter/attempt=1.log","/root/airflow/logs/dag_id=trigger_test/run_id=manual__2025-02-16T12:23:29.118614+00:00_gOTl0Qub/task_id=waiter/attempt=1.log.trigger.14.log"]</span></p>
<p><span class="event">::endgroup::</span></p>
<p>[<time datetime="2025-02-16T12:23:30.033308">2025-02-16T12:23:30.033308</time>] <span class="log-level debug">DEBUG</span> - <span class="event">Hook impls: []</span> <span class="log-key logger">logger="airflow.listeners.listener"</span></p>
```

Although not used by the UI, the non-application/json content type is now
updated to a) include the continuation token as a header, and to set the
content type as application/x-ndjson

* Fix typescript useLogs

* style: group metadata pop

* style: reduce if-else and directly use bool for assigning metadata["download_logs"]

* style: improve type annotation

* test(test_log_reader): fix existing unit tests

* test(api_fastapi): fix existing test_log unit tests

* feat(api_connexion/log): update v1 api to the latest log format

* test(providers/elasticsearch): fix part of the existing unit test

* test(providers/amazon): fix TestCloudwatchTaskHandler::test_read

* feat(providers/amazon): add airflow 3 compat logic

* feat(providers/google): add airflow 3 task handler log handling logic

* feat(providers/elasticsearch): add airflow 3 task handler log handling logic

* feat(providers/microsoft): add airflow 3 task handler log handling logic

* feat(providers/redis): add airflow 3 task handler log handling logic

* feat(providers/opensearch): add airflow 3 task handler log handling logic

* test: ignore unneeded tests

* test(log_handlers): fix pendulum.tz version imcompat

* feat: force StructuredLogMessage check when initialing

---------

Co-authored-by: Brent Bovenzi <brent.bovenzi@gmail.com>
Co-authored-by: Wei Lee <weilee.rx@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:logging area:UI Related to UI/UX. For Frontend Developers. legacy api Whether legacy API changes should be allowed in PR legacy ui Whether legacy UI change should be allowed in PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

AIP-84 | Get Logs - Improve the formatting of the content response

6 participants