Conversation
ae3f8eb to
5e17b0a
Compare
There was a problem hiding this comment.
Pull request overview
This PR implements mesh-based grouping for the counter system, allowing statistics to be tracked and queried per mesh rather than only globally. The changes refactor the counter infrastructure from using atomic operations to map-based group tracking, and add initialization logic to populate counters from existing store data on startup.
Changes:
- Refactored
CounterandDistributionCounterto support group-based (mesh) counting instead of global atomic counters - Added
CountByMeshandDistributionByMeshmethods to theCounterManagerinterface for mesh-specific queries - Implemented counter initialization from the resource store on component startup to ensure accurate counts
- Updated the overview API handler to support optional mesh query parameter for filtered statistics
- Added mesh index to resource queries in discovery subscribers for improved query performance
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/console/counter/counter.go | Refactored Counter and DistributionCounter from atomic operations to map-based group tracking |
| pkg/console/counter/manager.go | Added mesh-aware counting methods and updated event processing to track resources by mesh |
| pkg/console/counter/component.go | Added initialization logic to populate counters from existing store data on startup |
| pkg/console/handler/overview.go | Added support for optional mesh query parameter in cluster overview endpoint |
| pkg/core/discovery/subscriber/nacos_service.go | Added mesh index to resource queries for improved filtering |
| pkg/core/discovery/subscriber/instance.go | Added mesh index to instance resource queries for improved filtering |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func (cm *counterManager) CountByMesh(kind resmodel.ResourceKind, mesh string) int64 { | ||
| if mesh == "" { | ||
| mesh = "default" | ||
| } | ||
| if counter, exists := cm.simpleCounters[kind]; exists { | ||
| return counter.GetByGroup(mesh) | ||
| } | ||
| return 0 | ||
| } | ||
|
|
||
| func (cm *counterManager) DistributionByMesh(metric CounterType, mesh string) map[string]int64 { | ||
| if mesh == "" { | ||
| mesh = "default" | ||
| } | ||
| return result | ||
| counter, exists := cm.distributionByType[metric] | ||
| if !exists { | ||
| return map[string]int64{} | ||
| } | ||
| return counter.GetByGroup(mesh) |
There was a problem hiding this comment.
The new mesh-based counting functionality introduced in CountByMesh and DistributionByMesh methods lacks test coverage. Given the complexity of the grouping logic and the presence of tests elsewhere in the repository, tests should be added to verify correct behavior for single mesh, multiple meshes, and edge cases like empty mesh names.
pkg/console/counter/component.go
Outdated
| return fmt.Errorf("component %s does not implement store.Router", runtime.ResourceStore) | ||
| } | ||
|
|
||
| c.initializeCountsFromStore(storeRouter) |
There was a problem hiding this comment.
The error returned by initializeCountsFromStore is being ignored. If initialization fails, the counter manager will start with incorrect counts but the application will continue running. This should either return the error to fail fast, or log a warning if partial initialization is acceptable.
| c.initializeCountsFromStore(storeRouter) | |
| if err := c.initializeCountsFromStore(storeRouter); err != nil { | |
| fmt.Printf("warning: failed to initialize counter manager from store: %v\n", err) | |
| } |
| if oldKey == newKey { | ||
| return | ||
| } | ||
| oldMesh := extractMeshName(oldObj) | ||
| newMesh := extractMeshName(newObj) |
There was a problem hiding this comment.
The update method only updates counters when oldKey != newKey, but it doesn't handle the case where the mesh changes while the key stays the same. This means if a resource moves from one mesh to another without changing its protocol/release/discovery value, the counters won't be updated correctly. The condition should check if either the key or mesh has changed.
| if oldKey == newKey { | |
| return | |
| } | |
| oldMesh := extractMeshName(oldObj) | |
| newMesh := extractMeshName(newObj) | |
| oldMesh := extractMeshName(oldObj) | |
| newMesh := extractMeshName(newObj) | |
| if oldKey == newKey && oldMesh == newMesh { | |
| return | |
| } |
pkg/console/counter/component.go
Outdated
| for mesh, count := range meshCounts { | ||
| for i := int64(0); i < count; i++ { | ||
| counter.Increment(mesh) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if kind == meshresource.InstanceKind { | ||
| for mesh, distributions := range meshDistributions { | ||
| for key, count := range distributions { | ||
| if len(key) > 9 && key[:9] == "protocol:" { | ||
| if cfg := cm.getDistributionConfig(kind, ProtocolCounter); cfg != nil { | ||
| for i := int64(0); i < count; i++ { | ||
| cfg.counter.Increment(mesh, key[9:]) | ||
| } | ||
| } | ||
| } else if len(key) > 8 && key[:8] == "release:" { | ||
| if cfg := cm.getDistributionConfig(kind, ReleaseCounter); cfg != nil { | ||
| for i := int64(0); i < count; i++ { | ||
| cfg.counter.Increment(mesh, key[8:]) | ||
| } | ||
| } | ||
| } else if len(key) > 11 && key[:11] == "discovery:" { | ||
| if cfg := cm.getDistributionConfig(kind, DiscoveryCounter); cfg != nil { | ||
| for i := int64(0); i < count; i++ { | ||
| cfg.counter.Increment(mesh, key[11:]) |
There was a problem hiding this comment.
Using a loop to call Increment multiple times is inefficient. Consider adding a bulk increment method like IncrementBy(group string, count int64) to the Counter struct to improve performance during initialization.
| for mesh, count := range meshCounts { | |
| for i := int64(0); i < count; i++ { | |
| counter.Increment(mesh) | |
| } | |
| } | |
| } | |
| if kind == meshresource.InstanceKind { | |
| for mesh, distributions := range meshDistributions { | |
| for key, count := range distributions { | |
| if len(key) > 9 && key[:9] == "protocol:" { | |
| if cfg := cm.getDistributionConfig(kind, ProtocolCounter); cfg != nil { | |
| for i := int64(0); i < count; i++ { | |
| cfg.counter.Increment(mesh, key[9:]) | |
| } | |
| } | |
| } else if len(key) > 8 && key[:8] == "release:" { | |
| if cfg := cm.getDistributionConfig(kind, ReleaseCounter); cfg != nil { | |
| for i := int64(0); i < count; i++ { | |
| cfg.counter.Increment(mesh, key[8:]) | |
| } | |
| } | |
| } else if len(key) > 11 && key[:11] == "discovery:" { | |
| if cfg := cm.getDistributionConfig(kind, DiscoveryCounter); cfg != nil { | |
| for i := int64(0); i < count; i++ { | |
| cfg.counter.Increment(mesh, key[11:]) | |
| // Use bulk increment when supported by the underlying counter implementation. | |
| type bulkCounter interface { | |
| IncrementBy(group string, count int64) | |
| } | |
| if bc, ok := counter.(bulkCounter); ok { | |
| for mesh, count := range meshCounts { | |
| bc.IncrementBy(mesh, count) | |
| } | |
| } else { | |
| for mesh, count := range meshCounts { | |
| for i := int64(0); i < count; i++ { | |
| counter.Increment(mesh) | |
| } | |
| } | |
| } | |
| } | |
| if kind == meshresource.InstanceKind { | |
| // Use bulk increment when supported by the underlying distribution counter implementation. | |
| type distBulkCounter interface { | |
| IncrementBy(mesh, label string, count int64) | |
| } | |
| for mesh, distributions := range meshDistributions { | |
| for key, count := range distributions { | |
| if len(key) > 9 && key[:9] == "protocol:" { | |
| if cfg := cm.getDistributionConfig(kind, ProtocolCounter); cfg != nil { | |
| if bc, ok := cfg.counter.(distBulkCounter); ok { | |
| bc.IncrementBy(mesh, key[9:], count) | |
| } else { | |
| for i := int64(0); i < count; i++ { | |
| cfg.counter.Increment(mesh, key[9:]) | |
| } | |
| } | |
| } | |
| } else if len(key) > 8 && key[:8] == "release:" { | |
| if cfg := cm.getDistributionConfig(kind, ReleaseCounter); cfg != nil { | |
| if bc, ok := cfg.counter.(distBulkCounter); ok { | |
| bc.IncrementBy(mesh, key[8:], count) | |
| } else { | |
| for i := int64(0); i < count; i++ { | |
| cfg.counter.Increment(mesh, key[8:]) | |
| } | |
| } | |
| } | |
| } else if len(key) > 11 && key[:11] == "discovery:" { | |
| if cfg := cm.getDistributionConfig(kind, DiscoveryCounter); cfg != nil { | |
| if bc, ok := cfg.counter.(distBulkCounter); ok { | |
| bc.IncrementBy(mesh, key[11:], count) | |
| } else { | |
| for i := int64(0); i < count; i++ { | |
| cfg.counter.Increment(mesh, key[11:]) | |
| } |
pkg/console/counter/component.go
Outdated
| if len(key) > 9 && key[:9] == "protocol:" { | ||
| if cfg := cm.getDistributionConfig(kind, ProtocolCounter); cfg != nil { | ||
| for i := int64(0); i < count; i++ { | ||
| cfg.counter.Increment(mesh, key[9:]) | ||
| } | ||
| } | ||
| } else if len(key) > 8 && key[:8] == "release:" { | ||
| if cfg := cm.getDistributionConfig(kind, ReleaseCounter); cfg != nil { | ||
| for i := int64(0); i < count; i++ { | ||
| cfg.counter.Increment(mesh, key[8:]) | ||
| } | ||
| } | ||
| } else if len(key) > 11 && key[:11] == "discovery:" { | ||
| if cfg := cm.getDistributionConfig(kind, DiscoveryCounter); cfg != nil { | ||
| for i := int64(0); i < count; i++ { | ||
| cfg.counter.Increment(mesh, key[11:]) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Using magic numbers for string prefix lengths (9, 8, 11) makes the code fragile and error-prone. Consider using constants or the strings.HasPrefix function with explicit prefix strings to make the code more maintainable and less prone to errors if the prefixes change.
| func (c *Counter) Increment(group string) { | ||
| if group == "" { | ||
| group = "default" | ||
| } | ||
| c.mu.Lock() | ||
| defer c.mu.Unlock() | ||
| c.data[group]++ | ||
| } | ||
|
|
||
| func (c *Counter) Decrement(group string) { | ||
| if group == "" { | ||
| group = "default" | ||
| } | ||
| c.mu.Lock() | ||
| defer c.mu.Unlock() | ||
| if value, ok := c.data[group]; ok { | ||
| value-- | ||
| if value <= 0 { | ||
| delete(c.data, group) | ||
| } else { | ||
| c.data[group] = value | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The Counter and DistributionCounter refactoring from atomic operations to map-based group tracking introduces significant complexity but lacks test coverage. Tests should verify thread safety, correct increment/decrement behavior per group, proper cleanup when counts reach zero, and the GetByGroup functionality.
| func (c *managerComponent) initializeCountsFromStore(storeRouter store.Router) error { | ||
| if err := c.initializeResourceCount(storeRouter, meshresource.InstanceKind); err != nil { | ||
| return fmt.Errorf("failed to initialize instance count: %w", err) | ||
| } | ||
|
|
||
| if err := c.initializeResourceCount(storeRouter, meshresource.ApplicationKind); err != nil { | ||
| return fmt.Errorf("failed to initialize application count: %w", err) | ||
| } | ||
|
|
||
| if err := c.initializeResourceCount(storeRouter, meshresource.ServiceProviderMetadataKind); err != nil { | ||
| return fmt.Errorf("failed to initialize service provider metadata count: %w", err) | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| func (c *managerComponent) initializeResourceCount(storeRouter store.Router, kind resmodel.ResourceKind) error { | ||
| resourceStore, err := storeRouter.ResourceKindRoute(kind) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| allResources := resourceStore.List() | ||
|
|
||
| meshCounts := make(map[string]int64) | ||
| meshDistributions := make(map[string]map[string]int64) | ||
|
|
||
| for _, obj := range allResources { | ||
| resource, ok := obj.(resmodel.Resource) | ||
| if !ok { | ||
| continue | ||
| } | ||
|
|
||
| mesh := resource.ResourceMesh() | ||
| if mesh == "" { | ||
| mesh = "default" | ||
| } | ||
|
|
||
| meshCounts[mesh]++ | ||
|
|
||
| if kind == meshresource.InstanceKind { | ||
| instance, ok := resource.(*meshresource.InstanceResource) | ||
| if ok && instance.Spec != nil { | ||
| protocol := instance.Spec.GetProtocol() | ||
| if protocol != "" { | ||
| if meshDistributions[mesh] == nil { | ||
| meshDistributions[mesh] = make(map[string]int64) | ||
| } | ||
| meshDistributions[mesh]["protocol:"+protocol]++ | ||
| } | ||
|
|
||
| releaseVersion := instance.Spec.GetReleaseVersion() | ||
| if releaseVersion != "" { | ||
| if meshDistributions[mesh] == nil { | ||
| meshDistributions[mesh] = make(map[string]int64) | ||
| } | ||
| meshDistributions[mesh]["release:"+releaseVersion]++ | ||
| } | ||
|
|
||
| if meshDistributions[mesh] == nil { | ||
| meshDistributions[mesh] = make(map[string]int64) | ||
| } | ||
| meshDistributions[mesh]["discovery:"+mesh]++ | ||
| } | ||
| } | ||
| } | ||
|
|
||
| cm := c.manager.(*counterManager) | ||
|
|
||
| if counter, exists := cm.simpleCounters[kind]; exists { | ||
| for mesh, count := range meshCounts { | ||
| for i := int64(0); i < count; i++ { | ||
| counter.Increment(mesh) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if kind == meshresource.InstanceKind { | ||
| for mesh, distributions := range meshDistributions { | ||
| for key, count := range distributions { | ||
| if len(key) > 9 && key[:9] == "protocol:" { | ||
| if cfg := cm.getDistributionConfig(kind, ProtocolCounter); cfg != nil { | ||
| for i := int64(0); i < count; i++ { | ||
| cfg.counter.Increment(mesh, key[9:]) | ||
| } | ||
| } | ||
| } else if len(key) > 8 && key[:8] == "release:" { | ||
| if cfg := cm.getDistributionConfig(kind, ReleaseCounter); cfg != nil { | ||
| for i := int64(0); i < count; i++ { | ||
| cfg.counter.Increment(mesh, key[8:]) | ||
| } | ||
| } | ||
| } else if len(key) > 11 && key[:11] == "discovery:" { | ||
| if cfg := cm.getDistributionConfig(kind, DiscoveryCounter); cfg != nil { | ||
| for i := int64(0); i < count; i++ { | ||
| cfg.counter.Increment(mesh, key[11:]) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
The initializeCountsFromStore function, which is critical for ensuring correct counter initialization on startup, lacks test coverage. Tests should verify correct initialization from existing store data, proper handling of resources across multiple meshes, and correct distribution counter initialization for instances.
pkg/console/counter/component.go
Outdated
| if len(key) > 9 && key[:9] == "protocol:" { | ||
| if cfg := cm.getDistributionConfig(kind, ProtocolCounter); cfg != nil { | ||
| for i := int64(0); i < count; i++ { | ||
| cfg.counter.Increment(mesh, key[9:]) | ||
| } | ||
| } | ||
| } else if len(key) > 8 && key[:8] == "release:" { | ||
| if cfg := cm.getDistributionConfig(kind, ReleaseCounter); cfg != nil { | ||
| for i := int64(0); i < count; i++ { | ||
| cfg.counter.Increment(mesh, key[8:]) | ||
| } | ||
| } | ||
| } else if len(key) > 11 && key[:11] == "discovery:" { | ||
| if cfg := cm.getDistributionConfig(kind, DiscoveryCounter); cfg != nil { | ||
| for i := int64(0); i < count; i++ { | ||
| cfg.counter.Increment(mesh, key[11:]) | ||
| } | ||
| } |
There was a problem hiding this comment.
Using loops to call Increment multiple times is inefficient. Consider adding a bulk increment method to the DistributionCounter struct to improve performance during initialization, especially when dealing with large numbers of resources.
pkg/console/counter/component.go
Outdated
| protocol := instance.Spec.GetProtocol() | ||
| if protocol != "" { | ||
| if meshDistributions[mesh] == nil { | ||
| meshDistributions[mesh] = make(map[string]int64) | ||
| } | ||
| meshDistributions[mesh]["protocol:"+protocol]++ | ||
| } | ||
|
|
||
| releaseVersion := instance.Spec.GetReleaseVersion() | ||
| if releaseVersion != "" { | ||
| if meshDistributions[mesh] == nil { | ||
| meshDistributions[mesh] = make(map[string]int64) | ||
| } | ||
| meshDistributions[mesh]["release:"+releaseVersion]++ | ||
| } | ||
|
|
||
| if meshDistributions[mesh] == nil { | ||
| meshDistributions[mesh] = make(map[string]int64) | ||
| } |
There was a problem hiding this comment.
The code repeatedly checks and initializes meshDistributions[mesh] on lines 137, 145, and 151. This logic should be refactored to initialize the map once at the beginning of the instance processing block to improve code clarity and avoid redundant checks.
| protocol := instance.Spec.GetProtocol() | |
| if protocol != "" { | |
| if meshDistributions[mesh] == nil { | |
| meshDistributions[mesh] = make(map[string]int64) | |
| } | |
| meshDistributions[mesh]["protocol:"+protocol]++ | |
| } | |
| releaseVersion := instance.Spec.GetReleaseVersion() | |
| if releaseVersion != "" { | |
| if meshDistributions[mesh] == nil { | |
| meshDistributions[mesh] = make(map[string]int64) | |
| } | |
| meshDistributions[mesh]["release:"+releaseVersion]++ | |
| } | |
| if meshDistributions[mesh] == nil { | |
| meshDistributions[mesh] = make(map[string]int64) | |
| } | |
| if meshDistributions[mesh] == nil { | |
| meshDistributions[mesh] = make(map[string]int64) | |
| } | |
| protocol := instance.Spec.GetProtocol() | |
| if protocol != "" { | |
| meshDistributions[mesh]["protocol:"+protocol]++ | |
| } | |
| releaseVersion := instance.Spec.GetReleaseVersion() | |
| if releaseVersion != "" { | |
| meshDistributions[mesh]["release:"+releaseVersion]++ | |
| } |
robocanic
left a comment
There was a problem hiding this comment.
Great Work! I've left some comments, if you have any problems, discuss it with me.
pkg/console/counter/component.go
Outdated
| if kind == meshresource.InstanceKind { | ||
| for mesh, distributions := range meshDistributions { | ||
| for key, count := range distributions { | ||
| if len(key) > 9 && key[:9] == "protocol:" { |
There was a problem hiding this comment.
Suggestion:这里的逻辑是不是能直接放到上面的for-loop里面,将对应的子类型increment,不需要再来一个map中转
9efa888 to
17329aa
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
pkg/console/counter/counter.go:168
- The newly added GetByGroup method lacks documentation. Public methods should have comments describing their purpose, parameters, and return values.
func (c *DistributionCounter) GetByGroup(group string) map[string]int64 {
if group == "" {
group = "default"
}
c.mu.RLock()
defer c.mu.RUnlock()
groupData, exists := c.data[group]
if !exists {
return map[string]int64{}
}
result := make(map[string]int64, len(groupData))
for k, v := range groupData {
result[k] = v
}
return result
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| counter.Decrement() | ||
| counter.Decrement(mesh) | ||
| logger.Debugf("CounterManager: Decrement %s for mesh=%s, current count=%d", counter.name, mesh, counter.GetByGroup(mesh)) | ||
| case cache.Updated: |
There was a problem hiding this comment.
Simple counters do not handle mesh changes during update events. If a resource's mesh changes from "meshA" to "meshB" during an update, the counter for "meshA" should decrement and "meshB" should increment, but currently the update case is a no-op. This can lead to incorrect counts when resources move between meshes.
| case cache.Updated: | |
| case cache.Updated: | |
| oldMesh := extractMeshName(event.OldObj()) | |
| newMesh := extractMeshName(event.NewObj()) | |
| if oldMesh != newMesh { | |
| counter.Decrement(oldMesh) | |
| logger.Debugf("CounterManager: Decrement %s for mesh=%s (Updated), current count=%d", counter.name, oldMesh, counter.GetByGroup(oldMesh)) | |
| counter.Increment(newMesh) | |
| logger.Debugf("CounterManager: Increment %s for mesh=%s (Updated), current count=%d", counter.name, newMesh, counter.GetByGroup(newMesh)) | |
| } |
| Distribution(metric CounterType) map[string]int64 | ||
| CountByMesh(kind resmodel.ResourceKind, mesh string) int64 |
There was a problem hiding this comment.
The newly added CountByMesh and DistributionByMesh methods lack documentation comments. Public interface methods should have comments describing their purpose, parameters, and return values to help API consumers understand how to use them.
| Distribution(metric CounterType) map[string]int64 | |
| CountByMesh(kind resmodel.ResourceKind, mesh string) int64 | |
| Distribution(metric CounterType) map[string]int64 | |
| // CountByMesh returns the number of resources of the given kind that belong to the specified mesh. | |
| // The mesh parameter identifies the mesh to filter on. | |
| CountByMesh(kind resmodel.ResourceKind, mesh string) int64 | |
| // DistributionByMesh returns a distribution of counts for the given metric limited to the specified mesh. | |
| // The returned map keys are metric values (for example, protocol or release) and the values are their counts. |
| func (c *Counter) GetByGroup(group string) int64 { | ||
| if group == "" { | ||
| group = "default" | ||
| } | ||
| c.mu.RLock() | ||
| defer c.mu.RUnlock() | ||
| return c.data[group] | ||
| } |
There was a problem hiding this comment.
The newly added GetByGroup method lacks documentation. Public methods should have comments describing their purpose, parameters, and return values.
robocanic
left a comment
There was a problem hiding this comment.
LGTM. Please merge the develop branch and I will merge into it.
|
ok |



implement counter by key