Skip to content

Add CounterDisplayConfig to counters in the processed profile format - #5912

Merged
fatadel merged 3 commits into
firefox-devtools:mainfrom
fatadel:generic-counters-5752
Apr 15, 2026
Merged

Add CounterDisplayConfig to counters in the processed profile format#5912
fatadel merged 3 commits into
firefox-devtools:mainfrom
fatadel:generic-counters-5752

Conversation

@fatadel

@fatadelfatadel commented Mar 25, 2026

Copy link
Copy Markdown
Contributor

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 #5752.

@fatadel
fatadel requested review from canova, fqueze and mstange and removed request for canova and mstangeMarch 25, 2026 15:16
@codecov

codecovBot commented Mar 25, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 57.50000% with 17 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.32%. Comparing base (4308a73) to head (3641a8e).
⚠️ Report is 2 commits behind head on main.

Files with missing linesPatch %Lines
src/profile-logic/processed-profile-versioning.ts40.90%13 Missing ⚠️
src/profile-logic/process-profile.ts60.00%4 Missing ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #5912 +/- ##
==========================================
- Coverage 85.37% 85.32% -0.05% 
==========================================
Files 322 322 Lines 32069 32092 +23 Branches 8814 8738 -76 ==========================================
+ Hits 27378 27384 +6 - Misses 4260 4277 +17 
Partials 431 431 

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@fatadel

Copy link
Copy Markdown
ContributorAuthor

This change should NOT introduce any visually-perceivable diffs, however, please let me know if you want me to provide profiles (and under what settings if so).

@canovacanova left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for working on this Adel! It's going to be a great improvement to have generic counters in the frontend!

I'm adding some preliminary review comments that I wrote while doing a first pass. But I didn't check the existing counter track visualization code yet, I remember that they had some subtle differences in the way we visualize while discussing it with @fqueze. So I'll do another pass by checking the existing counters.

There are a few things/questions I mentioned below that are high level. For example: it would be good to make the gecko profile format to have the same display object and not only "hints".


Another thing I want to mention is the gecko profile format upgrader that we are adding right now. It might be easier to land the processed profile format versioning at first, and then once we have a backend patch, land the gecko profile versioning in a later PR as a follow-up. This is because if we land the gecko profile format bump now, and then we work on some other feature that we might want to land before this in the backend, we will be blocked until we have an implementation for that gecko format. So usually it's better to land them close to each other. Since we have a few PTO lined up for multiple people, I guess it's going to take a bit longer to implement the Firefox side.

To be able to do that, we can move the gecko profile upgrader to the process-profile phase. And once we bump the format, we can move that process profile phase into the gecko profile upgrader (which hopefully will be straightforward to do as a follow-up).

Comment threadsrc/types/profile.ts Outdated
Comment threadsrc/types/profile.ts Outdated
Comment threadsrc/types/profile.ts Outdated
Comment threadsrc/types/profile.ts Outdated
Comment threadsrc/types/gecko-profile.ts Outdated
Comment threadsrc/types/profile.ts Outdated
Comment threadsrc/profile-logic/process-profile.ts Outdated
@fqueze

Copy link
Copy Markdown
Contributor

useDecimation: whether to apply min-max decimation for dense data

Is this needed? Do you have an example of a case where not using min-max decimation is better? I thought min-max decimation was just an implementation detail of how we are drawing the charts quickly, without causing any visible change.

@fqueze

Copy link
Copy Markdown
Contributor

It's going to be a great improvement to have generic counters in the frontend!

+1 It's very exciting that it'll become easy to add a new counter in a profile and see the result within minutes instead of spending hours to hack the front-end to show it!

I would recommend also looking at marker tracks (declared in marker schemas), as I think eventually we'll want to merge the marker chart implementations with the counter chart implementation. If anything in the format can be made closer, that might simplify future steps.

@fatadel
fatadelforce-pushed the generic-counters-5752 branch from 5936952 to cd81618CompareMarch 30, 2026 14:54
@fatadelfatadel closed this Mar 30, 2026
@fatadel
fatadelforce-pushed the generic-counters-5752 branch from cd81618 to 66e023aCompareMarch 30, 2026 14:57
@fatadelfatadel reopened this Mar 30, 2026
@fatadelfatadel changed the title Add display property to counter formats to make them self-describing for renderingAdd CounterDisplayConfig to counters in the processed profile formatMar 30, 2026
@fatadel

Copy link
Copy Markdown
ContributorAuthor

Thanks for your valuable comments, @canova and @fqueze! I hope I've correctly addressed all of them. Please, have another look.

@fatadel
fatadel requested a review from canovaMarch 30, 2026 15:32
Comment threadsrc/types/profile.ts Outdated
@fatadel
fatadelforce-pushed the generic-counters-5752 branch 2 times, most recently from 54b2afa to 3b8dd07CompareApril 2, 2026 14:12
@fatadel

Copy link
Copy Markdown
ContributorAuthor

We've decided with @canova that I will first provide another (an MVP) PR that involves leveraging this new display property to render the counters. We want to make sure that it contains enough information for the renderer before we merge this PR.

@fatadel
fatadelforce-pushed the generic-counters-5752 branch 2 times, most recently from bf24e32 to 22b9d50CompareApril 8, 2026 13:44
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.
@fatadel
fatadelforce-pushed the generic-counters-5752 branch from 22b9d50 to 21adfa0CompareApril 10, 2026 12:01
fatadel added a commit to fatadel/profiler that referenced this pull request Apr 10, 2026
 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
fatadel requested a review from canovaApril 10, 2026 12:59
fatadel added a commit to fatadel/profiler that referenced this pull request Apr 13, 2026
 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.

