You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "ashb (via GitHub)" <gi...@apache.org> on 2025/02/17 14:46:14 UTC
[PR] Render structured logs in the new UI rather than showing raw JSON [airflow]
ashb opened a new pull request, #46827:
URL: https://github.com/apache/airflow/pull/46827
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
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of an existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
<!-- Please keep an empty line above the dashes. -->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#pull-request-guidelines)** for more information.
In case of fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+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 newsfragment file, named `{pr_number}.significant.rst` or `{issue_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
Re: [PR] Render structured logs in the new UI rather than showing raw JSON [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #46827:
URL: https://github.com/apache/airflow/pull/46827#discussion_r1967892745
##########
airflow/api_fastapi/core_api/datamodels/log.py:
##########
@@ -16,11 +16,29 @@
# under the License.
from __future__ import annotations
-from pydantic import BaseModel
+from datetime import datetime
+from typing import Annotated
+
+from pydantic import BaseModel, ConfigDict, WithJsonSchema
+
+
+class StructuredLogMessage(BaseModel):
+ """An individual log message."""
+
+ # Not every message has a timestamp.
+ timestamp: Annotated[
+ datetime | None,
+ # Schema level, say this is always a datetime if it exists
+ WithJsonSchema({"type": "string", "format": "date-time"}),
Review Comment:
> The risk of it getting out of sync is minimal -- it's on the next line. Yes, if the code tried to send a None it would return an error.
Ok, sounds good.
> Such as?
Can't remember exactly the datamodel that needed it, but I'm positive we have encountered this. (I would have to check all the model to remember, It happened only once or twice).
> Why did you choose not to override it?
Simply adding a null check in the front-end wasn't too expensive, but I agree that this is a more accurate representation. It's just that having to manually specify the spec for those fields that are `undefined | actual_type` is annoying. (especially if the type is another Datamodel / not a primitive type).
That's good for me if you think that's preferable.
--
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
Re: [PR] Render structured logs in the new UI rather than showing raw JSON [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #46827:
URL: https://github.com/apache/airflow/pull/46827#discussion_r1969428822
##########
providers/elasticsearch/src/airflow/providers/elasticsearch/log/es_task_handler.py:
##########
@@ -358,12 +371,31 @@ def _read(
# If we hit the end of the log, remove the actual end_of_log message
# to prevent it from showing in the UI.
- def concat_logs(hits: list[Hit]):
+ def concat_logs(hits: list[Hit]) -> str:
log_range = (len(hits) - 1) if hits[-1].message == self.end_of_log_mark else len(hits)
return "\n".join(self._format_msg(hits[i]) for i in range(log_range))
if logs_by_host:
- message = [(host, concat_logs(hits)) for host, hits in logs_by_host.items()]
+ if AIRFLOW_V_3_0_PLUS:
+ from airflow.utils.log.file_task_handler import StructuredLogMessage
+
+ header = [
+ StructuredLogMessage.model_construct(
Review Comment:
I think it's worth confirming with Ash that this is intentional. There are other places I see why we do that, but for the header I'm not sure. Maybe it's because of the `sources` but since the model allow `extras` it shouldn't be a problem to validate.
Otherwise looks good to me.
--
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
Re: [PR] Render structured logs in the new UI rather than showing raw JSON [airflow]
Posted by "Lee-W (via GitHub)" <gi...@apache.org>.
Lee-W commented on code in PR #46827:
URL: https://github.com/apache/airflow/pull/46827#discussion_r1969553500
##########
providers/elasticsearch/src/airflow/providers/elasticsearch/log/es_task_handler.py:
##########
@@ -358,12 +371,31 @@ def _read(
# If we hit the end of the log, remove the actual end_of_log message
# to prevent it from showing in the UI.
- def concat_logs(hits: list[Hit]):
+ def concat_logs(hits: list[Hit]) -> str:
log_range = (len(hits) - 1) if hits[-1].message == self.end_of_log_mark else len(hits)
return "\n".join(self._format_msg(hits[i]) for i in range(log_range))
if logs_by_host:
- message = [(host, concat_logs(hits)) for host, hits in logs_by_host.items()]
+ if AIRFLOW_V_3_0_PLUS:
+ from airflow.utils.log.file_task_handler import StructuredLogMessage
+
+ header = [
+ StructuredLogMessage.model_construct(
Review Comment:
@pierrejeambrun would like to confirm whether we want to make this change. Either way works for me. I can make this change later 🙂
--
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
Re: [PR] Render structured logs in the new UI rather than showing raw JSON [airflow]
Posted by "Lee-W (via GitHub)" <gi...@apache.org>.
Lee-W commented on code in PR #46827:
URL: https://github.com/apache/airflow/pull/46827#discussion_r1971372551
##########
providers/elasticsearch/src/airflow/providers/elasticsearch/log/es_task_handler.py:
##########
@@ -358,12 +371,31 @@ def _read(
# If we hit the end of the log, remove the actual end_of_log message
# to prevent it from showing in the UI.
- def concat_logs(hits: list[Hit]):
+ def concat_logs(hits: list[Hit]) -> str:
log_range = (len(hits) - 1) if hits[-1].message == self.end_of_log_mark else len(hits)
return "\n".join(self._format_msg(hits[i]) for i in range(log_range))
if logs_by_host:
- message = [(host, concat_logs(hits)) for host, hits in logs_by_host.items()]
+ if AIRFLOW_V_3_0_PLUS:
+ from airflow.utils.log.file_task_handler import StructuredLogMessage
+
+ header = [
+ StructuredLogMessage.model_construct(
Review Comment:
@pierrejeambrun just updated it. could you please take a look if you have some bandwidth? Many thanks!
--
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
Re: [PR] Render structured logs in the new UI rather than showing raw JSON [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #46827:
URL: https://github.com/apache/airflow/pull/46827#discussion_r1967892745
##########
airflow/api_fastapi/core_api/datamodels/log.py:
##########
@@ -16,11 +16,29 @@
# under the License.
from __future__ import annotations
-from pydantic import BaseModel
+from datetime import datetime
+from typing import Annotated
+
+from pydantic import BaseModel, ConfigDict, WithJsonSchema
+
+
+class StructuredLogMessage(BaseModel):
+ """An individual log message."""
+
+ # Not every message has a timestamp.
+ timestamp: Annotated[
+ datetime | None,
+ # Schema level, say this is always a datetime if it exists
+ WithJsonSchema({"type": "string", "format": "date-time"}),
Review Comment:
> The risk of it getting out of sync is minimal -- it's on the next line. Yes, if the code tried to send a None it would return an error.
Ok, sounds good.
> Such as?
Can't remember exactly the datamodel that needed it, but I'm positive we have encountered this. (I would have to check all the models to remember, It happened only once or twice).
> Why did you choose not to override it?
Simply adding a null check in the front-end wasn't too expensive, but I agree that this is a more accurate representation. It's just that having to manually specify the spec for those fields that are `undefined | actual_type` is annoying. (especially if the type is another Datamodel / not a primitive type).
That's good for me if you think that's preferable.
--
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
Re: [PR] Render structured logs in the new UI rather than showing raw JSON [airflow]
Posted by "Lee-W (via GitHub)" <gi...@apache.org>.
Lee-W commented on PR #46827:
URL: https://github.com/apache/airflow/pull/46827#issuecomment-2671576627
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
--
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
Re: [PR] Render structured logs in the new UI rather than showing raw JSON [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #46827:
URL: https://github.com/apache/airflow/pull/46827#discussion_r1967892745
##########
airflow/api_fastapi/core_api/datamodels/log.py:
##########
@@ -16,11 +16,29 @@
# under the License.
from __future__ import annotations
-from pydantic import BaseModel
+from datetime import datetime
+from typing import Annotated
+
+from pydantic import BaseModel, ConfigDict, WithJsonSchema
+
+
+class StructuredLogMessage(BaseModel):
+ """An individual log message."""
+
+ # Not every message has a timestamp.
+ timestamp: Annotated[
+ datetime | None,
+ # Schema level, say this is always a datetime if it exists
+ WithJsonSchema({"type": "string", "format": "date-time"}),
Review Comment:
> The risk of it getting out of sync is minimal -- it's on the next line. Yes, if the code tried to send a None it would return an error.
Ok, sounds good.
> Such as?
Can't remember exactly the datamodel that needed it, but I'm positive we have encountered this. (I would have to check all the models to remember, It happened only once or twice).
> Why did you choose not to override it?
Simply adding a null check in the front-end wasn't too expensive, but I agree that this is a more accurate representation. It's just that having to manually specify the spec for those fields that are `undefined | actual_type` is annoying. (especially if the type is another Datamodel / not a primitive type). Also I wasn't sure if that `WithJsonSchema` would actually enforce data validation or if it was just for the spec file generation. (but you answered that)
That's good for me if you think that's preferable.
--
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
Re: [PR] Render structured logs in the new UI rather than showing raw JSON [airflow]
Posted by "Lee-W (via GitHub)" <gi...@apache.org>.
Lee-W commented on PR #46827:
URL: https://github.com/apache/airflow/pull/46827#issuecomment-2687592570
As the comments are resolved, I'll merge this one. Thanks all for your help!
--
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
Re: [PR] Render structured logs in the new UI rather than showing raw JSON [airflow]
Posted by "Lee-W (via GitHub)" <gi...@apache.org>.
Lee-W merged PR #46827:
URL: https://github.com/apache/airflow/pull/46827
--
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
Re: [PR] Render structured logs in the new UI rather than showing raw JSON [airflow]
Posted by "ashb (via GitHub)" <gi...@apache.org>.
ashb commented on code in PR #46827:
URL: https://github.com/apache/airflow/pull/46827#discussion_r1969538255
##########
providers/elasticsearch/src/airflow/providers/elasticsearch/log/es_task_handler.py:
##########
@@ -358,12 +371,31 @@ def _read(
# If we hit the end of the log, remove the actual end_of_log message
# to prevent it from showing in the UI.
- def concat_logs(hits: list[Hit]):
+ def concat_logs(hits: list[Hit]) -> str:
log_range = (len(hits) - 1) if hits[-1].message == self.end_of_log_mark else len(hits)
return "\n".join(self._format_msg(hits[i]) for i in range(log_range))
if logs_by_host:
- message = [(host, concat_logs(hits)) for host, hits in logs_by_host.items()]
+ if AIRFLOW_V_3_0_PLUS:
+ from airflow.utils.log.file_task_handler import StructuredLogMessage
+
+ header = [
+ StructuredLogMessage.model_construct(
Review Comment:
Likely not required, I was just thinking "why bother running validation if we know we don't need to". Happy to change it either way
--
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
Re: [PR] Render structured logs in the new UI rather than showing raw JSON [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #46827:
URL: https://github.com/apache/airflow/pull/46827#discussion_r1967892745
##########
airflow/api_fastapi/core_api/datamodels/log.py:
##########
@@ -16,11 +16,29 @@
# under the License.
from __future__ import annotations
-from pydantic import BaseModel
+from datetime import datetime
+from typing import Annotated
+
+from pydantic import BaseModel, ConfigDict, WithJsonSchema
+
+
+class StructuredLogMessage(BaseModel):
+ """An individual log message."""
+
+ # Not every message has a timestamp.
+ timestamp: Annotated[
+ datetime | None,
+ # Schema level, say this is always a datetime if it exists
+ WithJsonSchema({"type": "string", "format": "date-time"}),
Review Comment:
> The risk of it getting out of sync is minimal -- it's on the next line. Yes, if the code tried to send a None it would return an error.
Ok, sounds good.
> Such as?
Can't remember exactly the datamodel that needed, but I'm sure we have encountered this. (I would have to check all the model to remember, It happened only once or twice).
> Why did you choose not to override it?
Simply adding a null check in the front-end wasn't too expensive, but I agree that this is a more accurate representation. It's just that having to manually specify the spec for those fields that are `undefined | actual_type` is annoying. (especially if the type is another Datamodel / not a primitive type).
That's good for me if you think that's preferable.
##########
airflow/api_fastapi/core_api/datamodels/log.py:
##########
@@ -16,11 +16,29 @@
# under the License.
from __future__ import annotations
-from pydantic import BaseModel
+from datetime import datetime
+from typing import Annotated
+
+from pydantic import BaseModel, ConfigDict, WithJsonSchema
+
+
+class StructuredLogMessage(BaseModel):
+ """An individual log message."""
+
+ # Not every message has a timestamp.
+ timestamp: Annotated[
+ datetime | None,
+ # Schema level, say this is always a datetime if it exists
+ WithJsonSchema({"type": "string", "format": "date-time"}),
Review Comment:
> The risk of it getting out of sync is minimal -- it's on the next line. Yes, if the code tried to send a None it would return an error.
Ok, sounds good.
> Such as?
Can't remember exactly the datamodel that needed, but I'm sure we have encountered this. (I would have to check all the model to remember, It happened only once or twice).
> Why did you choose not to override it?
Simply adding a null check in the front-end wasn't too expensive, but I agree that this is a more accurate representation. It's just that having to manually specify the spec for those fields that are `undefined | actual_type` is annoying. (especially if the type is another Datamodel / not a primitive type).
That's good for me if you think that's preferable.
--
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
Re: [PR] Render structured logs in the new UI rather than showing raw JSON [airflow]
Posted by "Lee-W (via GitHub)" <gi...@apache.org>.
Lee-W closed pull request #46827: Render structured logs in the new UI rather than showing raw JSON
URL: https://github.com/apache/airflow/pull/46827
--
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
Re: [PR] Render structured logs in the new UI rather than showing raw JSON [airflow]
Posted by "ashb (via GitHub)" <gi...@apache.org>.
ashb commented on code in PR #46827:
URL: https://github.com/apache/airflow/pull/46827#discussion_r1967701783
##########
airflow/api_fastapi/core_api/datamodels/log.py:
##########
@@ -16,11 +16,29 @@
# under the License.
from __future__ import annotations
-from pydantic import BaseModel
+from datetime import datetime
+from typing import Annotated
+
+from pydantic import BaseModel, ConfigDict, WithJsonSchema
+
+
+class StructuredLogMessage(BaseModel):
+ """An individual log message."""
+
+ # Not every message has a timestamp.
+ timestamp: Annotated[
+ datetime | None,
+ # Schema level, say this is always a datetime if it exists
+ WithJsonSchema({"type": "string", "format": "date-time"}),
Review Comment:
This `WithJsonSchema` makes the schema an accurate representation of what we actually send is why.
`{"timestamp": "2025-02-24T14:04:03"}`
`{}`
are both allowed but
`{"timestamp": null}`
is not.
The risk of it getting out of sync is minimal -- it's on the next line. Yes, if the code tried to send a None it would return an error.
> Anyway we needed that at other places and we chose not to manually override the generated json.
Such as? Why did you choose not to override it?
--
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
Re: [PR] Render structured logs in the new UI rather than showing raw JSON [airflow]
Posted by "Lee-W (via GitHub)" <gi...@apache.org>.
Lee-W commented on PR #46827:
URL: https://github.com/apache/airflow/pull/46827#issuecomment-2674096070
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
--
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
Re: [PR] Render structured logs in the new UI rather than showing raw JSON [airflow]
Posted by "ashb (via GitHub)" <gi...@apache.org>.
ashb commented on code in PR #46827:
URL: https://github.com/apache/airflow/pull/46827#discussion_r1967430430
##########
airflow/api_fastapi/core_api/datamodels/log.py:
##########
@@ -16,11 +16,29 @@
# under the License.
from __future__ import annotations
-from pydantic import BaseModel
+from datetime import datetime
+from typing import Annotated
+
+from pydantic import BaseModel, ConfigDict, WithJsonSchema
+
+
+class StructuredLogMessage(BaseModel):
+ """An individual log message."""
+
+ # Not every message has a timestamp.
+ timestamp: Annotated[
+ datetime | None,
+ # Schema level, say this is always a datetime if it exists
+ WithJsonSchema({"type": "string", "format": "date-time"}),
Review Comment:
This is to not include the `None` option as a possible value to the client. At the python level this is either a datetime, or one. But when we serialize this, if it's present it's always a datetime.
So the override is needed so that the JS doesn't see the properly as possibly nullable. A subtle distinction but a distinction none the less.
--
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
Re: [PR] Render structured logs in the new UI rather than showing raw JSON [airflow]
Posted by "ashb (via GitHub)" <gi...@apache.org>.
ashb commented on PR #46827:
URL: https://github.com/apache/airflow/pull/46827#issuecomment-2663407290
I have not tested this with an old/non-json Log, and I haven't fixed up the unit tests yet either.
--
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
Re: [PR] Render structured logs in the new UI rather than showing raw JSON [airflow]
Posted by "Lee-W (via GitHub)" <gi...@apache.org>.
Lee-W commented on PR #46827:
URL: https://github.com/apache/airflow/pull/46827#issuecomment-2674016883
just add compat code to providers. The tests should pass now... I think...
--
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
Re: [PR] Render structured logs in the new UI rather than showing raw JSON [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #46827:
URL: https://github.com/apache/airflow/pull/46827#discussion_r1967533273
##########
airflow/api_fastapi/core_api/datamodels/log.py:
##########
@@ -16,11 +16,29 @@
# under the License.
from __future__ import annotations
-from pydantic import BaseModel
+from datetime import datetime
+from typing import Annotated
+
+from pydantic import BaseModel, ConfigDict, WithJsonSchema
+
+
+class StructuredLogMessage(BaseModel):
+ """An individual log message."""
+
+ # Not every message has a timestamp.
+ timestamp: Annotated[
+ datetime | None,
+ # Schema level, say this is always a datetime if it exists
+ WithJsonSchema({"type": "string", "format": "date-time"}),
Review Comment:
like the key could be missing from the object ? Is that the use case ?
Yes unfortunately we do not have an easy way to specify that. Maybe we need two distinct models…
Anyway we needed that at other places and we chose not to manually override the generated json.
I’m not strong on that but I would recommend we don’t do it here as well. At least for consistency, also because null check on the front end is a one liner anyway (and most of the time the same check as undefined). And finally because the code and the spec could get out of sync by manually doing that. What happens if the code actually returns None ? Does that raises a 422 or return a response not in line with the spec ?
--
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
Re: [PR] Render structured logs in the new UI rather than showing raw JSON [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #46827:
URL: https://github.com/apache/airflow/pull/46827#discussion_r1967301350
##########
providers/elasticsearch/src/airflow/providers/elasticsearch/log/es_task_handler.py:
##########
@@ -358,12 +371,31 @@ def _read(
# If we hit the end of the log, remove the actual end_of_log message
# to prevent it from showing in the UI.
- def concat_logs(hits: list[Hit]):
+ def concat_logs(hits: list[Hit]) -> str:
log_range = (len(hits) - 1) if hits[-1].message == self.end_of_log_mark else len(hits)
return "\n".join(self._format_msg(hits[i]) for i in range(log_range))
if logs_by_host:
- message = [(host, concat_logs(hits)) for host, hits in logs_by_host.items()]
+ if AIRFLOW_V_3_0_PLUS:
+ from airflow.utils.log.file_task_handler import StructuredLogMessage
+
+ header = [
+ StructuredLogMessage.model_construct(
Review Comment:
Why are we skipping the validation whe instanciating `StructuredLogMessage` ?
##########
airflow/api_fastapi/core_api/datamodels/log.py:
##########
@@ -16,11 +16,29 @@
# under the License.
from __future__ import annotations
-from pydantic import BaseModel
+from datetime import datetime
+from typing import Annotated
+
+from pydantic import BaseModel, ConfigDict, WithJsonSchema
+
+
+class StructuredLogMessage(BaseModel):
+ """An individual log message."""
+
+ # Not every message has a timestamp.
+ timestamp: Annotated[
+ datetime | None,
+ # Schema level, say this is always a datetime if it exists
+ WithJsonSchema({"type": "string", "format": "date-time"}),
Review Comment:
I'm not sure the `WithJsonSchema` is necessary. This is automatically generated when the type is `datetime` in the final openapi spec file.
--
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
Re: [PR] Render structured logs in the new UI rather than showing raw JSON [airflow]
Posted by "Lee-W (via GitHub)" <gi...@apache.org>.
Lee-W commented on PR #46827:
URL: https://github.com/apache/airflow/pull/46827#issuecomment-2674210997
fixed the remaining ones!
--
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
Re: [PR] Render structured logs in the new UI rather than showing raw JSON [airflow]
Posted by "Lee-W (via GitHub)" <gi...@apache.org>.
Lee-W commented on PR #46827:
URL: https://github.com/apache/airflow/pull/46827#issuecomment-2677243567
all green!
--
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
Re: [PR] Render structured logs in the new UI rather than showing raw JSON [airflow]
Posted by "ashb (via GitHub)" <gi...@apache.org>.
ashb commented on PR #46827:
URL: https://github.com/apache/airflow/pull/46827#issuecomment-2663337710
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
```
--
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
Re: [PR] Render structured logs in the new UI rather than showing raw JSON [airflow]
Posted by "bbovenzi (via GitHub)" <gi...@apache.org>.
bbovenzi commented on PR #46827:
URL: https://github.com/apache/airflow/pull/46827#issuecomment-2664193589
> 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.
--
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
Re: [PR] Render structured logs in the new UI rather than showing raw JSON [airflow]
Posted by "Lee-W (via GitHub)" <gi...@apache.org>.
Lee-W commented on code in PR #46827:
URL: https://github.com/apache/airflow/pull/46827#discussion_r1968742573
##########
providers/elasticsearch/src/airflow/providers/elasticsearch/log/es_task_handler.py:
##########
@@ -358,12 +371,31 @@ def _read(
# If we hit the end of the log, remove the actual end_of_log message
# to prevent it from showing in the UI.
- def concat_logs(hits: list[Hit]):
+ def concat_logs(hits: list[Hit]) -> str:
log_range = (len(hits) - 1) if hits[-1].message == self.end_of_log_mark else len(hits)
return "\n".join(self._format_msg(hits[i]) for i in range(log_range))
if logs_by_host:
- message = [(host, concat_logs(hits)) for host, hits in logs_by_host.items()]
+ if AIRFLOW_V_3_0_PLUS:
+ from airflow.utils.log.file_task_handler import StructuredLogMessage
+
+ header = [
+ StructuredLogMessage.model_construct(
Review Comment:
I guess https://github.com/apache/airflow/pull/46827/files#diff-e6117e0d0a37880e15b0615aa1f1f936f7e85b61e915e68f2ca0c56073f79a92R28 might be the reason?
--
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
Re: [PR] Render structured logs in the new UI rather than showing raw JSON [airflow]
Posted by "jason810496 (via GitHub)" <gi...@apache.org>.
jason810496 commented on code in PR #46827:
URL: https://github.com/apache/airflow/pull/46827#discussion_r1959990074
##########
airflow/api_fastapi/core_api/routes/public/log.py:
##########
@@ -135,12 +135,22 @@ def get_log(
except TaskNotFound:
pass
- logs: Any
if accept == Mimetype.JSON or accept == Mimetype.ANY: # default
logs, metadata = task_log_reader.read_log_chunks(ti, try_number, metadata)
- # we must have token here, so we can safely ignore it
- token = URLSafeSerializer(request.app.state.secret_key).dumps(metadata) # type: ignore[assignment]
- return TaskInstancesLogResponse(continuation_token=token, content=str(logs[0])).model_dump()
- # text/plain. Stream
- logs = task_log_reader.read_log_stream(ti, try_number, metadata)
- return Response(media_type=accept, content="".join(list(logs)))
+ encoded_token = None
+ if not metadata.get("end_of_log", False):
+ encoded_token = URLSafeSerializer(request.app.state.secret_key).dumps(metadata)
+ return TaskInstancesLogResponse.model_construct(continuation_token=encoded_token, content=logs)
Review Comment:
Not a blocker, but I thought we were also planning to move `continuation_token` to the header for the `application/json` response.
--
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
Re: [PR] Render structured logs in the new UI rather than showing raw JSON [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #46827:
URL: https://github.com/apache/airflow/pull/46827#discussion_r1967533273
##########
airflow/api_fastapi/core_api/datamodels/log.py:
##########
@@ -16,11 +16,29 @@
# under the License.
from __future__ import annotations
-from pydantic import BaseModel
+from datetime import datetime
+from typing import Annotated
+
+from pydantic import BaseModel, ConfigDict, WithJsonSchema
+
+
+class StructuredLogMessage(BaseModel):
+ """An individual log message."""
+
+ # Not every message has a timestamp.
+ timestamp: Annotated[
+ datetime | None,
+ # Schema level, say this is always a datetime if it exists
+ WithJsonSchema({"type": "string", "format": "date-time"}),
Review Comment:
like the key could be missing from the object ? Is that the use case ?
Yes unfortunately we do not have an easy way to specify that. Maybe we need two distinct models…
Anyway we needed that at other places and we chose not to manually override the generated json.
I’m not strong on that but I would recommend we don’t do it here as well. At least for consistency, also because. null check on the front end is a one liner anyway. And finally because the code and the spec could get out of sync by manually doing that. What happens if the code actually return 200 ? Does that raises a 422 or return a response not in line with the spec ?
--
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
Re: [PR] Render structured logs in the new UI rather than showing raw JSON [airflow]
Posted by "Lee-W (via GitHub)" <gi...@apache.org>.
Lee-W commented on code in PR #46827:
URL: https://github.com/apache/airflow/pull/46827#discussion_r1969374999
##########
providers/elasticsearch/src/airflow/providers/elasticsearch/log/es_task_handler.py:
##########
@@ -358,12 +371,31 @@ def _read(
# If we hit the end of the log, remove the actual end_of_log message
# to prevent it from showing in the UI.
- def concat_logs(hits: list[Hit]):
+ def concat_logs(hits: list[Hit]) -> str:
log_range = (len(hits) - 1) if hits[-1].message == self.end_of_log_mark else len(hits)
return "\n".join(self._format_msg(hits[i]) for i in range(log_range))
if logs_by_host:
- message = [(host, concat_logs(hits)) for host, hits in logs_by_host.items()]
+ if AIRFLOW_V_3_0_PLUS:
+ from airflow.utils.log.file_task_handler import StructuredLogMessage
+
+ header = [
+ StructuredLogMessage.model_construct(
Review Comment:
@pierrejeambrun WDYT? Should we merge it, or should we change this one? Thanks!
--
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
Re: [PR] Render structured logs in the new UI rather than showing raw JSON [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #46827:
URL: https://github.com/apache/airflow/pull/46827#discussion_r1969711046
##########
providers/elasticsearch/src/airflow/providers/elasticsearch/log/es_task_handler.py:
##########
@@ -358,12 +371,31 @@ def _read(
# If we hit the end of the log, remove the actual end_of_log message
# to prevent it from showing in the UI.
- def concat_logs(hits: list[Hit]):
+ def concat_logs(hits: list[Hit]) -> str:
log_range = (len(hits) - 1) if hits[-1].message == self.end_of_log_mark else len(hits)
return "\n".join(self._format_msg(hits[i]) for i in range(log_range))
if logs_by_host:
- message = [(host, concat_logs(hits)) for host, hits in logs_by_host.items()]
+ if AIRFLOW_V_3_0_PLUS:
+ from airflow.utils.log.file_task_handler import StructuredLogMessage
+
+ header = [
+ StructuredLogMessage.model_construct(
Review Comment:
Not strong about it. I think changing it doesn't hurt and is safer
--
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
Re: [PR] Render structured logs in the new UI rather than showing raw JSON [airflow]
Posted by "Lee-W (via GitHub)" <gi...@apache.org>.
Lee-W commented on code in PR #46827:
URL: https://github.com/apache/airflow/pull/46827#discussion_r1969712749
##########
providers/elasticsearch/src/airflow/providers/elasticsearch/log/es_task_handler.py:
##########
@@ -358,12 +371,31 @@ def _read(
# If we hit the end of the log, remove the actual end_of_log message
# to prevent it from showing in the UI.
- def concat_logs(hits: list[Hit]):
+ def concat_logs(hits: list[Hit]) -> str:
log_range = (len(hits) - 1) if hits[-1].message == self.end_of_log_mark else len(hits)
return "\n".join(self._format_msg(hits[i]) for i in range(log_range))
if logs_by_host:
- message = [(host, concat_logs(hits)) for host, hits in logs_by_host.items()]
+ if AIRFLOW_V_3_0_PLUS:
+ from airflow.utils.log.file_task_handler import StructuredLogMessage
+
+ header = [
+ StructuredLogMessage.model_construct(
Review Comment:
Got it. Sounds good. đź‘Ť Will continue work on it.
--
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
Re: [PR] Render structured logs in the new UI rather than showing raw JSON [airflow]
Posted by "Lee-W (via GitHub)" <gi...@apache.org>.
Lee-W commented on code in PR #46827:
URL: https://github.com/apache/airflow/pull/46827#discussion_r1967893170
##########
providers/elasticsearch/src/airflow/providers/elasticsearch/log/es_task_handler.py:
##########
@@ -358,12 +371,31 @@ def _read(
# If we hit the end of the log, remove the actual end_of_log message
# to prevent it from showing in the UI.
- def concat_logs(hits: list[Hit]):
+ def concat_logs(hits: list[Hit]) -> str:
log_range = (len(hits) - 1) if hits[-1].message == self.end_of_log_mark else len(hits)
return "\n".join(self._format_msg(hits[i]) for i in range(log_range))
if logs_by_host:
- message = [(host, concat_logs(hits)) for host, hits in logs_by_host.items()]
+ if AIRFLOW_V_3_0_PLUS:
+ from airflow.utils.log.file_task_handler import StructuredLogMessage
+
+ header = [
+ StructuredLogMessage.model_construct(
Review Comment:
This basically follows https://github.com/apache/airflow/pull/46827/files#diff-e7f34f73940eb52d92bb991abedc1c963431c5373c12dff739c8fb7d03e93d3aR417. Would like to confirm with @ashb whether there's any reason we're doing it. it not we probably could go with `StructuredLogMessage(...)`?
--
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