Repository navigation
Conversation
`--time-range` and `--absolute-timestamps` were global options, so `insights`, `navigations`, `diff` and `scan` accepted them and returned results for the full trace without any warning. Numeric options had no validation: `--top -5`, `--top abc` and `--bucket 0` returned empty results with exit code 0. - Declare `--time-range` and `--absolute-timestamps` only on the commands that filter by time. `diff` keeps `--absolute-timestamps` for its window values. Other commands reject both options through yargs strict mode. - Validate `--top`/`--limit`/`--slowest` (integer >= 1), `--min-duration` (number >= 0) and `--bucket` (duration > 0) with a clear error message. - Extract `loadTrace` and `loadTraceWithTimeRange` helpers to replace the load and time-range block repeated in each command handler. - Document the time range options in the README. Refs ChromeDevTools#9
mangeshraut712
left a comment
mangeshraut712
left a comment
There was a problem hiding this comment.
Scoping the time-range options per command and rejecting them elsewhere will be a lot clearer for agents than silently ignoring them. The extracted loadTraceWithTimeRange helper also cuts down a lot of repetition.
| function positiveDuration(flag: string): (value: unknown) => number { | ||
| return (value) => { | ||
| const ms = parseTimeValue(String(value)); | ||
| if (ms <= 0) { | ||
| throw new Error(`--${flag} must be a duration > 0. Received: ${value}`); | ||
| } | ||
| return ms; | ||
| }; |
There was a problem hiding this comment.
> 0 still lets through durations that give an empty result, which is the case this PR is trying to remove. extractProfile advances with currentEnd = Math.min(endMs, round(currentStart + bucketSizeMs)), and round defaults to 2 decimals. So --bucket 1us (0.001 ms) or --bucket 0.004 makes currentEnd === currentStart on the first iteration. It breaks out and returns buckets: [] with exit 0.
At the other end, --bucket 10us is accepted and on a 10 s trace that means ~1M BottomUpRootNode builds. Rejecting anything below 0.01 ms (or whatever resolution round uses) would close the empty-result case. A cap on totalRangeMs / bucket with a clear error might be worth adding too. I worked this out from reading profile.ts and haven't run it.
There was a problem hiding this comment.
Confirmed by running it: --bucket 1us and --bucket 0.004 returned buckets: [] with exit 0, and --bucket 10us returned 16000 buckets on the 160 ms test fixture. Fixed in 2e829a0:
--bucketbelow0.01msfails with--bucket must be a duration >= 0.01ms. Received: 1us.profilefails when the range needs more than 1000 buckets and suggests the smallest valid size, for exampleBucket size 0.01ms creates 16000 buckets over 160ms. The maximum is 1000. Use a bucket size of at least 0.16ms.
Bucket bounds are rounded to 2 decimals, so a bucket below 0.01ms returned an empty profile with exit 0. Reject it in the --bucket coercion. Also fail when the time range needs more than 1000 buckets, and suggest the smallest valid bucket size in the error.
Part of #9.
Problem
--time-rangeand--absolute-timestampswere global options, so every command accepted them.insights,navigations,diffandscando not filter by time, so they ignored the option and returned results for the full trace without any warning.Numeric options had no validation. An agent reads an empty result as "nothing found":
tasks <file> --top -5tasks: [], exit 0tasks <file> --top abctasks: [], exit 0profile <file> --bucket 0buckets: [], exit 0tasks <file> --min-duration -1Changes
Declare
--time-rangeand--absolute-timestampsonly on the commands that filter by time (overview,tasks,find,profile,scheduling,network,interactions,shifts,events).diffkeeps--absolute-timestampsfor--window-aand--window-b. Other commands reject both options through yargs strict mode, for exampleUnknown arguments: time-range, timeRange.Validate numeric options with yargs
coerce:--top/--limit/--slowest: integer >= 1--min-duration: number >= 0--bucket: duration >= 0.01ms, and at most 1000 buckets per profile. Bucket bounds are rounded to 2 decimals, so smaller buckets returned an empty profile. The error for too many buckets gives the smallest valid size.Errors include the raw value, for example
--top must be a positive integer. Received: abc.Extract
loadTraceandloadTraceWithTimeRangehelpers. They replace the load and time-range block repeated in each handler.Document the
--bucketlimits in the READMEprofilesection.Move the time range options from "Global Options" to a new "Time Range Options" section in the README.
Numeric options do not declare
type: 'number', so the coerce function receives the raw input and the error showsabcinstead ofNaN. As a side effect,--helpdoes not show the[number]label for these options.Verification
test/cli.test.tsandtest/extractors/profile.test.ts. 19 failed before the change.npm test(157 tests),npm run checkandnpm run buildpass.tasks --top 1 --time-range 0,200,profile --bucket 50ms,diff --absolute-timestamps) works as before.