New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: extend tracing startRecording API to take a full tracing config #13914
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -51,9 +51,7 @@ Once all child processes have acknowledged the `getCategories` request the | |
|
||
### `contentTracing.startRecording(options, callback)` | ||
|
||
* `options` Object | ||
* `categoryFilter` String | ||
* `traceOptions` String | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @MarshallOfSound How properly reflect in the docs the fact that the {
"included_categories": ["disabled-by-default-memory-infra"],
"excluded_categories": ["*"],
"memory_dump_config": {
"triggers": [
{ "mode": "light", "periodic_interval_ms": 50 },
{ "mode": "detailed", "periodic_interval_ms": 1000 }
]
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's a little 😬 that one of these formats uses There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @nornagon the |
||
* `options` ([TraceCategoriesAndOptions](structures/trace-categories-and-options.md) | [TraceConfig](structures/trace-config.md)) | ||
alexeykuzmin marked this conversation as resolved.
Show resolved
Hide resolved
|
||
* `callback` Function | ||
|
||
Start recording on all processes. | ||
|
@@ -62,35 +60,6 @@ Recording begins immediately locally and asynchronously on child processes | |
as soon as they receive the EnableRecording request. The `callback` will be | ||
called once all child processes have acknowledged the `startRecording` request. | ||
|
||
`categoryFilter` is a filter to control what category groups should be | ||
traced. A filter can have an optional `-` prefix to exclude category groups | ||
that contain a matching category. Having both included and excluded | ||
category patterns in the same list is not supported. | ||
|
||
Examples: | ||
|
||
* `test_MyTest*`, | ||
* `test_MyTest*,test_OtherStuff`, | ||
* `"-excluded_category1,-excluded_category2` | ||
|
||
`traceOptions` controls what kind of tracing is enabled, it is a comma-delimited | ||
list. Possible options are: | ||
|
||
* `record-until-full` | ||
* `record-continuously` | ||
* `trace-to-console` | ||
* `enable-sampling` | ||
* `enable-systrace` | ||
|
||
The first 3 options are trace recording modes and hence mutually exclusive. | ||
If more than one trace recording modes appear in the `traceOptions` string, | ||
the last one takes precedence. If none of the trace recording modes are | ||
specified, recording mode is `record-until-full`. | ||
|
||
The trace option will first be reset to the default option (`record_mode` set to | ||
`record-until-full`, `enable_sampling` and `enable_systrace` set to `false`) | ||
before options parsed from `traceOptions` are applied on it. | ||
|
||
### `contentTracing.stopRecording(resultFilePath, callback)` | ||
|
||
* `resultFilePath` String | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
# TraceCategoriesAndOptions Object | ||
|
||
* `categoryFilter` String – is a filter to control what category groups | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: can just be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I just copied it from the old docs. |
||
should be traced. A filter can have an optional `-` prefix to exclude | ||
category groups that contain a matching category. Having both included | ||
and excluded category patterns in the same list is not supported. Examples: | ||
`test_MyTest*`, `test_MyTest*,test_OtherStuff`, `-excluded_category1,-excluded_category2`. | ||
* `traceOptions` String - Controls what kind of tracing is enabled, | ||
it is a comma-delimited sequence of the following strings: | ||
`record-until-full`, `record-continuously`, `trace-to-console`, `enable-sampling`, `enable-systrace`, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i went looking, and fwiw i don't think |
||
e.g. `'record-until-full,enable-sampling'`. | ||
The first 3 options are trace recording modes and hence mutually exclusive. | ||
If more than one trace recording modes appear in the `traceOptions` string, | ||
the last one takes precedence. If none of the trace recording modes are | ||
specified, recording mode is `record-until-full`. | ||
The trace option will first be reset to the default option (`record_mode` set | ||
to `record-until-full`, `enable_sampling` and `enable_systrace` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is it Perhaps we should just link to https://paulirish.github.io/debugger-protocol-viewer/tot/Tracing/#type-TraceConfig or similar? |
||
set to `false`) before options parsed from `traceOptions` are applied on it. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
# TraceConfig Object | ||
|
||
* `included_categories` String[] (optional) | ||
* `excluded_categories` String[] (optional) | ||
* `memory_dump_config` Object (optional) | ||
|
||
See an example in the [Chromium docs][1]. | ||
|
||
[1]: https://chromium.googlesource.com/chromium/src/+/master/docs/memory-infra/memory_infra_startup_tracing.md#the-advanced-way |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,145 @@ | ||
const { remote } = require('electron') | ||
const chai = require('chai') | ||
const dirtyChai = require('dirty-chai') | ||
const fs = require('fs') | ||
const path = require('path') | ||
|
||
const { expect } = chai | ||
const { app, contentTracing } = remote | ||
|
||
chai.use(dirtyChai) | ||
|
||
const timeout = async (milliseconds) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no need for |
||
return new Promise((resolve) => { | ||
setTimeout(resolve, milliseconds) | ||
}) | ||
} | ||
|
||
const getPathInATempFolder = (filename) => { | ||
return path.join(app.getPath('temp'), filename) | ||
} | ||
|
||
describe('contentTracing', () => { | ||
beforeEach(function () { | ||
// FIXME: The tests are skipped on arm/arm64. | ||
if (process.platform === 'linux' && | ||
['arm', 'arm64'].includes(process.arch)) { | ||
this.skip() | ||
} | ||
}) | ||
|
||
const startRecording = async (options) => { | ||
return new Promise((resolve) => { | ||
contentTracing.startRecording(options, () => { | ||
resolve() | ||
}) | ||
}) | ||
} | ||
|
||
const stopRecording = async (filePath) => { | ||
return new Promise((resolve) => { | ||
contentTracing.stopRecording(filePath, (resultFilePath) => { | ||
resolve(resultFilePath) | ||
}) | ||
}) | ||
} | ||
|
||
const record = async (options, outputFilePath, recordTimeInMilliseconds = 1e3) => { | ||
await app.whenReady() | ||
|
||
await startRecording(options) | ||
await timeout(recordTimeInMilliseconds) | ||
const resultFilePath = await stopRecording(outputFilePath) | ||
|
||
return resultFilePath | ||
} | ||
|
||
const outputFilePath = getPathInATempFolder('trace.json') | ||
beforeEach(() => { | ||
if (fs.existsSync(outputFilePath)) { | ||
fs.unlinkSync(outputFilePath) | ||
} | ||
}) | ||
|
||
describe('startRecording', function () { | ||
this.timeout(5e3) | ||
|
||
const getFileSizeInKiloBytes = (filePath) => { | ||
const stats = fs.statSync(filePath) | ||
const fileSizeInBytes = stats.size | ||
const fileSizeInKiloBytes = fileSizeInBytes / 1024 | ||
return fileSizeInKiloBytes | ||
} | ||
|
||
it('accepts an empty config', async () => { | ||
const config = {} | ||
await record(config, outputFilePath) | ||
|
||
expect(fs.existsSync(outputFilePath)).to.be.true() | ||
|
||
const fileSizeInKiloBytes = getFileSizeInKiloBytes(outputFilePath) | ||
expect(fileSizeInKiloBytes).to.be.above(0, | ||
`the trace output file is empty, check "${outputFilePath}"`) | ||
}) | ||
|
||
it('accepts a trace config', async () => { | ||
// (alexeykuzmin): All categories are excluded on purpose, | ||
// so only metadata gets into the output file. | ||
const config = { | ||
excluded_categories: ['*'] | ||
} | ||
await record(config, outputFilePath) | ||
|
||
expect(fs.existsSync(outputFilePath)).to.be.true() | ||
|
||
// If the `excluded_categories` param above is not respected | ||
// the file size will be above 50KB. | ||
const fileSizeInKiloBytes = getFileSizeInKiloBytes(outputFilePath) | ||
const expectedMaximumFileSize = 10 // Depends on a platform. | ||
|
||
expect(fileSizeInKiloBytes).to.be.above(0, | ||
`the trace output file is empty, check "${outputFilePath}"`) | ||
expect(fileSizeInKiloBytes).to.be.below(expectedMaximumFileSize, | ||
`the trace output file is suspiciously large (${fileSizeInKiloBytes}KB), | ||
check "${outputFilePath}"`) | ||
}) | ||
|
||
it('accepts "categoryFilter" and "traceOptions" as a config', async () => { | ||
// (alexeykuzmin): All categories are excluded on purpose, | ||
// so only metadata gets into the output file. | ||
const config = { | ||
categoryFilter: '__ThisIsANonexistentCategory__', | ||
traceOptions: '' | ||
} | ||
await record(config, outputFilePath) | ||
|
||
expect(fs.existsSync(outputFilePath)).to.be.true() | ||
|
||
// If the `categoryFilter` param above is not respected | ||
// the file size will be above 50KB. | ||
const fileSizeInKiloBytes = getFileSizeInKiloBytes(outputFilePath) | ||
const expectedMaximumFileSize = 10 // Depends on a platform. | ||
|
||
expect(fileSizeInKiloBytes).to.be.above(0, | ||
`the trace output file is empty, check "${outputFilePath}"`) | ||
expect(fileSizeInKiloBytes).to.be.below(expectedMaximumFileSize, | ||
`the trace output file is suspiciously large (${fileSizeInKiloBytes}KB), | ||
check "${outputFilePath}"`) | ||
}) | ||
}) | ||
|
||
describe('stopRecording', function () { | ||
this.timeout(5e3) | ||
|
||
it('calls its callback with a result file path', async () => { | ||
const resultFilePath = await record(/* options */ {}, outputFilePath) | ||
expect(resultFilePath).to.be.a('string').and.be.equal(outputFilePath) | ||
}) | ||
|
||
// FIXME(alexeykuzmin): https://github.com/electron/electron/issues/16019 | ||
xit('creates a temporary file when an empty string is passed', async function () { | ||
const resultFilePath = await record(/* options */ {}, /* outputFilePath */ '') | ||
expect(resultFilePath).to.be.a('string').that.is.not.empty() | ||
}) | ||
}) | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this come before the dictionary version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a comment in code why it should go after.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nornagon Can you take a look again?