Copilot commented on code in PR #809: URL: https://github.com/apache/skywalking-banyandb/pull/809#discussion_r2438257369
########## ui/src/components/Trace/Editor.vue: ########## @@ -0,0 +1,259 @@ +<!-- + ~ Licensed to 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. Apache Software Foundation (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. +--> +<script setup> + import { reactive, ref, onMounted } from 'vue'; + import { ElMessage } from 'element-plus'; + import { getCurrentInstance } from '@vue/runtime-core'; + import { useRoute, useRouter } from 'vue-router'; + import { updateTrace, createTrace, getTrace } from '@/api'; + import FormHeader from '../common/FormHeader.vue'; + import TagEditor from './TagEditor.vue'; + import { rules, formConfig, traceFieldsConfig } from './data'; + + const $loadingCreate = getCurrentInstance().appContext.config.globalProperties.$loadingCreate; + const $loadingClose = getCurrentInstance().appContext.config.globalProperties.$loadingClose; + const $bus = getCurrentInstance().appContext.config.globalProperties.mittBus; + const route = useRoute(); + const router = useRouter(); + const tagEditorRef = ref(); + const ruleForm = ref(); + const { operator, name, group, type } = route.params; + const formData = reactive({ + group: group || '', + operator, + type, + name: name || '', + traceIdTagName: '', + timestampTagName: '', + spanIdTagName: '', + tags: [], + }); + + async function initTrace() { + if (operator === 'edit') { + $loadingCreate(); + const response = await getTrace(group, name); + $loadingClose(); + if (response.error) { + ElMessage({ + message: response.error.message, + type: 'error', + duration: 3000, + }); + return; + } + const { trace } = response; + Object.assign(formData, { + traceIdTagName: trace.traceIdTagName, + timestampTagName: trace.timestampTagName, + spanIdTagName: trace.spanIdTagName, + tags: trace.tags.map((d) => ({ + ...d, + key: d.name, + value: d.type, + })), + }); + } + } + const openEditTag = (index) => { + tagEditorRef.value.openDialog(formData.tags[index]).then((res) => { + formData.tags[index] = res; + }); + }; + const deleteTag = (index) => { + formData.tags.splice(index, 1); + }; + const openAddTag = () => { + tagEditorRef.value.openDialog().then((res) => { + formData.tags.push(res); + }); + }; + const submit = async () => { + if (!ruleForm.value) return; + await ruleForm.value.validate(async (valid) => { + if (valid) { + $loadingCreate(); + const param = { + trace: { + metadata: { + group: formData.group, + name: formData.name, + }, + tags: formData.tags.map((d) => ({ name: d.key, type: d.value })), + traceIdTagName: formData.traceIdTagName, + timestampTagName: formData.timestampTagName, + spanIdTagName: formData.spanIdTagName, + }, + }; + if (operator === 'create') { + const response = await createTrace(param); + $loadingClose(); + if (response.error) { + ElMessage({ + message: response.error.message, + type: 'error', + duration: 3000, + }); + return; + } + ElMessage({ + message: 'Created successfully', + type: 'success', + duration: 5000, + }); + $bus.emit('refreshAside'); + $bus.emit('deleteGroup', formData.group); + openResources(); + return; + } + const response = await updateTrace(formData.group, formData.name, param); + $loadingClose(); + if (response.error) { + ElMessage({ + message: response.error.message, + type: 'error', + duration: 3000, + }); + return; + } + ElMessage({ + message: 'Updated successfully', + type: 'success', + duration: 5000, + }); + $bus.emit('refreshAside'); + $bus.emit('deleteResource', formData.name); + openResources(); + } + }); + } + function openResources() { + const routeConfig = { + name: formData.type, + params: { + group: formData.group, + name: formData.name, + operator: 'read', + type: formData.type + '', + }, + }; + router.push(routeConfig); + const add = { + label: formData.name, + type: 'Read', + route: routeConfig, + }; + $bus.emit('AddTabs', add); + } + onMounted(() => { + initTrace(); + }); +</script> +<template> + <div> + <el-card shadow="always"> + <template #header> + <el-row> + <el-col :span="20"> + <FormHeader :fields="{ ...formData, catalog: formData.type }" /> + </el-col> + <el-col :span="4"> + <div class="flex align-item-center justify-end" style="height: 30px"> + <el-button size="small" type="primary" @click="submit(ruleFormRef)" color="#6E38F7">Submit</el-button> Review Comment: The template calls submit(ruleFormRef) but ruleFormRef is not defined; this can evaluate to an undefined reference in the template and is unnecessary since submit references ruleForm internally. Call submit without arguments. ```suggestion <el-button size="small" type="primary" @click="submit()" color="#6E38F7">Submit</el-button> ``` ########## ui/src/api/base.js: ########## @@ -0,0 +1,73 @@ +/* + * Licensed to 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. Apache Software Foundation (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. + */ + +const Timeout = 2 * 60 * 1000; +export let globalAbortController = new AbortController(); +export function abortRequestsAndUpdate() { + globalAbortController.abort(`Request timeout ${Timeout}ms`); + globalAbortController = new AbortController(); +} +class HTTPError extends Error { + response; + + constructor(response, detailText = "") { + super(detailText || response.statusText); + + this.name = "HTTPError"; + this.response = response; + } +} + +export const BasePath = `/graphql`; + +export async function httpQuery({ + url = "", + method = "GET", + json, + headers = {}, +}) { + const timeoutId = setTimeout(() => { + abortRequestsAndUpdate(); + }, Timeout); + + const response = await fetch(url, { + method, + headers: { + "Content-Type": "application/json", + accept: "application/json", + ...headers, + }, + body: JSON.stringify(json), + signal: globalAbortController.signal, + }) + .catch((error) => { + throw new HTTPError(error); + }) + .finally(() => { + clearTimeout(timeoutId); + }); + if (response.ok) { + return response.json(); + } else { + console.error(new HTTPError(response)); + return { + error: new HTTPError(response), + }; + } Review Comment: httpQuery throws on network/abort errors, but callers (e.g., getTrace, createTrace, updateTrace, queryTraces) expect a return object with an error field. Replace the throw with returning { error: new HTTPError(error) } so callers can uniformly handle errors and UI code (e.g., loading overlays) doesn't get stuck on exceptions. ########## ui/src/components/Trace/TraceRead.vue: ########## @@ -0,0 +1,342 @@ +<!-- + ~ Licensed to 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. Apache Software Foundation (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. +--> + +<script setup> + import { queryTraces, getindexRuleList } from '@/api/index'; + import { getCurrentInstance } from '@vue/runtime-core'; + import { useRoute } from 'vue-router'; + import { ElMessage } from 'element-plus'; + import { reactive, ref, watch } from 'vue'; + import { RefreshRight, Search, Download } from '@element-plus/icons-vue'; + import { jsonToYaml, yamlToJson } from '@/utils/yaml'; + import CodeMirror from '@/components/CodeMirror/index.vue'; + import FormHeader from '../common/FormHeader.vue'; + import { Last15Minutes, Shortcuts } from '../common/data'; + import JSZip from 'jszip'; + + const { proxy } = getCurrentInstance(); + const route = useRoute(); + const $loadingCreate = getCurrentInstance().appContext.config.globalProperties.$loadingCreate; + const $loadingClose = proxy.$loadingClose; + const yamlRef = ref(null); + const timeRange = ref([]); + const data = reactive({ + group: route.params.group, + tableData: [], + name: route.params.name, + indexRule: '', + }); + const yamlCode = ref(``); + const selectedSpans = ref({}); + + const getTraces = async (params) => { + $loadingCreate(); + const response = await queryTraces({ groups: [data.group], name: data.name, ...params }) + $loadingClose(); + if (response.error) { + data.tableData = []; + ElMessage({ + message: response.error.message, + type: 'error', + duration: 3000, + }); + return; + } + data.tableData = response.traces || []; + }; + + const getIndexRule = async () => { + if (!data.group) { + return; + } + try { + const response = await getindexRuleList(data.group); + if (response.status === 200 && response.data.indexRule && response.data.indexRule.length > 0) { + data.indexRule = response.data.indexRule[0].metadata; + } else { + data.indexRule = ''; + } + } catch (err) { + console.error('Failed to fetch indexRule:', err); + data.indexRule = ''; + ElMessage({ + message: 'Failed to fetch index rule: ' + err, + type: 'error', + duration: 3000, + }); + } + }; + + function searchTraces() { + yamlRef.value + .checkYaml(yamlCode.value) + .then(() => { + const json = yamlToJson(yamlCode.value).data; + getTraces(json); + }) + .catch((err) => { + ElMessage({ + dangerouslyUseHTMLString: true, + showClose: true, + message: `<div>${err.message}</div>`, Review Comment: Using dangerouslyUseHTMLString with an error message allows injection of arbitrary HTML. Prefer plain text messages to prevent XSS (e.g., remove dangerouslyUseHTMLString and pass message: err.message). ```suggestion showClose: true, message: err.message, ``` ########## ui/src/components/Trace/TraceRead.vue: ########## @@ -0,0 +1,342 @@ +<!-- + ~ Licensed to 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. Apache Software Foundation (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. +--> + +<script setup> + import { queryTraces, getindexRuleList } from '@/api/index'; + import { getCurrentInstance } from '@vue/runtime-core'; + import { useRoute } from 'vue-router'; + import { ElMessage } from 'element-plus'; + import { reactive, ref, watch } from 'vue'; + import { RefreshRight, Search, Download } from '@element-plus/icons-vue'; + import { jsonToYaml, yamlToJson } from '@/utils/yaml'; + import CodeMirror from '@/components/CodeMirror/index.vue'; + import FormHeader from '../common/FormHeader.vue'; + import { Last15Minutes, Shortcuts } from '../common/data'; + import JSZip from 'jszip'; + + const { proxy } = getCurrentInstance(); + const route = useRoute(); + const $loadingCreate = getCurrentInstance().appContext.config.globalProperties.$loadingCreate; + const $loadingClose = proxy.$loadingClose; + const yamlRef = ref(null); + const timeRange = ref([]); + const data = reactive({ + group: route.params.group, + tableData: [], + name: route.params.name, + indexRule: '', + }); + const yamlCode = ref(``); + const selectedSpans = ref({}); + + const getTraces = async (params) => { + $loadingCreate(); + const response = await queryTraces({ groups: [data.group], name: data.name, ...params }) + $loadingClose(); Review Comment: If queryTraces throws (e.g., on abort/network error), $loadingClose() is never called and the loading overlay can remain indefinitely. Wrap the call with try/finally to ensure the overlay closes even on failure. ```suggestion let response; try { response = await queryTraces({ groups: [data.group], name: data.name, ...params }); } catch (err) { ElMessage({ message: 'Failed to fetch traces: ' + (err && err.message ? err.message : err), type: 'error', duration: 3000, }); data.tableData = []; return; } finally { $loadingClose(); } ``` ########## ui/src/components/Trace/Editor.vue: ########## @@ -0,0 +1,259 @@ +<!-- + ~ Licensed to 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. Apache Software Foundation (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. +--> +<script setup> + import { reactive, ref, onMounted } from 'vue'; + import { ElMessage } from 'element-plus'; + import { getCurrentInstance } from '@vue/runtime-core'; Review Comment: Import getCurrentInstance from 'vue' instead of '@vue/runtime-core' to use the public API and avoid relying on internal packages. ```suggestion import { getCurrentInstance } from 'vue'; ``` ########## ui/src/components/Trace/TraceRead.vue: ########## @@ -0,0 +1,342 @@ +<!-- + ~ Licensed to 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. Apache Software Foundation (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. +--> + +<script setup> + import { queryTraces, getindexRuleList } from '@/api/index'; + import { getCurrentInstance } from '@vue/runtime-core'; + import { useRoute } from 'vue-router'; + import { ElMessage } from 'element-plus'; + import { reactive, ref, watch } from 'vue'; + import { RefreshRight, Search, Download } from '@element-plus/icons-vue'; + import { jsonToYaml, yamlToJson } from '@/utils/yaml'; + import CodeMirror from '@/components/CodeMirror/index.vue'; + import FormHeader from '../common/FormHeader.vue'; + import { Last15Minutes, Shortcuts } from '../common/data'; + import JSZip from 'jszip'; + + const { proxy } = getCurrentInstance(); + const route = useRoute(); + const $loadingCreate = getCurrentInstance().appContext.config.globalProperties.$loadingCreate; + const $loadingClose = proxy.$loadingClose; + const yamlRef = ref(null); + const timeRange = ref([]); + const data = reactive({ + group: route.params.group, + tableData: [], + name: route.params.name, + indexRule: '', + }); + const yamlCode = ref(``); + const selectedSpans = ref({}); + + const getTraces = async (params) => { + $loadingCreate(); + const response = await queryTraces({ groups: [data.group], name: data.name, ...params }) + $loadingClose(); + if (response.error) { + data.tableData = []; + ElMessage({ + message: response.error.message, + type: 'error', + duration: 3000, + }); + return; + } + data.tableData = response.traces || []; + }; + + const getIndexRule = async () => { + if (!data.group) { + return; + } + try { + const response = await getindexRuleList(data.group); + if (response.status === 200 && response.data.indexRule && response.data.indexRule.length > 0) { + data.indexRule = response.data.indexRule[0].metadata; + } else { + data.indexRule = ''; + } + } catch (err) { + console.error('Failed to fetch indexRule:', err); + data.indexRule = ''; + ElMessage({ + message: 'Failed to fetch index rule: ' + err, + type: 'error', + duration: 3000, + }); + } + }; + + function searchTraces() { + yamlRef.value + .checkYaml(yamlCode.value) + .then(() => { + const json = yamlToJson(yamlCode.value).data; + getTraces(json); + }) + .catch((err) => { + ElMessage({ + dangerouslyUseHTMLString: true, + showClose: true, + message: `<div>${err.message}</div>`, + type: 'error', + duration: 5000, + }); + }); + } + + function changeTimeRange() { + const json = yamlToJson(yamlCode.value); + if (!json.data.timeRange) { + json.data.timeRange = { + begin: '', + end: '', + }; + } + json.data.timeRange.begin = timeRange.value[0] ?? null; + json.data.timeRange.end = timeRange.value[1] ?? null; + yamlCode.value = jsonToYaml(json.data).data; + } + + async function initTraceData() { + if (!(data.group && data.name)) { + return; + } + await getIndexRule(); + if (!data.indexRule) { + return; + } + timeRange.value = [new Date(new Date().getTime() - Last15Minutes), new Date()]; + const range = jsonToYaml({ + timeRange: { + begin: timeRange.value[0], + end: timeRange.value[1], + }, + }).data; + yamlCode.value = `${range}groups: + - ${data.group} +name: ${data.name} +offset: 0 +limit: 10 +tagProjection: ["trace_id", "service_id"] +orderBy: + indexRuleName: ${data.indexRule.name} + sort: SORT_DESC`; + + getTraces(yamlToJson(yamlCode.value).data); + } + + function handleSelectionChange(traceIndex, selection) { + selectedSpans.value[traceIndex] = selection; + } + + async function downloadMultipleSpans(traceIndex) { + const selection = selectedSpans.value[traceIndex]; + if (!selection || selection.length === 0) { + ElMessage({ + message: 'Please select at least one span to download', + type: 'warning', + duration: 3000, + }); + return; + } + + try { + const zip = new JSZip(); + const trace = data.tableData[traceIndex]; + const timestamp = Date.now(); + let successCount = 0; + + // Add each span to the ZIP file + for (const span of selection) { + if (span && span.span) { + const base64Data = span.span; + const binaryString = atob(base64Data); + const bytes = new Uint8Array(binaryString.length); + for (let i = 0; i < binaryString.length; i++) { + bytes[i] = binaryString.charCodeAt(i); + } + // Find the original index of the span in the trace + const spanIndex = trace.spans.indexOf(span); + // Add the binary data to the ZIP file + zip.file(`span-${spanIndex + 1}.bin`, bytes); + successCount++; + } + }; + + if (successCount === 0) { + ElMessage({ + message: 'No valid spans to download', + type: 'warning', + duration: 3000, + }); + return; + } + + // Generate the ZIP file + const zipBlob = await zip.generateAsync({ type: 'blob' }); + + // Download the ZIP file + const url = URL.createObjectURL(zipBlob); + const link = document.createElement('a'); + link.href = url; + link.download = `trace-${traceIndex + 1}-spans-${timestamp}.zip`; + document.body.appendChild(link); + link.click(); + document.body.removeChild(link); + URL.revokeObjectURL(url); + + ElMessage({ + message: `Successfully downloaded ${successCount} span(s) as compressed file`, + type: 'success', + duration: 2000, + }); + } catch (err) { + ElMessage({ + message: 'Failed to download spans: ' + err.message, + type: 'error', + duration: 3000, + }); + } + } + + const getTagValue = (data) => { + if (!data.value) { + return ''; + } + return JSON.stringify(data.value); + }; Review Comment: This falsy check will hide valid values like 0, false, or empty string. Use a nullish check instead (e.g., data.value === null || data.value === undefined) so these values render. ########## ui/src/components/Trace/TraceRead.vue: ########## @@ -0,0 +1,342 @@ +<!-- + ~ Licensed to 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. Apache Software Foundation (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. +--> + +<script setup> + import { queryTraces, getindexRuleList } from '@/api/index'; + import { getCurrentInstance } from '@vue/runtime-core'; Review Comment: Import getCurrentInstance from 'vue' instead of '@vue/runtime-core'; the latter is an internal package and not part of the public API. ```suggestion import { getCurrentInstance } from 'vue'; ``` -- 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]
