Cleanup the DualStatsManager and fix any leftover Stats calls #62705
Cleanup the DualStatsManager and fix any leftover Stats calls #62705xBis7 wants to merge 6 commits into
Conversation
| stat.last_duration, | ||
| tags={"file_name": file_name, "bundle_name": normalized_bundle}, | ||
| tags={}, | ||
| legacy_name_tags={"bundle_name": normalized_bundle, "file_name": file_name}, |
There was a problem hiding this comment.
My initial assumption here was that the tags would be the other way around -- why don't we want bundle name and filepath included in the modern tags?
There was a problem hiding this comment.
They are.
It goes like this
- check the
legacy_name_tagsand cross-reference them with the variables in the YAML file - set the
legacy_name_tagsas variables in the legacy name - build the legacy metric name
- export the legacy metric with the
tagsparam - merge the
tagsand thelegacy_name_tags - export the new name with the merged dictionary as tags
That was actually a bug because, by just setting the tags, there weren't any available variables for the legacy name.
I've added comments in the code to explain the above.
The legacy_name_tags are going to end up as regular tags for the modern name. And the tags are just for whatever tag is set on both metrics.
There was a problem hiding this comment.
Oh. That's slightly confusing. The behabiour makes sense, but, just from the callsite (of which there are going to be lots) it's a bit confusing that legacy_name_tags gets applied to non-legacy metrics.
🤔
There was a problem hiding this comment.
I know. The behavior is consistent with before but the DualStatsManager logic is confusing. I renamed the extra_tags param to legacy_name_tags to make it more intuitive and added comments but it's still not that clear.
Do you have any suggestions?
Before the DualStatsManager
- legacy metrics didn't have any tags and everything was included in the name
- modern metrics had a simpler name and all the variables from the legacy metric name, as tags
The thing is that I'm trying to cover an edge case where the legacy and the modern metric had some extra tags, apart from the name variables.
There was a problem hiding this comment.
@ashb Can you make a final review? Then I think it would be great to merge.
|
This @patch(f"{Stats.__module__}.Stats.incr")and this mock_stats_incr.assert_called_with(
"edge_worker.ti.finish",
tags={
"dag_id": "test_dag",
"queue": "default",
"state": "failed",
"task_id": "started_running_orphaned",
},
)are wrong, but the test passes. I'm looking into it. |
|
Regarding the |
|
Hey @jscheffl, thank you for the review! I changed all the |
256ae43 to
1125582
Compare
jscheffl
left a comment
There was a problem hiding this comment.
Viewing the edge part - looks good to me! Thanks for the rework!
|
@jscheffl @dheerajturaga Thank you for the reviews! The CI failures seem unrelated. |
6e560ad to
ccbe11b
Compare
ccbe11b to
ac037b1
Compare
|
@ashb @amoghrajesh Can you take a look? |
|
I'm looking into removing the I'll keep this PR open just in case, I need to split the above patch into smaller ones. Otherwise, I'll close it. |
|
I'm closing this PR. Due to the work in #63932, this isn't relevant anymore. |
related discussion: #62019 (comment)
DualStatsManager code explanation
Usually the legacy metrics don't have any tags and all the variables are included in the name.
The
tagsdict refers to tags that are set on both the new name metric and the legacy name metric (separately from the name variables).The
legacy_name_tagsdict refers to the legacy name variables.The legacy metrics will only have the
tagsvalues as tags.Both dictionaries are eventually merged and the resulting dict is used for the final tags of the new metric.
For example,
the legacy metric won't have any tags and after that is exported,
{}andtagsare merged and then set as the new metric tags.Patch changes
The patch is consisted of the following changes
extra_tagstolegacy_name_tagsfor clarityDualStasManager...extra...with the word...legacy...in all referencestagsandlegacy_name_tagsparamsmetrics_template.yamltry-exceptpattern for the DualStatsManager import and maintained the old Stats calls as fallbacksWas generative AI tooling used to co-author this PR?
{pr_number}.significant.rstor{issue_number}.significant.rst, in airflow-core/newsfragments.