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 3 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.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #5944 +/- ##
==========================================
- Coverage 85.37% 85.28% -0.10%
==========================================
Files 322 316 -6
Lines 32069 31647 -422
Branches 8814 8763 -51
==========================================
- Hits 27378 26989 -389
+ Misses 4260 4227 -33
Partials 431 431 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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?
|
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 |
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.
cc362ba to
925b5f7
Compare
No, this was not intentional. I have realized that I missed it because the profile I've used did not have the Compositor track. |
|
Thanks for the feedback on tooltip rendering, @fqueze and @canova ! After thinking about it and consulting with Claude, here's the approach I'd like to propose. It combines @fqueze's idea of "generic default + specific overrides" with @canova's suggestion of a tooltipType field. The concept: generic by default, with opt-in overrides for rich tooltips We add an optional tooltipType field to CounterDisplayConfig: export type CounterDisplayConfig = { }; The logic in the component is:
|
|
(apparently the comment I wrote in the morning wasn't sent, and I lost the message. rewriting) I've thought about it a bit more and discussed with @fqueze and I think it might make sense to not add In the code where we decide which tooltip to show, instead of this field, we can look at the This would prevent us from adding an intermediate display field that will be removed soon. WDYT? |
Yeah, it makes sense. Instead of adding a |
# Conflicts: # src/app-logic/constants.ts # src/components/timeline/TrackBandwidthGraph.tsx # src/components/timeline/TrackMemoryGraph.tsx # src/components/timeline/TrackPowerGraph.tsx # src/profile-logic/process-profile.ts # src/profile-logic/processed-profile-versioning.ts # src/test/components/TrackBandwidth.test.tsx # src/test/components/TrackMemory.test.tsx # src/test/fixtures/profiles/processed-profile.ts


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.