Skip to content

Commit

Permalink
fix: extending tracing startRecording API to take a full tracing config
Browse files Browse the repository at this point in the history
This allows memory-infra to be traced correctly.
Fixes #12506.
  • Loading branch information
ZacWalk authored and alexeykuzmin committed Nov 14, 2018
1 parent a54dd10 commit 58d03dc
Show file tree
Hide file tree
Showing 3 changed files with 170 additions and 10 deletions.
29 changes: 21 additions & 8 deletions atom/browser/api/atom_api_content_tracing.cc
Expand Up @@ -7,6 +7,7 @@

#include "atom/common/native_mate_converters/callback.h"
#include "atom/common/native_mate_converters/file_path_converter.h"
#include "atom/common/native_mate_converters/value_converter.h"
#include "base/bind.h"
#include "base/files/file_util.h"
#include "content/public/browser/tracing_controller.h"
Expand All @@ -23,15 +24,27 @@ struct Converter<base::trace_event::TraceConfig> {
static bool FromV8(v8::Isolate* isolate,
v8::Local<v8::Value> val,
base::trace_event::TraceConfig* out) {
// XXX(alexeykuzmin): A combination of "categoryFilter" and "traceOptions"
// has to be checked first because none of the fields
// in the `memory_dump_config` dict below are mandatory
// and we cannot check the config format.
Dictionary options;
if (!ConvertFromV8(isolate, val, &options))
return false;
std::string category_filter, trace_options;
if (!options.Get("categoryFilter", &category_filter) ||
!options.Get("traceOptions", &trace_options))
return false;
*out = base::trace_event::TraceConfig(category_filter, trace_options);
return true;
if (ConvertFromV8(isolate, val, &options)) {
std::string category_filter, trace_options;
if (options.Get("categoryFilter", &category_filter) &&
options.Get("traceOptions", &trace_options)) {
*out = base::trace_event::TraceConfig(category_filter, trace_options);
return true;
}
}

base::DictionaryValue memory_dump_config;
if (ConvertFromV8(isolate, val, &memory_dump_config)) {
*out = base::trace_event::TraceConfig(memory_dump_config);
return true;
}

return false;
}
};

Expand Down
11 changes: 9 additions & 2 deletions docs/api/content-tracing.md
Expand Up @@ -52,8 +52,6 @@ Once all child processes have acknowledged the `getCategories` request the
### `contentTracing.startRecording(options, callback)`

* `options` Object
* `categoryFilter` String
* `traceOptions` String
* `callback` Function

Start recording on all processes.
Expand All @@ -62,6 +60,12 @@ 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.

The `options` object can be in two formats.
1. A combination of "categoryFilter" and "traceOptions".
* `options` Object
* `categoryFilter` String
* `traceOptions` String

`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
Expand Down Expand Up @@ -91,6 +95,9 @@ 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.

2. A trace config file format.
See an example [here](https://chromium.googlesource.com/chromium/src/+/master/docs/memory-infra/memory_infra_startup_tracing.md#the-advanced-way).

### `contentTracing.stopRecording(resultFilePath, callback)`

* `resultFilePath` String
Expand Down
140 changes: 140 additions & 0 deletions spec/api-content-tracing-spec.js
@@ -0,0 +1,140 @@
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) => {
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) => {
await app.whenReady()
const recordTimeInMilliseconds = 1e3

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 () => {
// XXX(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 () => {
// XXX(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({}, outputFilePath)
expect(resultFilePath).to.be.a('string').and.be.equal(outputFilePath)
})
})
})

0 comments on commit 58d03dc

Please sign in to comment.