Skip to content

appservice: Performance improvements made by copilot #2203

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.

Sign up for GitHub

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

Closed
nturinski wants to merge 2 commits into from
Closed

appservice: Performance improvements made by copilot #2203

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
faa41c7
Performance improvements made by copilot
Feb 25, 2026
6666731
Use promise.allSettled for some reslience
Feb 25, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 18 additions & 3 deletions appservice/src/createAppService/AppServicePlanListStep.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,16 +99,31 @@ export class AppServicePlanListStep extends AzureWizardPromptStep<IAppServiceWiz
}

let hasFilteredLocations: boolean = false;

// Pre-fetch OS info for all EP/WS plans in parallel (they don't have OS in their `kind`)
const epWsPlans = plans.filter(plan => plan.sku && (plan.sku.family === 'EP' || plan.sku.family === 'WS'));
const epWsOsMap = new Map<string, boolean>();
if (epWsPlans.length > 0) {
const client: WebSiteManagementClient = await createWebSiteClient(context);
const results = await Promise.allSettled(epWsPlans.map(async (plan) => {
const epPlan = await tryGetAppServicePlan(client, nonNullProp(plan, 'resourceGroup'), nonNullProp(plan, 'name'));
return { id: plan.id, isLinux: !!epPlan?.reserved };
}));
Comment on lines +107 to +111
Copy link
Copy Markdown
Contributor

@bwateratmsft bwateratmsft Feb 26, 2026

Choose a reason for hiding this comment

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

In other places, I've seen a query option "expand" to ask for Azure to add information. For example, in our cleanup tool for the BAMI subs, we use it to ask for the created time to be included in the results.

Can we see if there is an "expand" here to get the missing info, so we can do it all in the single list call?

Copy link
Copy Markdown
Contributor

@bwateratmsft bwateratmsft Feb 26, 2026

Choose a reason for hiding this comment

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

If we can't do it that way, we should probably put in a concurrency limiter (there's one in the auth pkg we can use).

Copy link
Copy Markdown
Member Author

@nturinski nturinski Feb 26, 2026

Choose a reason for hiding this comment

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

I made about 100 EP1 and WS ASPs to test this and hadn't run into any concurrency limit. But it also wasn't significantly faster than the old way, so it makes me wonder if there is something else going on here...

for (const result of results) {
if (result.status === 'fulfilled' && result.value.id) {
epWsOsMap.set(result.value.id, result.value.isLinux);
}
}
}

for (const plan of plans) {
const isNewSiteLinux: boolean = context.newSiteOS === WebsiteOS.linux;
let isPlanLinux: boolean = nonNullProp(plan, 'kind').toLowerCase().includes(WebsiteOS.linux);

if (plan.sku && (plan.sku.family === 'EP' || plan.sku.family === 'WS')) {
// elastic premium plans and workflow standard plans do not have the os in the kind, so we have to check the "reserved" property
// also, the "reserved" property is always "false" in the list of plans returned above. We have to perform a separate get on each plan
const client: WebSiteManagementClient = await createWebSiteClient(context);
const epPlan: AppServicePlan | undefined = await tryGetAppServicePlan(client, nonNullProp(plan, 'resourceGroup'), nonNullProp(plan, 'name'));
isPlanLinux = !!epPlan?.reserved;
isPlanLinux = epWsOsMap.get(plan.id ?? '') ?? false;
}

// plan.kind will contain "linux" for Linux plans, but will _not_ contain "windows" for Windows plans. Thus we check "isLinux" for both cases
Expand Down
4 changes: 2 additions & 2 deletions appservice/src/createAppService/LogAnalyticsCreateStep.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,13 @@
*--------------------------------------------------------------------------------------------*/

import { AzExtLocation, getResourceGroupFromId, LocationListStep, uiUtils } from "@microsoft/vscode-azext-azureutils";
import { AzureWizardExecuteStep, ExecuteActivityContext, nonNullProp, nonNullValueAndProp } from "@microsoft/vscode-azext-utils";
import { AzureWizardExecuteStepWithActivityOutput, ExecuteActivityContext, nonNullProp, nonNullValueAndProp } from "@microsoft/vscode-azext-utils";
import { l10n, Progress } from "vscode";
import { createOperationalInsightsManagementClient } from "../utils/azureClients";
import { getAppInsightsSupportedLocation } from "./getAppInsightsSupportedLocation";
import { IAppServiceWizardContext } from "./IAppServiceWizardContext";

export class LogAnalyticsCreateStep extends AzureWizardExecuteStep<IAppServiceWizardContext & Partial<ExecuteActivityContext>> {
export class LogAnalyticsCreateStep extends AzureWizardExecuteStepWithActivityOutput<IAppServiceWizardContext & Partial<ExecuteActivityContext>> {
Copy link

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

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

This change from AzureWizardExecuteStep to AzureWizardExecuteStepWithActivityOutput doesn't appear to be a performance improvement, unlike the other changes in this PR. The change is related to activity tracking/reporting rather than parallelization or performance optimization. Consider whether this change should be in a separate PR focused on improving activity output consistency, or if it was included here by mistake.

Copilot uses AI. Check for mistakes.
public priority: number = 134;
public stepName = 'LogAnalyticsCreateStep';
private _usedExistingLaw: boolean = false;
Expand Down
7 changes: 5 additions & 2 deletions appservice/src/pingFunctionApp.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,11 @@ import { IActionContext } from '@microsoft/vscode-azext-utils';
import { ParsedSite } from './SiteClient';

export async function pingFunctionApp(context: IActionContext, site: ParsedSite): Promise<void> {
const client = await site.createClient(context);
const genericClient = await createGenericClient(context, undefined);
// Create both clients in parallel since they are independent
const [client, genericClient] = await Promise.all([
site.createClient(context),
createGenericClient(context, undefined),
]);
const headers = createHttpHeaders({
'x-functions-key': (await client.listHostKeys()).masterKey || ''
});
Expand Down
10 changes: 7 additions & 3 deletions appservice/src/tree/DeploymentTreeItem.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,9 +118,8 @@ export class DeploymentTreeItem extends AzExtTreeItem {
});
});

let data: string = '';
for (const logEntry of logEntries) {
data += this.formatLogEntry(logEntry);
// Fetch all log entry details in parallel instead of sequentially
const detailResults = await Promise.all(logEntries.map(async (logEntry) => {
let detailedLogEntries: KuduModels.LogEntry[] = [];
await retryKuduCall(context, 'getLogEntryDetails', async () => {
await ignore404Error(context, async () => {
Expand All @@ -129,7 +128,12 @@ export class DeploymentTreeItem extends AzExtTreeItem {
}
});
});
return { logEntry, detailedLogEntries };
}));

let data: string = '';
for (const { logEntry, detailedLogEntries } of detailResults) {
data += this.formatLogEntry(logEntry);
for (const detailedEntry of detailedLogEntries) {
data += this.formatLogEntry(detailedEntry);
}
Expand Down