Expose counter information in profiler-cli#6084
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6084 +/- ##
==========================================
- Coverage 83.82% 83.43% -0.39%
==========================================
Files 330 339 +9
Lines 34682 35690 +1008
Branches 9703 10017 +314
==========================================
+ Hits 29072 29779 +707
- Misses 5181 5483 +302
+ Partials 429 428 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
|
Add `counter list` and `counter info <handle>` commands, and list each counter under its process in `profile info`, to inspect any counter track from the terminal. Counters get stable `c-N` handles, like threads and functions. Per-counter stats come from the counter's own tooltip schema, reusing the timeline tooltips' labels and formatters so the CLI and UI agree. Stats respect the current zoom. Part of firefox-devtools#6040
|
@canova makes sense, updated. |
canova
left a comment
canova
left a comment
There was a problem hiding this comment.
Thanks for the PR! Looks great to me with some nits!
One thing I noticed is that I think we don't currently use the sortWeight in the pq profile info command. It would be good to use that so it matches the web frontend. It can be also done as a follow-up.
This is only the first iteration for #6040.
Currently we show the counters in the profile info and get info of them with pq counter info. I think it would be great to get more information of the counter values, similar to "CPU activity over time" section that we have in the pq profile info as a follow-up. What do you think?
| const countersByPid = new Map<string, CounterSummary[]>(); | ||
| (getCounters(state) ?? []).forEach((_, index) => { | ||
| const counter = collectCounterSummary(store, threadMap, index); | ||
| const pid = String(profile.threads[counter.mainThreadIndex].pid); |
There was a problem hiding this comment.
counter has a pid already, we can do counter.pid directly. Also, pids should be string already, no need to do String().
| cpuMs: thread.cpuMs, | ||
| })), | ||
| remainingThreads: processItem.remainingThreads, | ||
| counters: countersByPid.get(String(processItem.pid)), |
There was a problem hiding this comment.
Same, I think no need to wrap it with a String.
| export type CounterStat = { | ||
| source: CounterTooltipDataSource; | ||
| label: string; | ||
| labelKey?: string; |
There was a problem hiding this comment.
Do we need labelKey? It was for localization, right? I think we can just remove it as we don't have any i18n in the cli.
| start: number, | ||
| end: number | ||
| ): number { | ||
| const [begin, finish] = getSampleIndexRangeForSelection(samples, start, end); |
There was a problem hiding this comment.
Nit: Hm, it would be good to get committedRangeCounterSampleSum from a counter selector instead of computing all the time.
2 participants
Add
counter listandcounter info <handle>commands, and list each counter under its process inprofile info, to inspect any counter track from the terminal. Counters get stablec-Nhandles, like threads and functions.Per-counter stats come from the counter's own tooltip schema, reusing the timeline tooltips' labels and formatters so the CLI and UI agree. Stats respect the current zoom.
This is only the first iteration for #6040.
Usage examples