Skip to content
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.

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

Adding run_id column to log table #37731

Merged
merged 1 commit into from Mar 5, 2024

Conversation

SamWheating
Copy link
Contributor

@SamWheating SamWheating commented Feb 26, 2024

closes: #37597

This adds the run_id column to the log table (without a backfill, see discussion) and exposes it in all of the different surfaces:

  • the /eventLogs API
  • the logs view (/logs/list)
  • the inset / react audit logs DagRun details page
  • the legacy (non-react) audit logs view in the DAG details page

Screenshots:

Dag-scoped audit logs:
image

Dagrun-scoped audit logs:
image

Open questions:

  • Should we be pre-populating the run_id filter in the inset audit logs view (similar to what we do with task_id when a single task is selected)? This would be more precise than just filtering on task_id + dates, but it would exclude relevant events which aren't tagged with a run_id (for example, pausing a DAG mid-execution)

@boring-cyborg boring-cyborg bot added area:API Airflow's REST/HTTP API area:UI Related to UI/UX. For Frontend Developers. area:webserver Webserver related Issues kind:documentation labels Feb 26, 2024
@SamWheating SamWheating force-pushed the sw-add-run-id-to-log branch 2 times, most recently from 6656f1e to b04aa58 Compare February 27, 2024 00:26
@SamWheating SamWheating marked this pull request as ready for review February 27, 2024 00:27
@SamWheating SamWheating changed the title WIP: adding run_id column to log table Adding run_id column to log table Feb 27, 2024
@bbovenzi
Copy link
Contributor

bbovenzi commented Feb 27, 2024

I think prepopulating the runId filter works. After this I will open up the before/after fields to be user-editable but with some default options like a dag run's entire durations. The before/after range doesn't capture changing the state or a note. so either approach has pros and cons

Also, after this and #37734. We probably want to make sure we are populating run_id everywhere we can.

Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

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

LGTM . But would love another maintainer's review

@potiuk
Copy link
Member

potiuk commented Feb 27, 2024

Just a watch-out - we have two migrations in-progress. I think we have a protection that will make the secon one not mergable if the first is but there will be some re-work needed once the other mighration is merged #37708

@jedcunningham
Copy link
Member

We actually have 3 in flight 😅: #37734

@potiuk
Copy link
Member

potiuk commented Feb 27, 2024

We actually have 3 in flight 😅: #37734

😱 😱 😱 😱 😱 😱 😱 😱 😱

@SamWheating
Copy link
Contributor Author

Just a watch-out - we have two migrations in-progress

Thanks for the heads up - I'll keep a close 👀 on the other two and try to keep this PR in a mergeable state.

@bbovenzi
Copy link
Contributor

We actually have 3 in flight 😅: #37734

Since we have two migrations touching Log. Should we do both at once?

@SamWheating
Copy link
Contributor Author

Since we have two migrations touching Log. Should we do both at once?

This is a pretty good idea. Which PR do you want to merge first? If needed I can port the DB changes from the other PR into this one.

@bbovenzi
Copy link
Contributor

Since we have two migrations touching Log. Should we do both at once?

This is a pretty good idea. Which PR do you want to merge first? If needed I can port the DB changes from the other PR into this one.

The other Audit Log one still has errors in the CI. So let's merge this first. The only change I had to make over there is to increase the max size of the event string.

@SamWheating
Copy link
Contributor Author

SamWheating commented Feb 29, 2024

Looks like the other TaskInstance migration PR was merged, I'll get this one updated.

@SamWheating SamWheating force-pushed the sw-add-run-id-to-log branch 2 times, most recently from 9f2400a to 45e11e4 Compare February 29, 2024 18:03
@bbovenzi
Copy link
Contributor

bbovenzi commented Mar 4, 2024

Since we have two migrations touching Log. Should we do both at once?

@jedcunningham @potiuk What do you prefer? Two separate event log table migrations, or combine them?

Copy link
Contributor

@bbovenzi bbovenzi left a comment

Choose a reason for hiding this comment

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

lgtm. We'll merge the two migrations in the second PR

@bbovenzi bbovenzi added this to the Airflow 2.9.0 milestone Mar 5, 2024
@bbovenzi bbovenzi merged commit c41d194 into apache:main Mar 5, 2024
59 checks passed
@utkarsharma2 utkarsharma2 added the type:improvement Changelog: Improvements label Mar 6, 2024
utkarsharma2 pushed a commit to astronomer/airflow that referenced this pull request Apr 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:API Airflow's REST/HTTP API area:UI Related to UI/UX. For Frontend Developers. area:webserver Webserver related Issues kind:documentation type:improvement Changelog: Improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add dag run_id to Audit Log
5 participants