-
Notifications
You must be signed in to change notification settings - Fork 55
Perfetto controls #679
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
base: master
Are you sure you want to change the base?
Perfetto controls #679
Conversation
| "default": { | ||
| "enable": false, | ||
| "dir": "${workspaceFolder}/profiles", | ||
| "filename": "${appTitle}_${timestamp}.prof" |
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.
please use ".perfetto-trace" instead of ".prof" for the filename suffix.
| "filename": { | ||
| "type": "string", | ||
| "description": "The name of the profile file. Can include variables like ${appTitle} and ${timestamp}", | ||
| "default": "${appTitle}_${timestamp}.prof" |
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.
Please use ".perfetto-trace" instead of ".prof" for filename suffix.
| "filename": { | ||
| "type": "string", | ||
| "description": "The name of the profile file. Can include variables like ${appTitle} and ${timestamp}", | ||
| "default": "${appTitle}_${timestamp}.prof" |
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.
Please use ".perfetto-trace" instead of ".prof" for filename suffix.
| profiling: { | ||
| enable: false, | ||
| dir: '${workspaceFolder}/profiles/', | ||
| filename: '${appTitle}-${timestamp}.bsprof' |
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.
Please ".perfetto-trace" instead of ".bsprof" for trace filename suffix.
Also, later in the code when testing for profiling, what should the default behavior be if the filename and/or dir are missing? What's the default?
| const startResponse = await perfettoController.startTracing(); | ||
|
|
||
| if (startResponse.error) { | ||
| this.showError('Error starting perfetto tracing: ' + startResponse.message); |
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.
Can the error messages be made more consistent? In some cases we say "perfetto tracing", in others, it's "Perfetto tracing, and in other places it's "Tracing" or "Perfetto tracing". If someone's doing a case-sensitive filter, this can lead to a needlessly painful debugging experience.
Also, perhaps a good sign that this logic needs to be made a bit more DRY? Maybe use a Template Method pattern? Im thinking something like this (so we have only a couple places where the messages are printed, reducing the chance for variability, and making the code a bit tighter?):
private async handleTracingOperation(
operationName: string,
operation: () => Promise
): Promise {
this.showInfo(${operationName} perfetto Tracing!);
const response = await operation();
if (response.error) {
this.showError(`Error ${operationName.toLowerCase()} perfetto tracing: ${response.message}`);
} else {
this.showInfo(`Perfetto tracing ${operationName.toLowerCase()}d: ${response.message}`);
}
}
public async executeCommand(command: string): Promise<void> {
switch (command) {
case 'start':
await this.handleTracingOperation('Starting', () => this.perfettoController.startTracing());
break;
case 'stop':
await this.handleTracingOperation('Stopping', () => this.perfettoController.stopTracing());
break;
case 'enable':
await this.handleTracingOperation('Enabling', () => this.perfettoController.enableTracing());
break;
// Add other cases here
default:
this.showError(`Unknown command: ${command}`);
break;
}
}
}
| } | ||
| rokuDeployOptions.packageConfig = 'launch.json: ' + selectedConfig.rootDir; | ||
|
|
||
| if (selectedConfig?.profiling?.dir.includes('${workspaceFolder}')) { |
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.
What should happen if profiling is enabled but there's no dir? Does the DebugConfigurationProvider.ts value become the default for any missing fields? E.g. if the developer's launch.json has:
profiling: { enable: true}
will the dir and filename be filled by default from DebugConfigurationProvider.ts as:
profiling: {
enable: true, <--- this is from developer's launch.json
dir: '${workspaceFolder}/profiles/', <--- populated from DebugConfigurationProvider.ts?
filename: '${appTitle}-${timestamp}.bsprof' <--- populated from DebugConfigurationProvider.ts?
}
Support for enable, start and stop perfetto feature.