feat: 支持 metrics #310
feat: 支持 metrics #310
Conversation
WalkthroughThe recent updates across multiple Go files introduce enhanced metrics tracking capabilities using Prometheus, alongside some structural changes to data handling and function parameters. These modifications aim to improve monitoring, version tracking, and event handling in the application's configuration and notification components. Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
|
感谢您提出Pull Request，我会尽快Review。我会在1-2日内进行查看或者回复，如果遇到节假日可能会处理较慢，敬请谅解。 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
go.modis excluded by!**/*.mod
Files selected for processing (7)
- client.go (1 hunks)
- component/metrics/metrics.go (1 hunks)
- component/notify/componet_notify.go (4 hunks)
- component/remote/async.go (2 hunks)
- extension/metrics.go (1 hunks)
- storage/change_event.go (4 hunks)
- storage/repository.go (3 hunks)
Additional comments not posted (7)
component/metrics/metrics.go (2)
8-13: Ensure that the labels used inNamespaceVersionGaugeare accurate and meaningful for the intended metrics.Verification successful
The verification process has confirmed that the labels used in
NamespaceVersionGaugeare both accurate and meaningful for the intended metrics. The labels "appid", "req_cluster", "use_cluster", "namespace", "ip", and "version" are appropriately used in the codebase, with "req_cluster" and "use_cluster" representing different aspects of cluster configuration, which justifies their separate usage. This aligns well with the intended functionality of tracking namespace versions in different clusters and configurations.
- appid is correctly used to identify the application.
- req_cluster and use_cluster effectively differentiate between the requested cluster and the cluster actually used, which is crucial for accurate metrics in a distributed environment.
- namespace, ip, and version are straightforward and directly relate to their respective metrics.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the labels used in NamespaceVersionGauge are accurate and meaningful. # Test: Search for the usage of NamespaceVersionGauge. Expect: Labels should match the context where they are used. rg --type go $'NamespaceVersionGauge.WithLabelValues'Length of output: 458
Script:
#!/bin/bash # Description: Verify the purpose of 'req_cluster' and 'use_cluster' in the context of NamespaceVersionGauge. # Test: Search for definitions or comments related to 'appConfig.Cluster' and 'apolloConfig.Cluster' to understand their roles. rg --type go -C 3 'appConfig.Cluster|apolloConfig.Cluster'Length of output: 4415
15-20: Ensure that the labels used inLatestCheckGaugeare accurate and meaningful for the intended metrics.Verification successful
The labels used in
LatestCheckGaugeare confirmed to be accurate and meaningful for the intended metrics. The functionutils.GetInternal(), which corresponds to the "ip" label, is specifically designed to retrieve an internal IP address, aligning perfectly with its usage in the metric. This verification addresses the concern raised in the review comment.
- File:
component/metrics/metrics.go- Label Usage Verification:
component/notify/componet_notify.goScripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the labels used in LatestCheckGauge are accurate and meaningful. # Test: Search for the usage of LatestCheckGauge. Expect: Labels should match the context where they are used. rg --type go $'LatestCheckGauge.WithLabelValues'Length of output: 242
Script:
#!/bin/bash # Description: Verify the function `utils.GetInternal()` to ensure it returns an IP address or an equivalent identifier. # Test: Search for the implementation of `utils.GetInternal()` to understand its functionality. ast-grep --lang go --pattern $'func GetInternal() { $$$ }'Length of output: 61
Script:
#!/bin/bash # Description: Broaden the search for any occurrence of `GetInternal` to catch any definition, usage, or alias. # Test: Search across all Go files for `GetInternal` to understand its context and definition. rg --type go 'GetInternal'Length of output: 1211
component/notify/componet_notify.go (1)
65-68: Ensure that the labels used inLatestCheckGaugeare correctly applied and that the metric accurately reflects the intended measurement.storage/change_event.go (1)
Line range hint
42-98: Ensure that the new fieldsAppIdandClusterin the event structures are correctly utilized throughout the system, especially in metrics and logging.Verification successful
The review comment concerning the utilization of the new fields
AppIdandClusterin metrics and logging has been verified. The fields are actively used in metrics, as evidenced by their inclusion in various metric labels. Although no explicit logging statements were found, the usage in metrics typically fulfills a similar role in monitoring and debugging, which might be considered sufficient depending on the system's architecture and requirements.
- Metrics usage found in:
metrics.OnchangeGauge.WithLabelValues(event.AppId, event.Cluster, event.Namespace, utils.GetInternal()).Set(float64(event.NotificationID))metrics.NamespaceVersionGauge.WithLabelValues(apolloConfig.AppID, appConfig.Cluster, apolloConfig.Cluster, notifyConfig.NamespaceName, utils.GetInternal(), version).Set(1)Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of new fields `AppId` and `Cluster`. # Test: Search for the usage of `AppId` and `Cluster`. Expect: Fields should be correctly utilized in metrics and logging. rg --type go $'AppId|Cluster'Length of output: 3416
component/remote/async.go (1)
88-95: Ensure that the version tracking logic correctly handles the transition between old and new versions, and that metrics are accurately recorded.Verification successful
The version tracking logic in the code correctly handles the transition between old and new versions by setting the metric value for the old version to 0 and for the new version to 1. Additionally, the metrics are accurately recorded with appropriate labels, ensuring that the transitions are well-documented and traceable. Based on this analysis:
- The handling of version transitions in
component/remote/async.gois implemented as expected.- The metrics recording uses consistent labels and correctly reflects the version changes.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the version tracking logic and the accuracy of metrics recording. # Test: Search for the usage of `NamespaceVersionGauge`. Expect: Logic should correctly handle version transitions and metrics should be accurately recorded. rg --type go $'NamespaceVersionGauge.WithLabelValues'Length of output: 458
client.go (1)
111-113: Ensure that the metrics initialization at the start of theStartWithConfigfunction is correctly placed to capture all necessary data.Metrics initialization is correctly placed at the start of the function to ensure all subsequent operations are monitored.
storage/repository.go (1)
622-622: Ensure that the metrics recording in thepushChangeEventfunction correctly reflects the onchange events.Verification successful
The verification process has confirmed that the
OnchangeGaugemetric is used within thepushChangeEventfunction instorage/repository.go. The context provided by the script output shows that the metric is set right before triggering theOnChangelistener, which aligns with the intended purpose of recording onchange events. The parameters used (event.AppId,event.Cluster,event.Namespace, andutils.GetInternal()) and the setting of the gauge tofloat64(event.NotificationID)are appropriate for reflecting the details of the onchange event.Given this information, the implementation of the metrics recording in the
pushChangeEventfunction correctly reflects the onchange events as intended in the original review comment.Metrics Recording in
pushChangeEventFunction is Correct
- The usage of
OnchangeGaugein thepushChangeEventfunction is correctly implemented to reflect onchange events.- The metric is set with appropriate event details and is triggered in the correct function context.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the accuracy of metrics recording in onchange events. # Test: Search for the usage of `OnchangeGauge`. Expect: Metrics should correctly reflect onchange events. rg --type go $'OnchangeGauge.WithLabelValues'Length of output: 207
Script:
#!/bin/bash # Verify that the `OnchangeGauge` is only used in the `pushChangeEvent` function and check its context. rg --type go -C 5 "OnchangeGauge.WithLabelValues"Length of output: 714
| // OnchangeGauge 触发 onchange 监控, 值为版本 | ||
| OnchangeGauge = prometheus.NewGaugeVec(prometheus.GaugeOpts{ | ||
| Name: "onchange", | ||
| Subsystem: "sdk", | ||
| Namespace: "apollo", | ||
| }, []string{"appid", "use_cluster", "namespace", "ip"}) |
There was a problem hiding this comment.
Tip
Codebase Verification
The verification process has revealed a potential issue with the label usage in OnchangeGauge:
- The label "ip" is correctly used to represent IP addresses, aligning with the output of
utils.GetInternal(). - However, the label "use_cluster" might be misleading as it is used with
event.Cluster, which appears to be a cluster identifier rather than indicating a boolean state or usage pattern as the label might suggest.
It would be beneficial to review and possibly revise the label "use_cluster" to ensure it accurately reflects its intended purpose.
Analysis chain
Ensure that the labels used in OnchangeGauge are accurate and meaningful for the intended metrics.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the labels used in OnchangeGauge are accurate and meaningful.
# Test: Search for the usage of OnchangeGauge. Expect: Labels should match the context where they are used.
rg --type go $'OnchangeGauge.WithLabelValues'
Length of output: 207
Script:
#!/bin/bash
# Description: Further investigate the usage of labels in OnchangeGauge.
# Test 1: Check the return type and value of `utils.GetInternal()` to see if it matches the expected IP address format.
rg --type go -A 5 'func GetInternal()'
# Test 2: Search for documentation or comments that clarify the intended use of `event.Cluster` to see if it matches the "use_cluster" label.
rg --type go -C 3 'event.Cluster'
Length of output: 796
| var m = make(map[string]string) | ||
|
|
||
| func GetVersionByNamespace(appid, cluster, namespace string) string { | ||
| return m[appid+cluster+namespace] | ||
| } | ||
|
|
||
| func SetVersionByNamespace(appid, cluster, namespace, version string) { | ||
| m[appid+cluster+namespace] = version | ||
| } |
There was a problem hiding this comment.
Consider using a more robust key structure for the version map to avoid potential key collisions.
- m[appid+cluster+namespace] = version
+ m[appid + "|" + cluster + "|" + namespace] = versionCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| var m = make(map[string]string) | |
| func GetVersionByNamespace(appid, cluster, namespace string) string { | |
| return m[appid+cluster+namespace] | |
| } | |
| func SetVersionByNamespace(appid, cluster, namespace, version string) { | |
| m[appid+cluster+namespace] = version | |
| } | |
| var m = make(map[string]string) | |
| func GetVersionByNamespace(appid, cluster, namespace string) string { | |
| return m[appid+cluster+namespace] | |
| } | |
| func SetVersionByNamespace(appid, cluster, namespace, version string) { | |
| m[appid + "|" + cluster + "|" + namespace] = version | |
| } |
| // SetMetricsRegister 甚至 metrics register | ||
| func SetMetricsRegister(register *prometheus.Registry) { | ||
| globalMetrics = register | ||
| } |
There was a problem hiding this comment.
Typographical error in the comment of SetMetricsRegister. It should be "set" instead of "甚至".
- // SetMetricsRegister 甚至 metrics register
+ // SetMetricsRegister set metrics registerCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| // SetMetricsRegister 甚至 metrics register | |
| func SetMetricsRegister(register *prometheus.Registry) { | |
| globalMetrics = register | |
| } | |
| // SetMetricsRegister set metrics register | |
| func SetMetricsRegister(register *prometheus.Registry) { | |
| globalMetrics = register | |
| } |
| if err := register.Register(metrics.NamespaceVersionGauge); err != nil { | ||
| log.Errorf("register namespace_version metrics fail, error: %v", err) | ||
| } | ||
|
|
||
| // 最近更新内容监控 | ||
| if err := register.Register(metrics.LatestCheckGauge); err != nil { | ||
| log.Errorf("register namespace_version metrics fail, error: %v", err) | ||
| } | ||
|
|
||
| // onchange 监控 | ||
| if err := register.Register(metrics.OnchangeGauge); err != nil { | ||
| log.Errorf("register namespace_version metrics fail, error: %v", err) | ||
| } |
There was a problem hiding this comment.
Ensure error handling is consistent and correct in the registration of metrics. Currently, the error message always refers to namespace_version which might be incorrect for other metrics.
- log.Errorf("register namespace_version metrics fail, error: %v", err)
+ log.Errorf("register %s metrics fail, error: %v", metricName, err)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| if err := register.Register(metrics.NamespaceVersionGauge); err != nil { | |
| log.Errorf("register namespace_version metrics fail, error: %v", err) | |
| } | |
| // 最近更新内容监控 | |
| if err := register.Register(metrics.LatestCheckGauge); err != nil { | |
| log.Errorf("register namespace_version metrics fail, error: %v", err) | |
| } | |
| // onchange 监控 | |
| if err := register.Register(metrics.OnchangeGauge); err != nil { | |
| log.Errorf("register namespace_version metrics fail, error: %v", err) | |
| } | |
| if err := register.Register(metrics.NamespaceVersionGauge); err != nil { | |
| log.Errorf("register %s metrics fail, error: %v", "namespace_version", err) | |
| } | |
| // 最近更新内容监控 | |
| if err := register.Register(metrics.LatestCheckGauge); err != nil { | |
| log.Errorf("register %s metrics fail, error: %v", "latest_check", err) | |
| } | |
| // onchange 监控 | |
| if err := register.Register(metrics.OnchangeGauge); err != nil { | |
| log.Errorf("register %s metrics fail, error: %v", "onchange", err) | |
| } |
新增 metrics 链路指标监控
Summary by CodeRabbit
New Features
AppIdandClusterto change events for better context in configurations.Enhancements
StartWithConfigfunction.Documentation