Copilot commented on code in PR #541: URL: https://github.com/apache/skywalking-booster-ui/pull/541#discussion_r3036177734
########## src/views/dashboard/related/pprof/components/NewTask.vue: ########## @@ -0,0 +1,177 @@ +<!-- Licensed to the Apache Software Foundation (ASF) under one or more +contributor license agreements. See the NOTICE file distributed with +this work for additional information regarding copyright ownership. +The ASF licenses this file to You under the Apache License, Version 2.0 +(the "License"); you may not use this file except in compliance with +the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. --> + +<template> + <div class="pprof-task"> + <div> + <div class="label">{{ t("instance") }}</div> + <Selector + class="profile-input" + :multiple="true" + :value="serviceInstanceIds" + size="small" + :options="pprofStore.instances" + placeholder="Select instances" + @change="changeInstances" + :filterable="true" + /> + </div> + <div> + <div class="label">{{ t("pprofEvent") }}</div> + <Radio class="mb-5" :value="eventType" :options="PprofEvents" @change="changeEventType" /> + </div> + <div v-if="requiresDuration"> + <div class="label">{{ t("duration") }}</div> + <Radio class="mb-5" :value="duration" :options="DurationOptions" @change="changeDuration" /> + <div v-if="duration === DurationOptions[5].value" class="custom-duration"> + <div class="label">{{ t("customDuration") }} ({{ t("minutes") }})</div> + <el-input + size="small" + class="profile-input" + v-model="customDurationMinutes" + type="number" + :min="1" + placeholder="Enter duration in minutes" + /> + </div> + <div class="hint">{{ t("pprofDurationHint") }}</div> + </div> + <div v-if="requiresDumpPeriod"> + <div class="label">{{ t("pprofDumpPeriod") }}</div> + <el-input + size="small" + class="profile-input" + v-model="dumpPeriod" + type="number" + :min="1" + placeholder="Enter dump period" + /> + <div class="hint"> + {{ eventType === "BLOCK" ? t("pprofDumpPeriodBlockHint") : t("pprofDumpPeriodMutexHint") }} + </div> + </div> + <div> + <el-button @click="createTask" type="primary" class="create-task-btn" :loading="loading"> + {{ t("createTask") }} + </el-button> + </div> + </div> +</template> +<script lang="ts" setup> + import { computed, ref } from "vue"; + import { useI18n } from "vue-i18n"; + import { usePprofStore } from "@/store/modules/pprof"; + import { useSelectorStore } from "@/store/modules/selectors"; + import { ElMessage } from "element-plus"; + import { DurationOptions, PprofEvents, DurationRequiredEvents, DumpPeriodRequiredEvents } from "./data"; + import type { PprofTaskCreationRequest } from "@/types/pprof"; + + /* global defineEmits */ + const emits = defineEmits(["close"]); + const pprofStore = usePprofStore(); + const selectorStore = useSelectorStore(); + const { t } = useI18n(); + const serviceInstanceIds = ref<string[]>([]); + const eventType = ref<string>(PprofEvents[0].value); + const duration = ref<string>(DurationOptions[1].value); + const customDurationMinutes = ref<number>(5); + const dumpPeriod = ref<number | string>(""); + const loading = ref<boolean>(false); + const requiresDuration = computed(() => DurationRequiredEvents.includes(eventType.value)); + const requiresDumpPeriod = computed(() => DumpPeriodRequiredEvents.includes(eventType.value)); + + function changeDuration(val: string) { + duration.value = val; + } + + function changeEventType(val: string) { + eventType.value = val; + } + + function changeInstances(options: { id: string }[]) { + serviceInstanceIds.value = options.map((d: { id: string }) => d.id); + } + + async function createTask() { + const params: PprofTaskCreationRequest = { + serviceId: selectorStore.currentService?.id || "", + serviceInstanceIds: serviceInstanceIds.value, + events: eventType.value, + }; + + if (requiresDuration.value) { + const finalDuration = + duration.value === DurationOptions[5].value ? Number(customDurationMinutes.value) : Number(duration.value); + if (!finalDuration || finalDuration < 1) { + ElMessage.error(t("invalidPprofDuration")); + return; + } + params.duration = finalDuration; + } + + if (requiresDumpPeriod.value) { + const finalDumpPeriod = Number(dumpPeriod.value); + if (!finalDumpPeriod || finalDumpPeriod < 1) { + ElMessage.error(t("invalidPprofDumpPeriod")); + return; + } + params.dumpPeriod = finalDumpPeriod; + } + + loading.value = true; + const res = await pprofStore.createTask(params); + loading.value = false; + if (res?.errors) { + ElMessage.error(res.errors); + return; + } + const result = res?.data?.task; + if (result?.errorReason) { + ElMessage.error(result.errorReason); + return; + } + emits("close"); + ElMessage.success(t("taskCreatedSuccessfully")); + } Review Comment: `createTask()` treats a falsy/undefined response from `pprofStore.createTask()` as success (it will still close the dialog and show a success toast). This can happen when `serviceId` is empty (store action returns early) or if the action returns `undefined` for any reason. Add a guard that verifies the response has expected data (e.g., `res?.data?.task`) before showing success / closing, and show an error message otherwise. ########## src/views/dashboard/related/pprof/components/PprofStack.vue: ########## @@ -0,0 +1,164 @@ +<!-- Licensed to the Apache Software Foundation (ASF) under one or more +contributor license agreements. See the NOTICE file distributed with +this work for additional information regarding copyright ownership. +The ASF licenses this file to You under the Apache License, Version 2.0 +(the "License"); you may not use this file except in compliance with +the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. --> +<template> + <div ref="graph"></div> +</template> +<script lang="ts" setup> + import { ref, watch } from "vue"; + import * as d3 from "d3"; + import d3tip from "d3-tip"; + import { flamegraph } from "d3-flame-graph"; + import { usePprofStore } from "@/store/modules/pprof"; + import type { PprofStackElement, PprofFlameGraphNode } from "@/types/pprof"; + import "d3-flame-graph/dist/d3-flamegraph.css"; + import { treeForeach } from "@/utils/flameGraph"; + + /*global Nullable*/ + const pprofStore = usePprofStore(); + const stackTree = ref<Nullable<PprofFlameGraphNode>>(null); + const selectStack = ref<Nullable<PprofFlameGraphNode>>(null); + const graph = ref<Nullable<HTMLDivElement>>(null); + const flameChart = ref<any>(null); + const min = ref<number>(1); + const max = ref<number>(1); + + function drawGraph() { + if (flameChart.value) { + flameChart.value.destroy(); + } + if (!pprofStore.analyzeTrees.length || !graph.value) { + stackTree.value = null; + return; + } + const root: PprofFlameGraphNode = { + parentId: "0", + originId: "1", + name: "Virtual Root", + children: [], + value: 0, + id: "1", + symbol: "Virtual Root", + dumpCount: 0, + self: 0, + }; + countRange(); + const elements = processTree((pprofStore.analyzeTrees[0].elements || []) as PprofStackElement[]); + if (!elements) { + stackTree.value = null; + return; + } + stackTree.value = elements; + const treeRoot = { ...root, ...elements }; + const width = graph.value.getBoundingClientRect().width || 0; + const w = width < 800 ? 802 : width; + flameChart.value = flamegraph() + .width(w - 15) + .cellHeight(18) + .transitionDuration(750) + .minFrameSize(1) + .transitionEase(d3.easeCubic as any) + .sort(true) + .title("") + .selfValue(false) + .inverted(true) + .onClick((d: { data: PprofFlameGraphNode }) => { + selectStack.value = d.data; + }) + .setColorMapper((d, originalColor) => (d.highlight ? "#6aff8f" : originalColor)); + const tip = (d3tip as any)() + .attr("class", "d3-tip") + .direction("s") + .html((d: { data: PprofFlameGraphNode } & { parent: { data: PprofFlameGraphNode } }) => { + const name = d.data.name.replace("<", "<").replace(">", ">"); + const rateOfParent = + (d.parent && + `<div class="mb-5">Percentage Of Selected: ${ + ( + (d.data.dumpCount / ((selectStack.value && selectStack.value.dumpCount) || treeRoot.dumpCount)) * + 100 + ).toFixed(3) + "%" + }</div>`) || + ""; + const rateOfRoot = `<div class="mb-5">Percentage Of Root: ${ + ((d.data.dumpCount / treeRoot.dumpCount) * 100).toFixed(3) + "%" + }</div>`; + return `<div class="mb-5 name">Symbol: ${name}</div> + <div class="mb-5">Total: ${d.data.dumpCount}</div> + <div class="mb-5">Self: ${d.data.self}</div> + ${rateOfParent}${rateOfRoot}`; + }) + .style("max-width", "400px"); + flameChart.value.tooltip(tip); + d3.select(graph.value).datum(treeRoot).call(flameChart.value); + } + + function countRange() { + const list = (pprofStore.analyzeTrees[0]?.elements || []).map((ele: PprofStackElement) => ele.dumpCount); + max.value = Math.max(...(list.length ? list : [1])); + min.value = Math.min(...(list.length ? list : [1])); + } + + function processTree(arr: PprofStackElement[]): Nullable<PprofFlameGraphNode> { + const copyArr = JSON.parse(JSON.stringify(arr)); + const obj: Record<string, PprofFlameGraphNode> = {}; + let res = null as Nullable<PprofFlameGraphNode>; + for (const item of copyArr) { + item.parentId = String(Number(item.parentId) + 1); + item.originId = String(Number(item.id) + 1); + item.name = item.symbol; + delete item.id; + obj[item.originId] = item; + } + const scale = d3.scaleLinear().domain([min.value, max.value]).range([1, 200]); + for (const item of copyArr) { + if (item.parentId === "0") { + const val = Number(scale(item.dumpCount).toFixed(4)); + item.value = val; + res = item as PprofFlameGraphNode; + } + for (const key in obj) { + if (item.originId === obj[key].parentId) { + const val = Number(scale(obj[key].dumpCount).toFixed(4)); + obj[key].value = val; + if (item.children) { + item.children.push(obj[key]); + } else { + item.children = [obj[key]]; + } + } Review Comment: The child-linking logic uses a nested loop (`for (const item of copyArr)` and then iterating all keys in `obj`), which is O(n²). With large `elements` this can cause slow rendering / UI jank. Consider building a `parentId -> children[]` adjacency map to attach children in O(n). ```suggestion const childrenMap: Record<string, PprofFlameGraphNode[]> = {}; let res = null as Nullable<PprofFlameGraphNode>; const scale = d3.scaleLinear().domain([min.value, max.value]).range([1, 200]); for (const item of copyArr) { item.parentId = String(Number(item.parentId) + 1); item.originId = String(Number(item.id) + 1); item.name = item.symbol; delete item.id; const node = item as PprofFlameGraphNode; node.value = Number(scale(node.dumpCount).toFixed(4)); obj[node.originId] = node; if (childrenMap[node.parentId]) { childrenMap[node.parentId].push(node); } else { childrenMap[node.parentId] = [node]; } } for (const item of copyArr) { const node = obj[item.originId]; if (node.parentId === "0") { res = node; } if (childrenMap[node.originId]) { node.children = childrenMap[node.originId]; ``` ########## src/views/dashboard/related/pprof/components/PprofStack.vue: ########## @@ -0,0 +1,164 @@ +<!-- Licensed to the Apache Software Foundation (ASF) under one or more +contributor license agreements. See the NOTICE file distributed with +this work for additional information regarding copyright ownership. +The ASF licenses this file to You under the Apache License, Version 2.0 +(the "License"); you may not use this file except in compliance with +the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. --> +<template> + <div ref="graph"></div> +</template> +<script lang="ts" setup> + import { ref, watch } from "vue"; + import * as d3 from "d3"; + import d3tip from "d3-tip"; + import { flamegraph } from "d3-flame-graph"; + import { usePprofStore } from "@/store/modules/pprof"; + import type { PprofStackElement, PprofFlameGraphNode } from "@/types/pprof"; + import "d3-flame-graph/dist/d3-flamegraph.css"; + import { treeForeach } from "@/utils/flameGraph"; + + /*global Nullable*/ + const pprofStore = usePprofStore(); + const stackTree = ref<Nullable<PprofFlameGraphNode>>(null); + const selectStack = ref<Nullable<PprofFlameGraphNode>>(null); + const graph = ref<Nullable<HTMLDivElement>>(null); + const flameChart = ref<any>(null); + const min = ref<number>(1); + const max = ref<number>(1); + + function drawGraph() { + if (flameChart.value) { + flameChart.value.destroy(); + } + if (!pprofStore.analyzeTrees.length || !graph.value) { + stackTree.value = null; + return; + } + const root: PprofFlameGraphNode = { + parentId: "0", + originId: "1", + name: "Virtual Root", + children: [], + value: 0, + id: "1", + symbol: "Virtual Root", + dumpCount: 0, + self: 0, + }; + countRange(); + const elements = processTree((pprofStore.analyzeTrees[0].elements || []) as PprofStackElement[]); + if (!elements) { + stackTree.value = null; + return; + } + stackTree.value = elements; + const treeRoot = { ...root, ...elements }; + const width = graph.value.getBoundingClientRect().width || 0; + const w = width < 800 ? 802 : width; + flameChart.value = flamegraph() + .width(w - 15) + .cellHeight(18) + .transitionDuration(750) + .minFrameSize(1) + .transitionEase(d3.easeCubic as any) + .sort(true) + .title("") + .selfValue(false) + .inverted(true) + .onClick((d: { data: PprofFlameGraphNode }) => { + selectStack.value = d.data; + }) + .setColorMapper((d, originalColor) => (d.highlight ? "#6aff8f" : originalColor)); + const tip = (d3tip as any)() + .attr("class", "d3-tip") + .direction("s") + .html((d: { data: PprofFlameGraphNode } & { parent: { data: PprofFlameGraphNode } }) => { + const name = d.data.name.replace("<", "<").replace(">", ">"); + const rateOfParent = + (d.parent && Review Comment: The tooltip HTML only escapes the first occurrences of "<" and ">" in `d.data.name`, leaving the rest of the string unescaped (and not escaping `&`/quotes). Since the tooltip is rendered as HTML, this can allow HTML injection if `codeSignature` contains unexpected characters. Use a proper HTML-escape helper (escape all occurrences, including `&`, `<`, `>`, `"`, `'`) before interpolating into the tooltip HTML. ########## src/views/dashboard/related/pprof/components/PprofStack.vue: ########## @@ -0,0 +1,164 @@ +<!-- Licensed to the Apache Software Foundation (ASF) under one or more +contributor license agreements. See the NOTICE file distributed with +this work for additional information regarding copyright ownership. +The ASF licenses this file to You under the Apache License, Version 2.0 +(the "License"); you may not use this file except in compliance with +the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. --> +<template> + <div ref="graph"></div> +</template> +<script lang="ts" setup> + import { ref, watch } from "vue"; + import * as d3 from "d3"; + import d3tip from "d3-tip"; + import { flamegraph } from "d3-flame-graph"; + import { usePprofStore } from "@/store/modules/pprof"; + import type { PprofStackElement, PprofFlameGraphNode } from "@/types/pprof"; + import "d3-flame-graph/dist/d3-flamegraph.css"; + import { treeForeach } from "@/utils/flameGraph"; + + /*global Nullable*/ + const pprofStore = usePprofStore(); + const stackTree = ref<Nullable<PprofFlameGraphNode>>(null); + const selectStack = ref<Nullable<PprofFlameGraphNode>>(null); + const graph = ref<Nullable<HTMLDivElement>>(null); + const flameChart = ref<any>(null); + const min = ref<number>(1); + const max = ref<number>(1); + + function drawGraph() { + if (flameChart.value) { + flameChart.value.destroy(); + } + if (!pprofStore.analyzeTrees.length || !graph.value) { + stackTree.value = null; + return; + } + const root: PprofFlameGraphNode = { + parentId: "0", + originId: "1", + name: "Virtual Root", + children: [], + value: 0, + id: "1", + symbol: "Virtual Root", + dumpCount: 0, + self: 0, + }; + countRange(); + const elements = processTree((pprofStore.analyzeTrees[0].elements || []) as PprofStackElement[]); + if (!elements) { + stackTree.value = null; + return; + } + stackTree.value = elements; + const treeRoot = { ...root, ...elements }; + const width = graph.value.getBoundingClientRect().width || 0; + const w = width < 800 ? 802 : width; + flameChart.value = flamegraph() + .width(w - 15) + .cellHeight(18) + .transitionDuration(750) + .minFrameSize(1) + .transitionEase(d3.easeCubic as any) + .sort(true) + .title("") + .selfValue(false) + .inverted(true) + .onClick((d: { data: PprofFlameGraphNode }) => { + selectStack.value = d.data; + }) + .setColorMapper((d, originalColor) => (d.highlight ? "#6aff8f" : originalColor)); + const tip = (d3tip as any)() + .attr("class", "d3-tip") + .direction("s") + .html((d: { data: PprofFlameGraphNode } & { parent: { data: PprofFlameGraphNode } }) => { + const name = d.data.name.replace("<", "<").replace(">", ">"); + const rateOfParent = + (d.parent && + `<div class="mb-5">Percentage Of Selected: ${ + ( + (d.data.dumpCount / ((selectStack.value && selectStack.value.dumpCount) || treeRoot.dumpCount)) * + 100 + ).toFixed(3) + "%" + }</div>`) || + ""; + const rateOfRoot = `<div class="mb-5">Percentage Of Root: ${ + ((d.data.dumpCount / treeRoot.dumpCount) * 100).toFixed(3) + "%" + }</div>`; + return `<div class="mb-5 name">Symbol: ${name}</div> + <div class="mb-5">Total: ${d.data.dumpCount}</div> + <div class="mb-5">Self: ${d.data.self}</div> + ${rateOfParent}${rateOfRoot}`; + }) + .style("max-width", "400px"); + flameChart.value.tooltip(tip); + d3.select(graph.value).datum(treeRoot).call(flameChart.value); + } + + function countRange() { + const list = (pprofStore.analyzeTrees[0]?.elements || []).map((ele: PprofStackElement) => ele.dumpCount); + max.value = Math.max(...(list.length ? list : [1])); + min.value = Math.min(...(list.length ? list : [1])); + } + + function processTree(arr: PprofStackElement[]): Nullable<PprofFlameGraphNode> { + const copyArr = JSON.parse(JSON.stringify(arr)); + const obj: Record<string, PprofFlameGraphNode> = {}; + let res = null as Nullable<PprofFlameGraphNode>; + for (const item of copyArr) { + item.parentId = String(Number(item.parentId) + 1); + item.originId = String(Number(item.id) + 1); + item.name = item.symbol; + delete item.id; + obj[item.originId] = item; + } + const scale = d3.scaleLinear().domain([min.value, max.value]).range([1, 200]); + for (const item of copyArr) { + if (item.parentId === "0") { + const val = Number(scale(item.dumpCount).toFixed(4)); + item.value = val; + res = item as PprofFlameGraphNode; + } + for (const key in obj) { + if (item.originId === obj[key].parentId) { + const val = Number(scale(obj[key].dumpCount).toFixed(4)); + obj[key].value = val; + if (item.children) { + item.children.push(obj[key]); + } else { + item.children = [obj[key]]; + } Review Comment: `processTree()` deep-copies the full elements array via `JSON.parse(JSON.stringify(arr))`, which can be expensive for large profiling trees. Consider avoiding JSON serialization (e.g., build a new array via `map` or operate on a normalized structure) to reduce CPU time and allocations. ```suggestion const copyArr = arr.map( (item) => ({ ...item, parentId: String(Number(item.parentId) + 1), originId: String(Number(item.id) + 1), name: item.symbol, }) as PprofFlameGraphNode, ); const obj: Record<string, PprofFlameGraphNode> = {}; let res = null as Nullable<PprofFlameGraphNode>; for (const item of copyArr) { obj[item.originId] = item; } const scale = d3.scaleLinear().domain([min.value, max.value]).range([1, 200]); for (const item of copyArr) { const val = Number(scale(item.dumpCount).toFixed(4)); item.value = val; if (item.parentId === "0") { res = item; continue; } const parent = obj[item.parentId]; if (parent) { if (parent.children) { parent.children.push(item); } else { parent.children = [item]; ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
