Revert "Add task_execution_time histogram (#56355)"#57844
Conversation
This reverts commit b9b3361.
![gemini-code-assist[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request reverts the addition of histogram-based metrics for task execution times and other related statistics. The changes correctly remove the histogram logic from dashboard panels, runtime metrics collection, and associated tests. My review confirms that the revert is mostly clean, but I've identified one piece of leftover code from the reverted feature that should be removed for code clarity.
| histogram_buckets_s = [ | ||
| 0.1, | ||
| 0.25, | ||
| 0.5, | ||
| 1.0, | ||
| 2.5, | ||
| 5.0, | ||
| 7.5, | ||
| 10.0, | ||
| 15.0, | ||
| 20.0, | ||
| 25.0, | ||
| 50.0, | ||
| 75.0, | ||
| 100.0, | ||
| 150.0, | ||
| 500.0, | ||
| 1000.0, | ||
| 2500.0, | ||
| 5000.0, | ||
| ] |
There was a problem hiding this comment.
This histogram_buckets_s list appears to be a leftover from the reverted histogram implementation. It's defined as a class attribute but is no longer used anywhere in the class or module after the other changes in this PR. It should be removed to avoid confusion and dead code.
![cursor[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| 1000.0, | ||
| 2500.0, | ||
| 5000.0, | ||
| ] |
There was a problem hiding this comment.
Bug: Unused Histogram Buckets Clutter Dataclass
The histogram_buckets_s list, a remnant of a reverted histogram implementation, is incorrectly defined as an unused instance attribute within the OpRuntimeMetrics dataclass. This unnecessarily adds a large list to every instance and clutters the class definition.
|
fixing forward instead: #57851 |
2 participants
This reverts commit b9b3361.
Description
Revert histograms for a future refactor to remove locks and to abstract away histogram details.
Related issues
Additional information