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.

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

Jump to bottom

Fixed the issue of querying irrelevant endpoint information on the endpoint topology page. #442

Merged
merged 1 commit into from
Dec 21, 2024
Merged

Fixed the issue of querying irrelevant endpoint information on the endpoint topology page. #442

merged 1 commit into from
Dec 21, 2024

Conversation

Super-Lu
Copy link
Contributor

@Super-Lu Super-Lu commented Dec 19, 2024 •
edited by Fine0830
Loading

Fix the issue where when initiating a query on the endpoint topology page, if the query parameters contain the ID of the virtual endpoint User, all link information will be retrieved.
Fixes apache/skywalking#12880

wu-sheng
wu-sheng reviewed Dec 19, 2024
View reviewed changes
src/store/modules/topology.ts Outdated
@@ -338,14 +338,18 @@ export const topologyStore = defineStore({
}
const res = await this.getEndpointTopology(endpointIds);
if (depth > 1) {
const ids = res.nodes.map((item: Node) => item.id).filter((d: string) => !endpointIds.includes(d));
const ids = res.nodes
.filter((item: Node) => item.isReal)
Copy link
Member

Choose a reason for hiding this comment

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

A database could be not real. I think we should only filter User out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A database could be not real. I think we should only filter User out.

Is it filtered like this?
.filter((item: Node) => item.name != ' User')

Copy link
Member

Choose a reason for hiding this comment

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

Conjectured nodes and the User node are not real.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I think that is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I think that is.

Okay, I'll make the changes now

@wu-sheng wu-sheng added the bug Something isn't working label Dec 19, 2024
@wu-sheng wu-sheng added this to the 10.2.0 milestone Dec 19, 2024
@wu-sheng wu-sheng requested a review from Fine0830 December 19, 2024 14:21
wu-sheng
wu-sheng reviewed Dec 19, 2024
View reviewed changes
src/store/modules/topology.ts Outdated
@@ -338,14 +338,19 @@ export const topologyStore = defineStore({
}
const res = await this.getEndpointTopology(endpointIds);
if (depth > 1) {
const ids = res.nodes.map((item: Node) => item.id).filter((d: string) => !endpointIds.includes(d));
const nodeName = "User";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const nodeName = "User";
const userNodeName = "User";

Copy link
Member

Choose a reason for hiding this comment

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

@Super-Lu You should not accept my codes directly, this is a variable renaming, the other codes are not aligned.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Super-Lu You should not accept my codes directly, this is a variable renaming, the other codes are not aligned.

Yes, thank you for the reminder.

@wu-sheng
Copy link
Member

Let's wait for @Fine0830 review later.

@Super-Lu
Copy link
Contributor Author

Let's wait for @Fine0830 review later.

OK

Fine0830
Fine0830 requested changes Dec 20, 2024
View reviewed changes
Copy link
Member

@Fine0830 Fine0830 left a comment

Choose a reason for hiding this comment

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

Thanks @Super-Lu . There is a nit in it. Please fix.

src/store/modules/topology.ts Outdated
Comment on lines 342 to 345
const ids = res.nodes
.filter((item: Node) => item.name != userNodeName)
.map((item: Node) => item.id)
.filter((d: string) => !endpointIds.includes(d));
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const ids = res.nodes
.filter((item: Node) => item.name != userNodeName)
.map((item: Node) => item.id)
.filter((d: string) => !endpointIds.includes(d));
const ids = res.nodes.filter((d: Node) => !endpointIds.includes(d.id) && d.name !== userNodeName).map((item: Node) => item.id);

src/store/modules/topology.ts Outdated
if (!ids.length) {
this.setTopology(res);
return;
}
const json = await this.getEndpointTopology(ids);
if (depth > 2) {
const pods = json.nodes
.filter((item: Node) => item.name != userNodeName)
Copy link
Member

Choose a reason for hiding this comment

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

Need to change as above

src/store/modules/topology.ts Outdated
@@ -357,6 +362,7 @@ export const topologyStore = defineStore({
const topo = await this.getEndpointTopology(pods);
if (depth > 3) {
const endpoints = topo.nodes
.filter((item: Node) => item.name != userNodeName)
Copy link
Member

Choose a reason for hiding this comment

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

Need to change as above

src/store/modules/topology.ts Outdated
@@ -368,6 +374,7 @@ export const topologyStore = defineStore({
const data = await this.getEndpointTopology(endpoints);
if (depth > 4) {
const nodeIds = data.nodes
.filter((item: Node) => item.name != userNodeName)
Copy link
Member

Choose a reason for hiding this comment

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

Need to change as above

@Super-Lu
Copy link
Contributor Author

Thanks @Super-Lu . There is a nit in it. Please fix.
Okay

@wu-sheng wu-sheng requested a review from Fine0830 December 20, 2024 13:50
@Super-Lu
Copy link
Contributor Author

Let's wait for @Fine0830 review later.

Excuse me, why does the submitted code format automatically change.
image

@wu-sheng
Copy link
Member

Is it code changed or code style changed? The git hook should only change/format code styles.

@Super-Lu
Copy link
Contributor Author

Is it code changed or code style changed? The git hook should only change/format code styles.

The code style has been modified, with one line becoming multiple lines

@wu-sheng
Copy link
Member

I think prettier did that.

@Super-Lu
Copy link
Contributor Author

I think prettier did that.

Sorry, I didn't understand what it meant. Are you saying this is normal？

@Super-Lu
Fix the issue where when initiating a query on the endpoint topology …
9df8170 
…page, if the query parameters contain the ID of the virtual endpoint User, all link information will be retrieved.
@wu-sheng
Copy link
Member

prettier is a bundled tool of this repo, it will reformat codes when you do git push.
If code logic is unchanged, we're good.

@Super-Lu
Copy link
Contributor Author

prettier is a bundled tool of this repo, it will reformat codes when you do git push. If code logic is unchanged, we're good.

I understand now, thank you for your explanation.

@Fine0830 Fine0830 merged commit c33d6c4 into apache:main Dec 21, 2024
5 checks passed
@wu-sheng
Copy link
Member

@Super-Lu You need to sync the submodule in the main repo(/ui folder), and update the changes.md in the main repo too.

@Super-Lu
Copy link
Contributor Author

@Super-Lu You need to sync the submodule in the main repo(/ui folder), and update the changes.md in the main repo too.

Sure, I'll handle it as soon as possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wu-sheng wu-sheng wu-sheng left review comments

@Fine0830 Fine0830 Fine0830 approved these changes

Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
10.2.0
Development

Successfully merging this pull request may close these issues.

[Bug] The endpoint topology graph page queries a large amount of unrelated endpoint information.
3 participants
@Super-Lu @wu-sheng @Fine0830