Skip to content

Alias task_display_name for EventLogResponse #55160

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Alias task_display_name for EventLogResponse #55160

wants to merge 2 commits into from
+32 −0

Conversation

guan404ming
Copy link
Contributor

Related Issue

#46730

How

  • alias the task_display_name for EventLogResponse
  • update related tests

^ 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 airflow-core/newsfragments.

@boring-cyborg boring-cyborg bot added area:airflow-ctl area:API Airflow's REST/HTTP API area:UI Related to UI/UX. For Frontend Developers. labels Sep 2, 2025
@guan404ming guan404ming force-pushed the alias-task-for-event-log branch from 660d08e to 5a2d4f4 Compare September 2, 2025 03:22
uranusjr
uranusjr reviewed Sep 2, 2025
View reviewed changes
airflow-core/src/airflow/api_fastapi/core_api/datamodels/event_logs.py
Comment on lines +44 to +46
task_display_name: str | None = Field(
validation_alias=AliasPath("task_instance", "task_display_name"), default=None
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don’t think this can ever be None? TaskInstance always defaults to task_id if the column does not contain a value.

Actually dag_display_name also cannot be None as far as I can tell. Maybe we should fix it too.

Copy link
Contributor Author

@guan404ming guan404ming Sep 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it might be null. Since dag_id and task_idcan also be None here, it means the relationships to dag_model and task_instance can be both None, which indicates that display_name is possible to be None.
However, when they do exist, the display names should always be never None as confirmed by the hybrid property logic like what you said. Please let me know if I've misunderstood anything, thanks!

Copy link
Member

@jason810496 jason810496 Sep 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I get the context correctly, there will be the cases where dag_id and task_id both will be None.

Take delete_asset_queued_events route as example:

airflow/airflow-core/src/airflow/api_fastapi/core_api/routes/public/assets.py

Lines 554 to 563 in c29696c

@assets_router.delete(
"/assets/{asset_id}/queuedEvents",
status_code=status.HTTP_204_NO_CONTENT,
responses=create_openapi_http_exception_doc([status.HTTP_404_NOT_FOUND]),
dependencies=[
Depends(requires_access_asset(method="DELETE")),
Depends(requires_access_dag(method="GET")),
Depends(action_logging()),
],
)

We can't get either dag_id or task_id from the query params and path params.

airflow/airflow-core/src/airflow/api_fastapi/logging/decorators.py

Lines 140 to 150 in c29696c

# Create log entry
log = Log(
event=event_name,
task_instance=None,
owner=user_name,
owner_display_name=user_display,
extra=json.dumps(extra_fields),
task_id=params.get("task_id"),
dag_id=params.get("dag_id"),
run_id=params.get("run_id") or params.get("dag_run_id"),
)

uranusjr
uranusjr reviewed Sep 2, 2025
View reviewed changes
airflow-core/src/airflow/models/log.py Outdated
task_instance = relationship(
"TaskInstance",
viewonly=True,
lazy="selectin",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any specific reason behind this lazy strat?

Copy link
Contributor Author

@guan404ming guan404ming Sep 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I put it for preventing N+1 queries but it seems redundant here. Let me remove it. Thanks for catching this.

@guan404ming guan404ming force-pushed the alias-task-for-event-log branch from 5a2d4f4 to 09e7e2a Compare September 2, 2025 10:07
@guan404ming guan404ming force-pushed the alias-task-for-event-log branch from 09e7e2a to 3c90f03 Compare September 2, 2025 11:09
jason810496
jason810496 reviewed Sep 2, 2025
View reviewed changes
Copy link
Member

@jason810496 jason810496 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Thanks for the PR!

airflow-core/src/airflow/api_fastapi/core_api/datamodels/event_logs.py
Comment on lines +44 to +46
task_display_name: str | None = Field(
validation_alias=AliasPath("task_instance", "task_display_name"), default=None
)
Copy link
Member

@jason810496 jason810496 Sep 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I get the context correctly, there will be the cases where dag_id and task_id both will be None.

Take delete_asset_queued_events route as example:

airflow/airflow-core/src/airflow/api_fastapi/core_api/routes/public/assets.py

Lines 554 to 563 in c29696c

@assets_router.delete(
"/assets/{asset_id}/queuedEvents",
status_code=status.HTTP_204_NO_CONTENT,
responses=create_openapi_http_exception_doc([status.HTTP_404_NOT_FOUND]),
dependencies=[
Depends(requires_access_asset(method="DELETE")),
Depends(requires_access_dag(method="GET")),
Depends(action_logging()),
],
)

We can't get either dag_id or task_id from the query params and path params.

airflow/airflow-core/src/airflow/api_fastapi/logging/decorators.py

Lines 140 to 150 in c29696c

# Create log entry
log = Log(
event=event_name,
task_instance=None,
owner=user_name,
owner_display_name=user_display,
extra=json.dumps(extra_fields),
task_id=params.get("task_id"),
dag_id=params.get("dag_id"),
run_id=params.get("run_id") or params.get("dag_run_id"),
)

pierrejeambrun
pierrejeambrun approved these changes Sep 3, 2025
View reviewed changes
Copy link
Member

@pierrejeambrun pierrejeambrun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. We can merge once other comments are resolved.

jason810496
jason810496 approved these changes Sep 3, 2025
View reviewed changes
Copy link
Member

@jason810496 jason810496 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM as well

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@uranusjr uranusjr uranusjr left review comments

@pierrejeambrun pierrejeambrun pierrejeambrun approved these changes

@jason810496 jason810496 jason810496 approved these changes

@bbovenzi bbovenzi Awaiting requested review from bbovenzi bbovenzi is a code owner

@ryanahamilton ryanahamilton Awaiting requested review from ryanahamilton ryanahamilton is a code owner

@jscheffl jscheffl Awaiting requested review from jscheffl jscheffl is a code owner

@shubhamraj-git shubhamraj-git Awaiting requested review from shubhamraj-git shubhamraj-git is a code owner

@bugraoz93 bugraoz93 Awaiting requested review from bugraoz93 bugraoz93 is a code owner

@kaxil kaxil Awaiting requested review from kaxil kaxil is a code owner

@potiuk potiuk Awaiting requested review from potiuk potiuk is a code owner

@ephraimbuddy ephraimbuddy Awaiting requested review from ephraimbuddy ephraimbuddy is a code owner

@rawwar rawwar Awaiting requested review from rawwar rawwar is a code owner

@XD-DENG XD-DENG Awaiting requested review from XD-DENG XD-DENG is a code owner

@ashb ashb Awaiting requested review from ashb ashb is a code owner

Assignees
No one assigned
Labels
area:airflow-ctl area:API Airflow's REST/HTTP API area:UI Related to UI/UX. For Frontend Developers.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@guan404ming @uranusjr @pierrejeambrun @jason810496