Copilot commented on code in PR #1414: URL: https://github.com/apache/dubbo-admin/pull/1414#discussion_r2797274187
########## pkg/console/model/graph.go: ########## @@ -0,0 +1,62 @@ +/* + * 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. + */ + +package model + +import ( + meshresource "github.com/apache/dubbo-admin/pkg/core/resource/apis/mesh/v1alpha1" +) + +// GraphNode represents a node in the graph for AntV G6 +type GraphNode struct { + ID string `json:"id"` + Label string `json:"label"` + Data interface{} `json:"data,omitempty"` // Additional data for the node +} + +// GraphEdge represents an edge in the graph for AntV G6 +type GraphEdge struct { + Source string `json:"source"` + Target string `json:"target"` + Data map[string]interface{} `json:"data,omitempty"` // Additional data for the edge +} + +// GraphData represents the complete graph structure for AntV G6 +type GraphData struct { + Nodes []GraphNode `json:"nodes"` + Edges []GraphEdge `json:"edges"` +} + +// CrossNode represents a node in the cross-linked list structure +type CrossNode struct { + Instance *meshresource.InstanceResource + Next *CrossNode // pointer to next node in the same row + Down *CrossNode // pointer to next node in the same column +} + +// CrossLinkedListGraph represents the cross-linked list structure as a directed graph +type CrossLinkedListGraph struct { + Head *CrossNode + Rows int // number of rows + Cols int // number of columns +} Review Comment: `CrossNode` / `CrossLinkedListGraph` appear unused by the new endpoint (which returns `GraphData`), but they add API surface and force an extra import. If the graph is meant to be nodes/edges for G6, consider removing these types (and the `meshresource` import) or moving them behind an actual use to avoid confusion/maintenance burden. ########## pkg/console/service/service.go: ########## @@ -522,3 +522,174 @@ func isArgumentRoute(condition string) bool { } return false } + +// SearchServiceAsCrossLinkedList builds a service dependency graph. +// If serviceName is provided, it finds applications that consume or provide that specific service. +// If serviceName is empty, it returns all service dependencies in the mesh. +// It constructs nodes (applications) and edges (dependencies) for graph visualization. +func SearchServiceAsCrossLinkedList(ctx consolectx.Context, req *model.ServiceGraphReq) (*model.GraphData, error) { + // Build indexes conditionally based on whether serviceName is provided + consumerIndexes := map[string]string{ + index.ByMeshIndex: req.Mesh, + } + providerIndexes := map[string]string{ + index.ByMeshIndex: req.Mesh, + } + + // Only filter by serviceName if it's provided + if req.ServiceName != "" { + consumerIndexes[index.ByServiceConsumerServiceName] = req.ServiceName + providerIndexes[index.ByServiceProviderServiceName] = req.ServiceName + } + + // Use ListByIndexes instead of PageListByIndexes to get all related resources + // since we need complete dependency graph, not paginated results + consumers, err := manager.ListByIndexes[*meshresource.ServiceConsumerMetadataResource]( + ctx.ResourceManager(), + meshresource.ServiceConsumerMetadataKind, + consumerIndexes) + if err != nil { + logger.Errorf("get service consumer for mesh %s failed, cause: %v", req.Mesh, err) + return nil, bizerror.New(bizerror.InternalError, "get service consumer failed, please try again") + } + + providers, err := manager.ListByIndexes[*meshresource.ServiceProviderMetadataResource]( + ctx.ResourceManager(), + meshresource.ServiceProviderMetadataKind, + providerIndexes) + if err != nil { + logger.Errorf("get service provider for mesh %s failed, cause: %v", req.Mesh, err) + return nil, bizerror.New(bizerror.InternalError, "get service provider failed, please try again") + } + + // Collect all unique applications (both consumers and providers) + consumerApps := make(map[string]bool) + for _, consumer := range consumers { + if consumer.Spec != nil { + consumerApps[consumer.Spec.ConsumerAppName] = true + } + } + + providerApps := make(map[string]bool) + for _, provider := range providers { + if provider.Spec != nil { + providerApps[provider.Spec.ProviderAppName] = true + } + } + + allApps := make(map[string]bool) + for app := range consumerApps { + allApps[app] = true + } + for app := range providerApps { + allApps[app] = true + } + + nodes := make([]model.GraphNode, 0, len(allApps)) + edges := make([]model.GraphEdge, 0) + + // Build app to service instances mapping + // For providers: map providerAppName -> list of instances providing this service + // For consumers: map consumerAppName -> empty list (consumers don't provide instances) + appInstances := make(map[string][]*meshresource.InstanceResource) + + // Get all instances for the mesh first + allInstances, err := manager.ListByIndexes[*meshresource.InstanceResource]( + ctx.ResourceManager(), + meshresource.InstanceKind, + map[string]string{ + index.ByMeshIndex: req.Mesh, + }) + if err != nil { + logger.Errorf("get instances for mesh %s failed, cause: %v", req.Mesh, err) Review Comment: `ListByIndexes` for instances logs an error but continues returning a successful graph response, which can silently drop instance data and make the API appear healthy when it's not. Consider returning an error (consistent with the consumer/provider fetches) or explicitly reflecting the partial-failure in the response so clients can react appropriately. ```suggestion logger.Errorf("get instances for mesh %s failed, cause: %v", req.Mesh, err) return nil, err ``` ########## pkg/console/handler/service.go: ########## @@ -229,3 +229,22 @@ func ServiceConfigArgumentRoutePUT(ctx consolectx.Context) gin.HandlerFunc { c.JSON(http.StatusOK, model.NewSuccessResp(nil)) } } + +// GetServiceGraph returns the service graph in a cross-linked list structure for visualization Review Comment: This handler comment says it returns a cross-linked list structure, but it returns `model.GraphData` (nodes/edges). Please update the comment to avoid misleading API documentation and align it with the actual response type. ```suggestion // GetServiceGraph returns the service graph as graph data (nodes and edges) for visualization ``` ########## pkg/console/service/service.go: ########## @@ -522,3 +522,174 @@ func isArgumentRoute(condition string) bool { } return false } + +// SearchServiceAsCrossLinkedList builds a service dependency graph. +// If serviceName is provided, it finds applications that consume or provide that specific service. +// If serviceName is empty, it returns all service dependencies in the mesh. +// It constructs nodes (applications) and edges (dependencies) for graph visualization. +func SearchServiceAsCrossLinkedList(ctx consolectx.Context, req *model.ServiceGraphReq) (*model.GraphData, error) { + // Build indexes conditionally based on whether serviceName is provided + consumerIndexes := map[string]string{ + index.ByMeshIndex: req.Mesh, + } + providerIndexes := map[string]string{ + index.ByMeshIndex: req.Mesh, + } + + // Only filter by serviceName if it's provided + if req.ServiceName != "" { + consumerIndexes[index.ByServiceConsumerServiceName] = req.ServiceName + providerIndexes[index.ByServiceProviderServiceName] = req.ServiceName + } + + // Use ListByIndexes instead of PageListByIndexes to get all related resources + // since we need complete dependency graph, not paginated results + consumers, err := manager.ListByIndexes[*meshresource.ServiceConsumerMetadataResource]( + ctx.ResourceManager(), + meshresource.ServiceConsumerMetadataKind, + consumerIndexes) + if err != nil { + logger.Errorf("get service consumer for mesh %s failed, cause: %v", req.Mesh, err) + return nil, bizerror.New(bizerror.InternalError, "get service consumer failed, please try again") + } + + providers, err := manager.ListByIndexes[*meshresource.ServiceProviderMetadataResource]( + ctx.ResourceManager(), + meshresource.ServiceProviderMetadataKind, + providerIndexes) + if err != nil { + logger.Errorf("get service provider for mesh %s failed, cause: %v", req.Mesh, err) + return nil, bizerror.New(bizerror.InternalError, "get service provider failed, please try again") + } + + // Collect all unique applications (both consumers and providers) + consumerApps := make(map[string]bool) + for _, consumer := range consumers { + if consumer.Spec != nil { + consumerApps[consumer.Spec.ConsumerAppName] = true + } + } + + providerApps := make(map[string]bool) + for _, provider := range providers { + if provider.Spec != nil { + providerApps[provider.Spec.ProviderAppName] = true + } + } + + allApps := make(map[string]bool) + for app := range consumerApps { + allApps[app] = true + } + for app := range providerApps { + allApps[app] = true + } + + nodes := make([]model.GraphNode, 0, len(allApps)) + edges := make([]model.GraphEdge, 0) + + // Build app to service instances mapping + // For providers: map providerAppName -> list of instances providing this service + // For consumers: map consumerAppName -> empty list (consumers don't provide instances) + appInstances := make(map[string][]*meshresource.InstanceResource) + + // Get all instances for the mesh first + allInstances, err := manager.ListByIndexes[*meshresource.InstanceResource]( + ctx.ResourceManager(), + meshresource.InstanceKind, + map[string]string{ + index.ByMeshIndex: req.Mesh, + }) + if err != nil { + logger.Errorf("get instances for mesh %s failed, cause: %v", req.Mesh, err) + } + + // Build app -> instances mapping + for _, instance := range allInstances { + if instance.Spec != nil && instance.Spec.AppName != "" { + appInstances[instance.Spec.AppName] = append(appInstances[instance.Spec.AppName], instance) + } + } + + // Build nodes for each app + // Provider nodes: data contains instances providing this service + // Consumer nodes: data is nil (consumers don't provide instances) + for appName := range allApps { + var instanceData interface{} + + instances := make([]interface{}, 0) + if appInsts, ok := appInstances[appName]; ok { + for _, instance := range appInsts { + instances = append(instances, toInstanceData(instance)) + } + } + instanceData = instances + + nodes = append(nodes, model.GraphNode{ + ID: appName, + Label: appName, + Data: instanceData, + }) + } + edgeKeyMap := make(map[string]struct{}) + + // Build edges between consumers and providers + // Only create edges between apps that actually have the service relationship + for _, consumer := range consumers { + if consumer.Spec != nil { + for _, provider := range providers { + if provider.Spec != nil { + // This is where you should check if the provider's service name and the consumer's service name match. + if consumer.Spec.ServiceName != provider.Spec.ServiceName { + continue + } + // If there are two instances, such as p1->c1 and p2->c1, two edges will be generated, which need to be merged. + // Merging logic: Only one edge is kept for identical source and target. + // However, using a loop would result in three levels of nesting. Therefore, an auxiliary map is created for efficient filtering. + edgeKey := consumer.Spec.ConsumerAppName + "->" + provider.Spec.ProviderAppName + if _, exists := edgeKeyMap[edgeKey]; exists { + continue + } + edgeKeyMap[edgeKey] = struct{}{} + edges = append(edges, model.GraphEdge{ + Source: consumer.Spec.ConsumerAppName, + Target: provider.Spec.ProviderAppName, + Data: map[string]interface{}{ + "type": "dependency", + "serviceName": req.ServiceName, + "consumerApp": consumer.Spec.ConsumerAppName, + "providerApp": provider.Spec.ProviderAppName, + }, + }) + } else { + logger.Warnf("provider spec is nil for provider resource: %s", provider.Name) + } + } + } else { + logger.Warnf("consumer spec is nil for consumer resource: %s", consumer.Name) + } Review Comment: Building edges uses a nested consumers×providers loop, which is O(C*P) and can become very expensive when `req.ServiceName` is empty (lists include all metadata in the mesh) or when there are many instances per service. A more scalable approach is to pre-index providers by `ServiceName` (and possibly by `ProviderAppName`) and then only iterate over matching providers for each consumer. ```suggestion // Build an index of providers by service name to avoid O(C*P) nested loops. providersByService := make(map[string][]int) for pIdx, provider := range providers { if provider.Spec == nil { logger.Warnf("provider spec is nil for provider resource: %s", provider.Name) continue } serviceName := provider.Spec.ServiceName providersByService[serviceName] = append(providersByService[serviceName], pIdx) } // Build edges between consumers and providers // Only create edges between apps that actually have the service relationship for _, consumer := range consumers { if consumer.Spec == nil { logger.Warnf("consumer spec is nil for consumer resource: %s", consumer.Name) continue } serviceName := consumer.Spec.ServiceName providerIdxs := providersByService[serviceName] for _, pIdx := range providerIdxs { provider := providers[pIdx] // provider.Spec is guaranteed non-nil here because we only indexed non-nil specs. // If there are two instances, such as p1->c1 and p2->c1, two edges will be generated, which need to be merged. // Merging logic: Only one edge is kept for identical source and target. // However, using a loop would result in three levels of nesting. Therefore, an auxiliary map is created for efficient filtering. edgeKey := consumer.Spec.ConsumerAppName + "->" + provider.Spec.ProviderAppName if _, exists := edgeKeyMap[edgeKey]; exists { continue } edgeKeyMap[edgeKey] = struct{}{} edges = append(edges, model.GraphEdge{ Source: consumer.Spec.ConsumerAppName, Target: provider.Spec.ProviderAppName, Data: map[string]interface{}{ "type": "dependency", "serviceName": req.ServiceName, "consumerApp": consumer.Spec.ConsumerAppName, "providerApp": provider.Spec.ProviderAppName, }, }) } ``` ########## pkg/console/service/service.go: ########## @@ -522,3 +522,174 @@ func isArgumentRoute(condition string) bool { } return false } + +// SearchServiceAsCrossLinkedList builds a service dependency graph. +// If serviceName is provided, it finds applications that consume or provide that specific service. +// If serviceName is empty, it returns all service dependencies in the mesh. +// It constructs nodes (applications) and edges (dependencies) for graph visualization. +func SearchServiceAsCrossLinkedList(ctx consolectx.Context, req *model.ServiceGraphReq) (*model.GraphData, error) { Review Comment: The function name and comments refer to a “cross-linked list structure”, but the implementation returns `*model.GraphData` (nodes/edges) and doesn't use the cross-linked list types. Consider renaming this function to reflect what it actually returns (e.g., ServiceGraph/DependencyGraph) and updating the doc comment to match the JSON shape. ########## pkg/console/service/service.go: ########## @@ -522,3 +522,174 @@ func isArgumentRoute(condition string) bool { } return false } + +// SearchServiceAsCrossLinkedList builds a service dependency graph. +// If serviceName is provided, it finds applications that consume or provide that specific service. +// If serviceName is empty, it returns all service dependencies in the mesh. +// It constructs nodes (applications) and edges (dependencies) for graph visualization. +func SearchServiceAsCrossLinkedList(ctx consolectx.Context, req *model.ServiceGraphReq) (*model.GraphData, error) { + // Build indexes conditionally based on whether serviceName is provided + consumerIndexes := map[string]string{ + index.ByMeshIndex: req.Mesh, + } + providerIndexes := map[string]string{ + index.ByMeshIndex: req.Mesh, + } + + // Only filter by serviceName if it's provided + if req.ServiceName != "" { + consumerIndexes[index.ByServiceConsumerServiceName] = req.ServiceName + providerIndexes[index.ByServiceProviderServiceName] = req.ServiceName + } + + // Use ListByIndexes instead of PageListByIndexes to get all related resources + // since we need complete dependency graph, not paginated results + consumers, err := manager.ListByIndexes[*meshresource.ServiceConsumerMetadataResource]( + ctx.ResourceManager(), + meshresource.ServiceConsumerMetadataKind, + consumerIndexes) + if err != nil { + logger.Errorf("get service consumer for mesh %s failed, cause: %v", req.Mesh, err) + return nil, bizerror.New(bizerror.InternalError, "get service consumer failed, please try again") + } + + providers, err := manager.ListByIndexes[*meshresource.ServiceProviderMetadataResource]( + ctx.ResourceManager(), + meshresource.ServiceProviderMetadataKind, + providerIndexes) + if err != nil { + logger.Errorf("get service provider for mesh %s failed, cause: %v", req.Mesh, err) + return nil, bizerror.New(bizerror.InternalError, "get service provider failed, please try again") + } + + // Collect all unique applications (both consumers and providers) + consumerApps := make(map[string]bool) + for _, consumer := range consumers { + if consumer.Spec != nil { + consumerApps[consumer.Spec.ConsumerAppName] = true + } + } + + providerApps := make(map[string]bool) + for _, provider := range providers { + if provider.Spec != nil { + providerApps[provider.Spec.ProviderAppName] = true + } + } + + allApps := make(map[string]bool) + for app := range consumerApps { + allApps[app] = true + } + for app := range providerApps { + allApps[app] = true + } + + nodes := make([]model.GraphNode, 0, len(allApps)) + edges := make([]model.GraphEdge, 0) + + // Build app to service instances mapping + // For providers: map providerAppName -> list of instances providing this service + // For consumers: map consumerAppName -> empty list (consumers don't provide instances) + appInstances := make(map[string][]*meshresource.InstanceResource) + + // Get all instances for the mesh first + allInstances, err := manager.ListByIndexes[*meshresource.InstanceResource]( + ctx.ResourceManager(), + meshresource.InstanceKind, + map[string]string{ + index.ByMeshIndex: req.Mesh, + }) + if err != nil { + logger.Errorf("get instances for mesh %s failed, cause: %v", req.Mesh, err) + } + + // Build app -> instances mapping + for _, instance := range allInstances { + if instance.Spec != nil && instance.Spec.AppName != "" { + appInstances[instance.Spec.AppName] = append(appInstances[instance.Spec.AppName], instance) + } + } + + // Build nodes for each app + // Provider nodes: data contains instances providing this service + // Consumer nodes: data is nil (consumers don't provide instances) + for appName := range allApps { + var instanceData interface{} + + instances := make([]interface{}, 0) + if appInsts, ok := appInstances[appName]; ok { + for _, instance := range appInsts { + instances = append(instances, toInstanceData(instance)) + } + } + instanceData = instances + + nodes = append(nodes, model.GraphNode{ + ID: appName, + Label: appName, + Data: instanceData, + }) Review Comment: The comment above this loop says consumer nodes should have `data` = nil, but the code always assigns `instances` (an empty slice when none exist). Because `Data` is an `interface{}` with `omitempty`, this will still serialize as `"data": []` for apps with no instances. Decide on the intended API contract and either set `Data` to nil when there are no instances or update the comment/API docs accordingly. ########## pkg/console/service/service.go: ########## @@ -522,3 +522,174 @@ func isArgumentRoute(condition string) bool { } return false } + +// SearchServiceAsCrossLinkedList builds a service dependency graph. +// If serviceName is provided, it finds applications that consume or provide that specific service. +// If serviceName is empty, it returns all service dependencies in the mesh. +// It constructs nodes (applications) and edges (dependencies) for graph visualization. +func SearchServiceAsCrossLinkedList(ctx consolectx.Context, req *model.ServiceGraphReq) (*model.GraphData, error) { + // Build indexes conditionally based on whether serviceName is provided + consumerIndexes := map[string]string{ + index.ByMeshIndex: req.Mesh, + } + providerIndexes := map[string]string{ + index.ByMeshIndex: req.Mesh, + } + + // Only filter by serviceName if it's provided + if req.ServiceName != "" { + consumerIndexes[index.ByServiceConsumerServiceName] = req.ServiceName + providerIndexes[index.ByServiceProviderServiceName] = req.ServiceName + } + + // Use ListByIndexes instead of PageListByIndexes to get all related resources + // since we need complete dependency graph, not paginated results + consumers, err := manager.ListByIndexes[*meshresource.ServiceConsumerMetadataResource]( + ctx.ResourceManager(), + meshresource.ServiceConsumerMetadataKind, + consumerIndexes) + if err != nil { + logger.Errorf("get service consumer for mesh %s failed, cause: %v", req.Mesh, err) + return nil, bizerror.New(bizerror.InternalError, "get service consumer failed, please try again") + } + + providers, err := manager.ListByIndexes[*meshresource.ServiceProviderMetadataResource]( + ctx.ResourceManager(), + meshresource.ServiceProviderMetadataKind, + providerIndexes) + if err != nil { + logger.Errorf("get service provider for mesh %s failed, cause: %v", req.Mesh, err) + return nil, bizerror.New(bizerror.InternalError, "get service provider failed, please try again") + } + + // Collect all unique applications (both consumers and providers) + consumerApps := make(map[string]bool) + for _, consumer := range consumers { + if consumer.Spec != nil { + consumerApps[consumer.Spec.ConsumerAppName] = true + } + } + + providerApps := make(map[string]bool) + for _, provider := range providers { + if provider.Spec != nil { + providerApps[provider.Spec.ProviderAppName] = true + } + } + + allApps := make(map[string]bool) + for app := range consumerApps { + allApps[app] = true + } + for app := range providerApps { + allApps[app] = true + } + + nodes := make([]model.GraphNode, 0, len(allApps)) + edges := make([]model.GraphEdge, 0) + + // Build app to service instances mapping + // For providers: map providerAppName -> list of instances providing this service + // For consumers: map consumerAppName -> empty list (consumers don't provide instances) + appInstances := make(map[string][]*meshresource.InstanceResource) + + // Get all instances for the mesh first + allInstances, err := manager.ListByIndexes[*meshresource.InstanceResource]( + ctx.ResourceManager(), + meshresource.InstanceKind, + map[string]string{ + index.ByMeshIndex: req.Mesh, + }) + if err != nil { + logger.Errorf("get instances for mesh %s failed, cause: %v", req.Mesh, err) + } + + // Build app -> instances mapping + for _, instance := range allInstances { + if instance.Spec != nil && instance.Spec.AppName != "" { + appInstances[instance.Spec.AppName] = append(appInstances[instance.Spec.AppName], instance) + } + } + + // Build nodes for each app + // Provider nodes: data contains instances providing this service + // Consumer nodes: data is nil (consumers don't provide instances) + for appName := range allApps { + var instanceData interface{} + + instances := make([]interface{}, 0) + if appInsts, ok := appInstances[appName]; ok { + for _, instance := range appInsts { + instances = append(instances, toInstanceData(instance)) + } + } + instanceData = instances + + nodes = append(nodes, model.GraphNode{ + ID: appName, + Label: appName, + Data: instanceData, + }) + } + edgeKeyMap := make(map[string]struct{}) + + // Build edges between consumers and providers + // Only create edges between apps that actually have the service relationship + for _, consumer := range consumers { + if consumer.Spec != nil { + for _, provider := range providers { + if provider.Spec != nil { + // This is where you should check if the provider's service name and the consumer's service name match. Review Comment: This inline comment reads like a leftover TODO/instruction and doesn't add value in production code. Please remove it or replace it with a concise explanation of the actual matching rule being implemented (service-name equality). ```suggestion // Only create edges when the consumer and provider reference the same service name. ``` -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
