Fix broken dag_processing.total_parse_time metric#62128
Open
nickstenning wants to merge 1 commit intoapache:mainfrom
Open
Fix broken dag_processing.total_parse_time metric#62128nickstenning wants to merge 1 commit intoapache:mainfrom
dag_processing.total_parse_time metric#62128nickstenning wants to merge 1 commit intoapache:mainfrom
Conversation
DagFileProcessorManager has been emitting a nonsense value for `dag_processing.total_parse_time` since 8774f28, which reversed the order in which `emit_metrics` and `prepare_file_queue` (then called `prepare_file_path_queue`) were called. As `prepare_file_path_queue` was responsible for resetting the value of `self._parsing_start_time`, the assumption made by `emit_metrics` was that it would be called once the file queue had been cleared, but crucially before `prepare_file_queue` was called to refill the queue. I've put the order of these calls back to one that generates valid metrics, and restructured the code in order to make the data dependencies explicit so that it's harder to make the same mistake in future refactoring.
nickstenning
requested review from
ephraimbuddy and
jedcunningham
as code owners
kaxil
reviewed
Feb 18, 2026
| self._refresh_dag_bundles(known_files=known_files) | ||
|
|
||
| if not self._file_queue: | ||
| if self._parsing_start_time: |
Member
There was a problem hiding this comment.
Could you add a simple test:
- First cycle with empty queue does not emit parse-time metric and does not crash,
- Subsequent empty-queue transition emits
dag_processing.total_parse_timefrom the previous cycle window, _parsing_start_timereset semantics are correct.
kaxil
reviewed
Feb 18, 2026
| self._refresh_dag_bundles(known_files=known_files) | ||
|
|
||
| if not self._file_queue: | ||
| if self._parsing_start_time: |
Member
There was a problem hiding this comment.
self._parsing_start_time is defined with init=False:
So this will fail because it hasn't been assigned yet.
Contributor
Author
There was a problem hiding this comment.
Ah, sorry, yes. It should be a hasattr or, more likely, we should update to have a default of None for this.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
DagFileProcessorManager has been emitting a nonsense value for
dag_processing.total_parse_timesince 8774f28, which reversed the order in whichemit_metricsandprepare_file_queue(then namedprepare_file_path_queue) were called.As
prepare_file_path_queuewas responsible for resetting the value ofself._parsing_start_time, the assumption made byemit_metricswas that it would be called once the file queue had been cleared, but crucially beforeprepare_file_queuewas called to refill the queue.I've put the order of these calls back to one that generates valid metrics, and restructured the code in order to make the data dependencies explicit so that it's harder to make the same mistake in future refactoring.