Add metrics for how often a dag gets serialised. - #68906
Conversation
27b6bfa to
88a6071
Compare
|
@xBis7 The comments has been addressed. Can you please review it again ? Thank you. |
88a6071 to
31f4624
Compare
31f4624 to
5aa50eb
Compare
5aa50eb to
7e3d88e
Compare
Miretpl
left a comment
There was a problem hiding this comment.
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.
|
This looks more like a new feature and more suitable for minor 3.4.0 |
SameerMesiah97
left a comment
There was a problem hiding this comment.
Just one tiny suggestion.
7e3d88e to
3f9506f
Compare
potiuk
left a comment
There was a problem hiding this comment.
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.incrcalls sit before mutually exclusivereturn Truepaths inwrite_dag, so there's no double-counting, and the earlyreturn False(min_update_interval) correctly doesn't emit. name_variables: ["dag_id", "bundle_name"]with a matchinglegacy_nameaccurately describes what's emitted. Worth calling out because #69078 declaresname_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}) |
There was a problem hiding this comment.
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:
- Emit only from the second site (the real write), and drop this one.
- Keep both but distinguish them — either a
reason/kindtag ("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
There was a problem hiding this comment.
@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.
Changes
(This gives operators visibility into how often DAGs are being re-serialized, to ensure we aren't unnecessarily re-serializing DAGs. )
Tests
Was generative AI tooling used to co-author this PR?
Co-authored-by: Claude Sonnet 4.6, Claude Opus 4.8 following the guidelines
{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.