@canovacanova left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good now. I found one more thing that could benefit from being inside the upgrader. Let me know what you think!

Comment threadsrc/types/profile.ts Outdated
Comment threadsrc/profile-logic/processed-profile-versioning.ts Outdated
@fatadel

Copy link
Copy Markdown
ContributorAuthor

Looks good now. I found one more thing that could benefit from being inside the upgrader. Let me know what you think!

I think it makes sense to include it in this PR. I don't want to have another upgrader for such a "mechanical" change.
So, I'll migrate counter.color into display.color in the upgrader (falling back to the current defaults when it's undefined) and remove the standalone color field from the types.

This also means the track components will start reading from display.color which we wanted to keep for the second PR only, but I think it's negligible here.

…rader
The existing optional `color` field on counters (used by external
importers) is now consolidated into `display.color`. The upgrader
uses the old `color` value as a fallback when deriving the display
config, then removes the standalone field. Components now read
color from `counter.display.color` directly.
@fatadel

Copy link
Copy Markdown
ContributorAuthor

I've migrated the color field. Could have a look just at the last commit, pls? @canova
I will squash it once you approve.

@fatadel
fatadel requested a review from canovaApril 15, 2026 08:42

@canovacanova left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Let's land it!

@fatadel
fatadel merged commit 7f631a0 into firefox-devtools:mainApr 15, 2026
19 of 21 checks passed
@fatadel
fatadel deleted the generic-counters-5752 branch April 15, 2026 10:18
@fatadel

Copy link
Copy Markdown
ContributorAuthor

I've migrated the color field. Could have a look just at the last commit, pls? @canova I will squash it once you approve.

Actually, it makes sense to keep that as a second commit. So, I've kept it here for history and squashed everything into main.

fatadel added a commit that referenced this pull request Apr 27, 2026
…#5944)
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 is simplified from 8 variants to 5 by replacing
'memory', 'power', 'process-cpu', and 'bandwidth' with a single
'counter' type. The graph branches on display.graphType for canvas
drawing (accumulated vs rate) and on counter.category/counter.name
for tooltip rendering, while display.unit and display.label drive
value formatting and the track label.
Counter tracks get a single dedicated slot in LOCAL_TRACK_INDEX_ORDER;
existing URLs are remapped by a new v16 URL upgrader. Display ordering
between counters uses the new display.sortWeight field, falling back
to a natural sort on counter name.
Closes#5752.
@fatadelfatadel mentioned this pull request Apr 28, 2026
fatadel added a commit that referenced this pull request Apr 28, 2026
Changes:
[Nazım Can Altınova] Add Adel to the automatic dependency reviews
(#5930)
[fatadel] Fix arrow panel appearing behind marker tooltips (#5926)
[fatadel] Upgrade Node.js from v22 to v24 (#5923)
[Markus Stange] Use createStackTableBySkippingDiscarded in focusSelf.
(#5916)
[Markus Stange] Propagate isJS to symbolicated funcs (#5907)
[Markus Stange] Always render the CPU-usage-aware activity graph when
CPU information is available (#5918)
[Nazım Can Altınova] Fallback to javascript highlighting in the source
view as a backup (#5936)
[Nazım Can Altınova] Properly type the return value of
_languageExtForPath (#5937)
[Nazım Can Altınova] Update typescript eslint dependencies (#5938)
[Markus Stange] Modernize more of the transform functions (#5934)
[Paul Adenot] Fix extractGeckoLogs for structured Log marker format (bug
2022540) (#5927)
[Nazım Can Altınova] Move some profile fetching code into a separate
module. (#5939)
[Markus Stange] Update the hovered item when panning any viewport canvas
(#5903)
[Markus Stange] Migrate Home page animation to CSS transitions and
remove react-transition-group (#5649)
[Nazım Can Altınova] Fix test/lint commands on Windows and fix CI
(#5947)
[Nazım Can Altınova] Convert profile-logic/js-tracer.tsx to a ts file
(#5942)
[Markus Stange] Remove panelLayoutGeneration (#5946)
[fatadel] Add CounterDisplayConfig to counters in the processed profile
format (#5912)
[Nazım Can Altınova] Fix eslint-config-prettier silently overriding
custom rules (#5955)
[fatadel] Dim non-matching nodes in the stack chart when searching
(#5935)
[Nazım Can Altınova] Fix loading .json.gz profiles from inside zip
archives (#5959)
[Ryan Hunt] Add a fullscreen button to the bottom box (#5605)
[Markus Stange] Speed up _computeCallNodeTableHierarchy by keeping
siblings ordered by func (#5964)
[Markus Stange] Replace symbolicator-cli with a profiler-edit node tool
(#5965)
[Nazım Can Altınova] Add "Include idle samples" toggle to the call tree
settings (#5968)
[Nazım Can Altınova] Add dark mode versions of the fullscreen icons
(#5972)
[fatadel] Replace 4 counter track components with a single generic
TrackCounter (#5944)
[fatadel] Use ephemeral port for esbuild's internal dev server (#5974)
[carverdamien] Remove category from LongTaskMarkerPayload (#5975)
And special thanks to our localizers:
de: Ger de: Michael Köhler el: Jim Spentzos en-GB: Ian Neal es-CL: ravmn fr: Théo Chevalier ia: Melo46 it: Francesco Lodolo [:flod] nl: Mark Heijl pt-BR: Marcelo Ghelman ru: Valery Ledovskoy ru: berry sv-SE: Andreas Pettersson tr: Grk zh-CN: Olvcpr423 zh-CN: wxie zh-TW: Pin-guang Chen
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@fatadel@fqueze@canova