Replace 4 counter track components with a single generic TrackCounter#5944
Replace 4 counter track components with a single generic TrackCounter#5944fatadel wants to merge 2 commits intofirefox-devtools:mainfrom
Conversation
Make counters self-describing in terms of rendering by adding `display` field of `CounterDisplayConfig` type. The value is derived from a counter's `category` and `name` fields. This data is sufficient to understand how a counter should be rendered allowing us to remove hardcoded logic for each counter. This is the first PR for issue firefox-devtools#5752.
Collapse the separate Memory, Power, ProcessCPU, and Bandwidth track implementations into a single TrackCounter component that renders any counter type using CounterDisplayConfig from PR firefox-devtools#5912. The LocalTrack union type is simplified from 8 variants to 5 by replacing 'memory', 'power', 'process-cpu', and 'bandwidth' with a single 'counter' type. The component branches on display.graphType for canvas drawing (accumulated vs rate) and on display.unit for tooltip rendering (bytes, pWh, percent, etc.). Track index ordering (for URL backward compatibility) is handled by a category-based mapping function. Display ordering uses the new display.sortWeight field. Part of firefox-devtools#5752.
fatadel
changed the title
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #5944 +/- ##
==========================================
- Coverage 85.38% 85.30% -0.09%
==========================================
Files 322 316 -6
Lines 32103 31675 -428
Branches 8839 8783 -56
==========================================
- Hits 27412 27021 -391
+ Misses 4260 4223 -37
Partials 431 431 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Nice! It looks like the order of the tacks have changed with this PR, is this intentional? Compositor is above the other counter tracks. |
canova
left a comment
canova
left a comment
There was a problem hiding this comment.
I'm a bit worried about the way we decide how we figure out which tooltip to show. Currently it works, but it looks brittle considering that another counter could have similar fields. I'm still leaning towards defining tooltip fields inside the profile format. But @fqueze was saying that it was difficult to make it generic. I think I would be okay with losing the l10n part of the tooltips to make it generic, so we can define all the things like the label and what it should display. What do you think?
| // Process CPU tooltip. | ||
| if (unit === 'percent') { | ||
| const cpuUsage = samples.count[counterIndex]; | ||
| const sampleTimeDeltaInMs = | ||
| counterIndex === 0 | ||
| ? interval | ||
| : samples.time[counterIndex] - samples.time[counterIndex - 1]; | ||
| const cpuRatio = | ||
| cpuUsage / sampleTimeDeltaInMs / maxCounterSampleCountPerMs; | ||
| return ( | ||
| <Tooltip mouseX={mouseX} mouseY={mouseY}> | ||
| <div className="timelineTrackCounterTooltip"> | ||
| <div className="timelineTrackCounterTooltipLine"> | ||
| CPU:{' '} |
There was a problem hiding this comment.
Currently we assume that all the percent type will be CPU. But we might want to have different non-cpu counters that might use 'percent' in the future.
| } | ||
|
|
||
| // Bandwidth tooltip — bytes with rate, CO2, and accumulated total. | ||
| if (unit === 'bytes' && display.graphType === 'line-rate') { |
There was a problem hiding this comment.
Currently we check if if (unit === 'bytes' && display.graphType === 'line-rate') { to see if this is a bandwidth counter. And then we have some strings below like this in the ftl files:
Data transferred up to this time
This is very specific to the bandwidth counters. And we might have another non-bandwidth counter that might have bytes as unit and line-rate as graphType. I don't think we should do these checks to assume it's some certain type of counter.
The same issue applies to the memory tooltip below.
|
I was thinking about something like this that we can define in the counter schema: type CounterTooltipDataSource =
| 'count' // samples.count[i]
| 'rate' // count / sampleTimeDelta
| 'cpu-ratio' // rate / maxCounterSampleCountPerMs
| 'accumulated' // accumulatedCounts[i] - minCount
| 'count-range' // total range across the visible graph
| 'selection-total' // sum over the preview selection
| 'sample-number'; // samples.number[i] — row is omitted when null
type CounterTooltipFormatter = 'bytes' | 'bytes-per-second' | 'percent' | 'number';
type CounterTooltipRow =
| { type: 'value'; source: CounterTooltipDataSource; format: CounterTooltipFormatter; label: string }
| { type: 'separator' };
// In CounterDisplayConfig:
tooltipRows: CounterTooltipRow[];And then we could use that like: tooltipRows: [
{ type: 'value', source: 'accumulated', format: 'bytes', label: 'relative memory at this time' },
{ type: 'value', source: 'count-range', format: 'bytes', label: 'memory range in graph' },
{ type: 'value', source: 'sample-number', format: 'number', label: 'allocations/deallocations since previous sample' },
]But not so sure if it works with all the counters that we have. |
Ideally we would have a generic implementation defined in the counter schema inside the profile JSON, and we can have specific overrides for specific counters (eg power, to show the CO2e values), similar to how network markers are handled differently. |
|
Yeah that sounds good. Also, adding context here after talking about it in sync. It would make sense to keep a |
3 participants
Collapse the separate Memory, Power, ProcessCPU, and Bandwidth track implementations into a single TrackCounter component that renders any counter type using CounterDisplayConfig from PR #5912.
The LocalTrack union type is simplified from 8 variants to 5 by replacing 'memory', 'power', 'process-cpu', and 'bandwidth' with a single 'counter' type. The component branches on display.graphType for canvas drawing (accumulated vs rate) and on display.unit for tooltip rendering (bytes, pWh, percent, etc.).
Track index ordering (for URL backward compatibility) is handled by a category-based mapping function. Display ordering uses the new display.sortWeight field.
This is the second (last) PR for issue #5752.