Skip to content

Add metrics for how often a dag gets serialised. - #68906

Merged
potiuk merged 1 commit into
apache:mainfrom
Ei-Sandi:dag-serialisation-metrics
Jul 30, 2026
Merged

Add metrics for how often a dag gets serialised.#68906
potiuk merged 1 commit into
apache:mainfrom
Ei-Sandi:dag-serialisation-metrics

Conversation

@Ei-Sandi

@Ei-Sandi Ei-Sandi commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Changes

  • Adds a new dag.serialization counter metric that is emitted every time a Dag is serialized and written to the metadata DB.
  • The metric is tagged with dag_id and bundle_name, so serialization frequency can be tracked per Dag and per bundle.

(This gives operators visibility into how often DAGs are being re-serialized, to ensure we aren't unnecessarily re-serializing DAGs. )

Tests

  • Covered by unit tests.

Was generative AI tooling used to co-author this PR?
  • Yes (please specify the tool below)
    Co-authored-by: Claude Sonnet 4.6, Claude Opus 4.8 following the guidelines

  • Read the Pull Request Guidelines for more information. Note: commit author/co-author name and email in commits become permanently public when merged.
  • For fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
  • When adding dependency, check compliance with the ASF 3rd Party License Policy.
  • For significant user-facing changes create newsfragment: {pr_number}.significant.rst, in airflow-core/newsfragments. You can add this file in a follow-up commit after the PR is created so you know the PR number.

@Ei-Sandi
Ei-Sandi marked this pull request as ready for review June 23, 2026 16:35

@xBis7 xBis7 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Mostly looks good! I have a few suggestions for the tests. Thanks!

BTW, I've tested it manually with breeze and metrics work as expected.

Comment thread airflow-core/tests/unit/models/test_serialized_dag.py Outdated
Comment thread airflow-core/tests/unit/models/test_serialized_dag.py
@Ei-Sandi
Ei-Sandi force-pushed the dag-serialisation-metrics branch from 27b6bfa to 88a6071 Compare June 23, 2026 17:56
@Ei-Sandi

Copy link
Copy Markdown
Contributor Author

@xBis7 The comments has been addressed. Can you please review it again ? Thank you.

Comment thread airflow-core/tests/unit/models/test_serialized_dag.py Outdated
Comment thread airflow-core/tests/unit/models/test_serialized_dag.py Outdated
@Ei-Sandi
Ei-Sandi force-pushed the dag-serialisation-metrics branch from 88a6071 to 31f4624 Compare June 24, 2026 12:15

@xBis7 xBis7 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM!

@Ei-Sandi
Ei-Sandi force-pushed the dag-serialisation-metrics branch from 31f4624 to 5aa50eb Compare June 24, 2026 14:24
@potiuk potiuk added the ready for maintainer review Set after triaging when all criteria pass. label Jun 25, 2026
@Ei-Sandi
Ei-Sandi force-pushed the dag-serialisation-metrics branch from 5aa50eb to 7e3d88e Compare July 15, 2026 14:28
@eladkal eladkal added this to the Airflow 3.3.1 milestone Jul 15, 2026
@eladkal eladkal added the backport-to-v3-3-test Backport to v3-3-test label Jul 15, 2026

@Miretpl Miretpl left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

As far as I know this part of the codebase - looks good. I will wait a couple of days with the merge for any other possible reviews.

@vatsrahul1001

Copy link
Copy Markdown
Contributor

This looks more like a new feature and more suitable for minor 3.4.0

@SameerMesiah97 SameerMesiah97 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just one tiny suggestion.

Comment thread airflow-core/src/airflow/models/serialized_dag.py Outdated
@Ei-Sandi
Ei-Sandi force-pushed the dag-serialisation-metrics branch from 7e3d88e to 3f9506f Compare July 29, 2026 21:21
@potiuk
potiuk merged commit ba105ea into apache:main Jul 30, 2026
78 checks passed
@potiuk potiuk removed the backport-to-v3-3-test Backport to v3-3-test label Jul 30, 2026

@potiuk potiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Re-reviewed this before merging and want one thing resolved first — the metric is sound but its two emission sites don't mean the same thing.

Things I checked and am happy with:

  • The two stats.incr calls sit before mutually exclusive return True paths in write_dag, so there's no double-counting, and the early return False (min_update_interval) correctly doesn't emit.
  • name_variables: ["dag_id", "bundle_name"] with a matching legacy_name accurately describes what's emitted. Worth calling out because #69078 declares name_variables: [] while tagging with two variables — yours is the correct pattern.

The nit is inline. Not a big change, but I'd rather it went in right than be corrected after people start alerting on it.


Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting

session.merge(dag_version)
# Update the latest DagCode
DagCode.update_source_code(dag_id=dag.dag_id, fileloc=dag.fileloc, session=session)
stats.incr("dag.serialization_writes", tags={"dag_id": dag.dag_id, "bundle_name": bundle_name})

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is the earlier of the two emission sites, and it fires on the path where the serialized Dag did not change — only dag_version.bundle_name / bundle_version / version_data were merged and DagCode.update_source_code refreshed. No new SerializedDagModel row is written here.

The metric description says "Number of times a Dag was serialized and written to the metadata DB", which doesn't match that path. Anyone using dag.serialization_writes to measure how often Dags actually re-serialize — the obvious use, and what the name suggests — will over-count every time a bundle version changes without the Dag changing.

Two options, either is fine:

  1. Emit only from the second site (the real write), and drop this one.
  2. Keep both but distinguish them — either a reason/kind tag ("metadata_refresh" vs "new_version"), or reword the description to say it counts write operations including version-metadata refreshes.

I'd lean towards (1) unless you specifically want visibility into the refresh path.


Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@potiuk There was a bug which was fixed in PR #63871

When the dag template contained a callable as a value, without any actual changes to the dag, every time that it was parsed, it would get reserialized. We noticed this issue by chance while monitoring the DB.

I think it's good to keep both metrics but distinguish between them. Having a metric for even when there isn't an actual write, will help us identify such issues sooner.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@potiuk I created a follow up PR #70838 with 2 different metrics names. Could you please have a look at it, thank you.

@Ei-Sandi
Ei-Sandi deleted the dag-serialisation-metrics branch July 31, 2026 14:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:DAG-processing ready for maintainer review Set after triaging when all criteria pass.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants