Skip to content
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

Merged
merged 1 commit into from Dec 20, 2018

Conversation

ZacWalk
Copy link
Contributor

@ZacWalk ZacWalk commented Aug 2, 2018

This allows memory-infra to be traced correctly. For memory-infra to be traced memory_dump_config and triggers need to be defined.
Fixes #12506.

Example usage:

const options = {
  "record_mode": "record-until-full",
  "enable_systrace": true,
  "enable_argument_filter": false,
  "excluded_categories": ["*"],
  "included_categories": ["disabled-by-default-memory-infra"],
  "memory_dump_config": {allowed_dump_modes: ["background", "light", "detailed"],
  "triggers": [{min_time_between_dumps_ms: 1000,  "mode": "detailed", "type": "periodic_interval"}]},
}

contentTracing.startRecording(options, () => { })

More? docs https://chromium.googlesource.com/chromium/src/+/master/docs/memory-infra/memory_infra_startup_tracing.md#the-advanced-way

  • PR description included and stakeholders cc'd
  • npm test passes
  • tests are changed or added
  • relevant documentation is changed or added
  • PR title follows semantic commit guidelines

Release Notes

Notes: allow passing a trace config as options to a "contentTracing.startRecording" call

@ZacWalk ZacWalk changed the title [WIP] fix: Extending tracing startRecording API to take a full tracing config. fix: Extending tracing startRecording API to take a full tracing config. Oct 5, 2018
@codebytere
Copy link
Member

@ZacWalk would you still like to land this?

@alexeykuzmin
Copy link
Contributor

@codebytere I'm going to land this.

Copy link
Member

@nornagon nornagon left a comment

Choose a reason for hiding this comment

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

needs a test.

}

base::DictionaryValue memory_dump_config;
if (ConvertFromV8(isolate, val, &memory_dump_config)) {
Copy link
Member

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?

Copy link
Contributor

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.

Copy link
Contributor

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?

@alexeykuzmin
Copy link
Contributor

needs a test.

Yeah. That's why two checkboxes in the description are left unchecked, and the wip label is there =)

@ZacWalk ZacWalk requested a review from a team November 13, 2018 18:21
@alexeykuzmin alexeykuzmin self-assigned this Nov 13, 2018
@alexeykuzmin alexeykuzmin added semver/minor backwards-compatible functionality and removed semver/minor backwards-compatible functionality labels Nov 13, 2018
@alexeykuzmin alexeykuzmin force-pushed the fix-memory-tracing branch 2 times, most recently from e84d93d to 58d03dc Compare November 14, 2018 11:11
@@ -52,8 +52,6 @@ Once all child processes have acknowledged the `getCategories` request the
### `contentTracing.startRecording(options, callback)`

* `options` Object
* `categoryFilter` String
* `traceOptions` String
Copy link
Contributor

Choose a reason for hiding this comment

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

@MarshallOfSound How properly reflect in the docs the fact that the options Object can have two different formats?
First format is {categoryFilter: "...", traceOptions: "..."}, another one something like

{
    "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 }
      ]
}

Copy link
Member

Choose a reason for hiding this comment

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

It's a little 😬 that one of these formats uses camelCase and the other uses snake_case. Is it worth converting one of them?

Copy link
Contributor

Choose a reason for hiding this comment

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

@nornagon the {categoryFilter: "...", traceOptions: "..."} variant is the only currently available, I don't want to break existing code.
The other one is a Chromium thing, and I don't want to overcomplicate it by adding format converting logic.

@alexeykuzmin alexeykuzmin changed the title fix: Extending tracing startRecording API to take a full tracing config. fix: extend tracing startRecording API to take a full tracing config Nov 14, 2018
@alexeykuzmin
Copy link
Contributor

alexeykuzmin commented Nov 21, 2018

@MarshallOfSound What does this error mean? How we can fix it?

electron-lint job

test/main.ts(470,31): error TS2345: Argument of type '{ categoryFilter: string; traceOptions: string; }' is not assignable to parameter of type 'TraceCategoriesAndOptions | TraceConfig'.
Type '{ categoryFilter: string; traceOptions: string; }' has no properties in common with type 'TraceConfig'.

@codebytere
Copy link
Member

codebytere commented Nov 21, 2018

@alexeykuzmin it's erroring here; you'll need to update that test to correspond to the new parameters and structures introduced in this PR

Copy link
Member

@nornagon nornagon left a comment

Choose a reason for hiding this comment

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

Should we deprecate the old way of doing it or are they both useful in some cases?

atom/browser/api/atom_api_content_tracing.cc Outdated Show resolved Hide resolved
spec/api-content-tracing-spec.js Outdated Show resolved Hide resolved
spec/api-content-tracing-spec.js Outdated Show resolved Hide resolved
@@ -52,8 +52,6 @@ Once all child processes have acknowledged the `getCategories` request the
### `contentTracing.startRecording(options, callback)`

* `options` Object
* `categoryFilter` String
* `traceOptions` String
Copy link
Member

Choose a reason for hiding this comment

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

It's a little 😬 that one of these formats uses camelCase and the other uses snake_case. Is it worth converting one of them?

@alexeykuzmin
Copy link
Contributor

LGTM but 4.0.0 is frozen and this isn't a blocker, so let's track this for 4.0.1

Ok, but I'll create a PR anyway. We'll merge it after the stable release then.

@alexeykuzmin alexeykuzmin merged commit 51cfb5c into master Dec 20, 2018
@release-clerk
Copy link

release-clerk bot commented Dec 20, 2018

Release Notes Persisted

allow passing a trace config as options to a "contentTracing.startRecording" call

@trop
Copy link
Contributor

trop bot commented Dec 20, 2018

I have automatically backported this PR to "3-0-x", please check out #16157

@trop trop bot removed the target/3-0-x label Dec 20, 2018
@trop
Copy link
Contributor

trop bot commented Dec 20, 2018

I have automatically backported this PR to "4-0-x", please check out #16158

@alexeykuzmin
Copy link
Contributor

/trop run backport-to 3-1-x

@trop
Copy link
Contributor

trop bot commented Dec 20, 2018

The backport process for this PR has been manually initiated,
sending your 1's and 0's to "3-1-x" here we go! :D

@trop
Copy link
Contributor

trop bot commented Dec 20, 2018

I have automatically backported this PR to "3-1-x", please check out #16159

@sofianguy sofianguy added this to Fixed (3.0.14) in 3.0.x / 3.1.x Jan 18, 2019
@sofianguy sofianguy added this to Fixed in 3.1.0-beta.5 in 3.1.x Jan 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
3.0.x / 3.1.x
Fixed (3.0.14)
3.1.x
Fixed in 3.1.0-beta.5
Development

Successfully merging this pull request may close these issues.

None yet

6 participants