Switch runtime metrics to operation-weighted progress#869
Merged
Conversation
Replace the in-tool partial-credit progress with a simpler operation-counting model: each heavy full-file operation adds the file's size to the work total when it starts and to the completed total when it ends, so progress reflects the actual, non-deterministic work with no per-file fraction weighting and no parsing of tool progress output. - Metrics: work.total/work.completed byte counters incremented at operation boundaries via an ambient thread-local file size; drop the FileSink partial-credit machinery. bytes.total stays the input-size reference. - Bracket the heavy full-file operations (closed-caption scan, idet, ffmpeg re-encode, HandBrake deinterlace, packet analysis, verify) with OpStarted/OpCompleted. - Keep the tool progress-parsing capability (ParseProgressFraction, ExecuteStreamStdOut, the Progress()/Json() builders) and its unit tests unused by this feature, for the media-tools library. - NativeAOT: enable EventSourceSupport when PublishAot is set so dotnet-counters and dotnet-monitor can read the meter from an AOT build; default builds are unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR refactors PlexCleaner’s develop-only runtime metrics to an operation-weighted model: each heavy full-file operation contributes the file’s size to work.total at start and to work.completed at completion, with progress.ratio derived from those gauges. It also adjusts tool wrappers and tests accordingly, and enables EventSource support for NativeAOT builds so metrics can be read via diagnostics tooling.
Changes:
- Replaced the prior per-file partial-credit progress model with operation-boundary
OpStarted/OpCompletedaccounting inMetrics. - Wrapped heavy full-file operations (ffprobe scans, ffmpeg verify/idet/encode, HandBrake deinterlace, packet analysis) with
OpStarted/OpCompletedand removed tool-output-driven progress reporting. - Enabled
EventSourceSupportfor AOT publishes to ensure diagnostics IPC is available fordotnet-counters/dotnet-monitor.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Updates the runtime-metrics documentation to describe operation-weighted progress and instrument set. |
| HISTORY.md | Updates 3.22 release notes to reflect the new operation-weighted progress model and instrument names. |
| PlexCleanerTests/MetricsTests.cs | Reworks unit tests to validate operation-weighted progress/ETA behavior and meter observability. |
| PlexCleaner/ProcessFile.cs | Removes the packet-level partial-credit reporting hook from analysis packet iteration. |
| PlexCleaner/ProcessDriver.cs | Switches file completion metrics call signature and ensures per-file thread-local size setup/teardown remains correct. |
| PlexCleaner/Process.cs | Removes now-unused duration seeding previously used for tool-progress-derived partial credit. |
| PlexCleaner/PlexCleaner.csproj | Enables EventSourceSupport only for PublishAot=true so metrics remain readable in AOT builds. |
| PlexCleaner/Metrics.cs | Implements operation-weighted work accounting via thread-local file size and new work.* gauges. |
| PlexCleaner/HandBrakeTool.cs | Removes progress-stream parsing and brackets the deinterlace operation with metrics work accounting. |
| PlexCleaner/FfProbeTool.cs | Brackets closed-caption scan and packet analysis with metrics work accounting. |
| PlexCleaner/FfMpegTool.cs | Brackets verify, idet, and encode operations with metrics work accounting; removes encode progress streaming. |
work.total and work.completed are run-scoped observable gauges, not Counter instruments, so name them gauges to avoid confusing dotnet-counters and OpenTelemetry consumers. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Move OpCompleted after the executed guard so a cancelled or failed-to-start tool is not credited as completed work, which would skew progress and ETA on interrupted runs. A non-zero exit still counts, the tool did the full-file work. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
GetPackets returns false on a non-zero exit where ffprobe still ran the scan, so gate OpCompleted on cancellation instead of the return value, otherwise a ran-but-non-zero scan strands its work in the total and skews progress for the rest of the run. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Gate OpCompleted on the scan having run, evidenced by exit 0 or captured stderr, instead of a cancellation check, so a cancellation or a failure to start (empty error) is excluded while a ran-but-non-zero scan is still counted. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
PlexCleaner/HandBrakeTool.cs:147
- Metrics.OpStarted() is called before Execute(...). If Execute returns false (cancellation or failure to start), work.total has already been incremented and is never rolled back, so progress.ratio/eta.seconds can remain skewed for the rest of the run. Consider only counting discovered work once the operation is confirmed to have started, or add a Metrics.OpAborted() (subtract current file size from work.total) and call it on the executed==false path.
Metrics.OpStarted();
bool executed = Execute(command, true, true, out BufferedCommandResult result);
if (!executed)
{
return false;
OpStarted adds the file size to the work total up front, so an operation that is cancelled or fails to start would leave the total permanently inflated and stop progress converging. Add OpAborted to roll that size back out, and call it on every abort path so each started operation resolves to exactly one of completed or aborted. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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
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.
Summary
Reworks the develop-only runtime metrics (still 3.22, unreleased) from the v2 in-tool partial-credit model to a simpler operation-counting model. Each heavy full-file operation on a file counts the file's size as work to do when it starts (
OpStarted) and as work done when it ends (OpCompleted), soprogress.ratio = work.completed / work.total. The total grows as the non-deterministic per-file path is discovered — no per-file fraction weighting, and no parsing of tool progress output (which risked perturbing tool behavior).Motivation: the metrics are a diagnostic for a small audience; the op-counting model is simpler, lower-risk (no
-progress/--json/stdout-stream parsing in the processing path), and covers every heavy pass uniformly — including the ffprobe closed-caption scan, which has no incremental progress signal and so could not be instrumented under the v2 model.Changes
Metrics.cs:work.total/work.completedbyte counters incremented at operation boundaries via an ambient thread-local file size; dropped theFileSinkpartial-credit machinery.bytes.totalstays the static input-size reference.OpStarted/OpCompleted: closed-caption scan, idet, ffmpeg re-encode (both overloads), HandBrake deinterlace, packet analysis, andVerifyMedia.FfMpeg/HandBrake.ParseProgressFraction,MediaTool.ExecuteStreamStdOut, and theProgress()/Json()builder methods, plusToolProgressParsingTests. Only the Metrics wiring was removed.<EventSourceSupport>true</EventSourceSupport>only whenPublishAot=true, so an AOT build links the enabled EventPipe and exposes the diagnostics IPC server fordotnet-counters/dotnet-monitor. Default (non-AOT) builds are unchanged. (EventSourceSupportis the real switch;EventPipeSupportis a no-op.)No version bump (develop-only feature, no released behavior). HISTORY 3.22 and the README Runtime Metrics section updated to describe op-counting.
Verification
dotnet format style --verify-no-changes --severity=infoclean; 238 tests pass (op-counting fold math + the retained parser tests); markdownlint clean.work.totalgrew as ops were discovered,progress.ratio= completed/total climbed. AOT binary: same, read live via the imagecounterswrapper.EventSourceSupport, creates the diagnostic socket sodotnet-countersattaches and reads the meter.🤖 Generated with Claude Code