🐛 Remove cluster label - #142
Conversation
Signed-off-by: clyang82 <chuyang@redhat.com>
WalkthroughReplaces cluster-based CloudEvents metrics labeling with consumer/agent-based labels across generic and gRPC metrics; updates metric definitions, emission functions, interceptors/streams, telemetry call sites, tests, and metric registration APIs accordingly. Changes
Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/cloudevents/generic/metrics_collector.go (1)
205-212: Bug: Unregister duplicates status metric and misses spec metric.UnregisterCloudEventsMetrics unregisters resourceStatusResyncDurationMetric twice and never unregisters resourceSpecResyncDurationMetric. This will leak the spec histogram in tests/process lifetime.
func UnregisterCloudEventsMetrics(register prometheus.Registerer) { register.Unregister(cloudeventsReceivedCounterMetric) register.Unregister(cloudeventsSentCounterMetric) - register.Unregister(resourceStatusResyncDurationMetric) - register.Unregister(resourceStatusResyncDurationMetric) + register.Unregister(resourceSpecResyncDurationMetric) + register.Unregister(resourceStatusResyncDurationMetric) register.Unregister(clientReconnectedCounterMetric) register.Unregister(workProcessedCounterMetric) }
🧹 Nitpick comments (9)
pkg/server/grpc/metrics/metrics_test.go (2)
143-145: Byte-counter expectations may be brittle across protobuf changes.Asserting exact values (2286, 69) can become flaky with minor proto or marshalling changes. Consider asserting presence and “> 0” (or a small range) instead of exact equality to reduce test fragility.
114-114: Typo: “gaugge” → “gauge”.Minor comment nit.
- // assert gaugge and counter metrics + // assert gauge and counter metricspkg/cloudevents/generic/metrics_collector.go (3)
224-234: Fix comment: “received” vs “sent”.The docstring above increaseCloudEventsReceivedCounter says “sent” but the function increments the received counter.
-// increaseCloudEventsReceivedCounter increases the cloudevents sent counter metric: +// increaseCloudEventsReceivedCounter increases the cloudevents received counter metric:
104-109: Docs nit: “second” → “seconds”.Metric names are *_duration_seconds; comments should match.
-// The resource spec resync duration metric is a histogram with a base metric name of 'resource_spec_resync_duration_second' +// The resource spec resync duration metric is a histogram with a base metric name of 'resource_spec_resync_duration_seconds' -// The resource status resync duration metric is a histogram with a base metric name of 'resource_status_resync_duration_second' +// The resource status resync duration metric is a histogram with a base metric name of 'resource_status_resync_duration_seconds'Also applies to: 138-145
225-233: Unused cluster params in helpers (intentional) — consider follow-up cleanup.These functions still accept cluster but no longer use it for labels. If these are package-internal, consider dropping or renaming the param to “_ string” to signal intentional non-use and avoid confusion. Defer if API stability is a concern for this PR.
Example (one function shown):
-func increaseCloudEventsReceivedCounter(source, cluster, dataType, subresource, action string) { +func increaseCloudEventsReceivedCounter(source, _ string, dataType, subresource, action string) {Also applies to: 236-248, 251-258, 261-268
pkg/cloudevents/generic/metrics_collector_test.go (1)
98-100: Reduce time-based flakiness by polling instead of fixed sleeps.Replace fixed sleeps with polling (require.Eventually or a simple loop with timeout) to make tests resilient under load/CI variance.
Example sketch:
require.Eventually(t, func() bool { // read counter and compare against expected return int(toFloat64Counter(receivedTotal)) == want }, 2*time.Second, 50*time.Millisecond)Also applies to: 241-243, 281-282
pkg/cloudevents/server/grpc/metrics/metrics.go (3)
108-131: Consider counting “called” and “msg_received” even on early unary errors.Currently, called_total and msg_received_total are incremented only after successful CE parse; early errors (bad request type, CE conversion/type parse failures) record processed/duration but not called/received. If you want “called” to reflect every RPC invocation (successful or not), increment those counters with data_type="unknown" before the early returns.
- pubReq, ok := req.(*pbv1.PublishRequest) + pubReq, ok := req.(*pbv1.PublishRequest) if !ok { - err := fmt.Errorf("invalid request type for Publish method") - recordCloudEventsMetrics(dataType, method, err, startTime) + err := fmt.Errorf("invalid request type for Publish method") + grpcCECalledCountMetric.WithLabelValues(dataType, method).Inc() + grpcCEMessageReceivedCountMetric.WithLabelValues(dataType, method).Inc() + recordCloudEventsMetrics(dataType, method, err, startTime) return nil, err } // convert the request to cloudevent and extract the source evt, err := binding.ToEvent(ctx, protocol.NewMessage(pubReq.Event)) if err != nil { - err = fmt.Errorf("failed to convert to cloudevent: %v", err) - recordCloudEventsMetrics(dataType, method, err, startTime) + err = fmt.Errorf("failed to convert to cloudevent: %v", err) + grpcCECalledCountMetric.WithLabelValues(dataType, method).Inc() + grpcCEMessageReceivedCountMetric.WithLabelValues(dataType, method).Inc() + recordCloudEventsMetrics(dataType, method, err, startTime) return nil, err } // extract the data type from event type eventType, err := types.ParseCloudEventsType(evt.Type()) if err != nil { - err = fmt.Errorf("failed to parse cloud event type %s, %v", evt.Type(), err) - recordCloudEventsMetrics(dataType, method, err, startTime) + err = fmt.Errorf("failed to parse cloud event type %s, %v", evt.Type(), err) + grpcCECalledCountMetric.WithLabelValues(dataType, method).Inc() + grpcCEMessageReceivedCountMetric.WithLabelValues(dataType, method).Inc() + recordCloudEventsMetrics(dataType, method, err, startTime) return nil, err } - dataType = eventType.CloudEventsDataType.String() + dataType = eventType.CloudEventsDataType.String() + grpcCECalledCountMetric.WithLabelValues(dataType, method).Inc() + grpcCEMessageReceivedCountMetric.WithLabelValues(dataType, method).Inc() - grpcCECalledCountMetric.WithLabelValues(dataType, method).Inc() - grpcCEMessageReceivedCountMetric.WithLabelValues(dataType, method).Inc()If prior behavior was intentional, please confirm and disregard.
Also applies to: 134-146
169-175: Update comment: no longer capturing cluster in stream path.Comment still mentions “captures the cluster and data type”; cluster is removed.
-// It captures the cluster and data type from the SubscriptionRequest and emits metrics. +// It captures the data type from the SubscriptionRequest and emits metrics.
96-107: Optional: guard by method == "Publish" in unary interceptor.If additional unary methods are ever added to the CloudEventService, the current logic will treat them as Publish and error on type assertions. Consider short-circuiting when method != "Publish".
- if service != gRPCCloudEventService { + if service != gRPCCloudEventService { return handler(ctx, req) } + if _, method := SplitMethod(info.FullMethod); method != "Publish" { + return handler(ctx, req) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
pkg/cloudevents/generic/metrics_collector.go(4 hunks)pkg/cloudevents/generic/metrics_collector_test.go(3 hunks)pkg/cloudevents/server/grpc/metrics/metrics.go(4 hunks)pkg/server/grpc/metrics/metrics_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-27T03:21:36.789Z
Learnt from: morvencao
PR: open-cluster-management-io/sdk-go#138
File: pkg/server/grpc/metrics/metrics.go:230-236
Timestamp: 2025-08-27T03:21:36.789Z
Learning: In the gRPC metrics implementation for open-cluster-management.io/sdk-go, the grpc_server_ce_called_total metric intentionally has different semantics for unary vs stream calls: unary calls increment it once per RPC invocation, while stream calls increment it once per received message. This is deliberate design to provide granular visibility into stream message activity.
Applied to files:
pkg/server/grpc/metrics/metrics_test.gopkg/cloudevents/server/grpc/metrics/metrics.go
📚 Learning: 2025-09-01T03:34:05.141Z
Learnt from: morvencao
PR: open-cluster-management-io/sdk-go#138
File: pkg/cloudevents/server/grpc/metrics/metrics.go:231-254
Timestamp: 2025-09-01T03:34:05.141Z
Learning: In open-cluster-management.io/sdk-go gRPC CloudEvents metrics, processing duration metrics should only be recorded for unary RPCs, not stream RPCs. Stream RPCs can be long-lived connections that persist as long as the gRPC server runs, making duration metrics confusing and less useful for operators debugging issues.
Applied to files:
pkg/server/grpc/metrics/metrics_test.gopkg/cloudevents/server/grpc/metrics/metrics.go
🧬 Code graph analysis (2)
pkg/cloudevents/generic/metrics_collector_test.go (1)
pkg/cloudevents/generic/types/types.go (4)
SubResourceSpec(27-27)ResyncRequestAction(38-38)ResyncResponseAction(41-41)SubResourceStatus(30-30)
pkg/cloudevents/server/grpc/metrics/metrics.go (1)
pkg/cloudevents/generic/types/types.go (2)
ParseCloudEventsType(209-230)CloudEventsDataType(165-169)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: integration
- GitHub Check: verify
- GitHub Check: unit
🔇 Additional comments (6)
pkg/server/grpc/metrics/metrics_test.go (2)
129-141: Cluster label removal reflected correctly in CE counters.Expected lines for grpc_server_ce_(called|msg_received|processed)_total now use only data_type and method (plus grpc_code for processed). This aligns with the PR goal and prior semantics.
120-121: Unary-only duration histogram assertion is correct.Asserting grpc_server_ce_processing_duration_seconds only for Publish (unary) matches the intended design for skipping duration on streams.
pkg/cloudevents/generic/metrics_collector_test.go (3)
101-105: Label order updates for sent/received totals look right.Sent: source, original_source, type, subresource, action. Received: source, type, subresource, action. Counts match len(resources).
238-253: Spec resync assertions align with new label schema.Received uses clusterName as source (from evt.Source), histogram labels (sourceID, type), and sent uses (sourceID, none, type, spec, resync_response). All consistent.
277-292: Status resync assertions align with new label schema.Received uses sourceID as source (from evt.Source), histogram labels (sourceID, type), sent uses (testAgentName, none, type, status, resync_response).
pkg/cloudevents/server/grpc/metrics/metrics.go (1)
219-229: LGTM: stream processed_total increments on handler exit; no duration histogram.Matches the documented behavior for long-lived streams.
|
/hold |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/cloudevents/generic/metrics_collector.go (1)
212-214: Bug: Unregister duplicates status metric and skips spec metricUnregister calls status twice and never unregisters spec.
Apply this diff:
func UnregisterCloudEventsMetrics(register prometheus.Registerer) { register.Unregister(cloudeventsReceivedCounterMetric) register.Unregister(cloudeventsSentCounterMetric) - register.Unregister(resourceStatusResyncDurationMetric) - register.Unregister(resourceStatusResyncDurationMetric) + register.Unregister(resourceSpecResyncDurationMetric) + register.Unregister(resourceStatusResyncDurationMetric) register.Unregister(clientReconnectedCounterMetric) register.Unregister(workProcessedCounterMetric) }
🧹 Nitpick comments (2)
pkg/cloudevents/generic/metrics_collector.go (2)
31-38: Label sets updated to use consumer — OK, but watch cardinalityThe label swaps look correct across received/sent/resync metrics. Please ensure consumer values are bounded and stable (avoid per-connection IDs) to keep Prometheus cardinality under control. Consider adding a brief doc comment noting that any cluster mapping should be handled via scrape-time relabeling, per PR intent.
Also applies to: 40-48, 50-56
108-113: Docs: “second” → “seconds” (nit) and metric name wordingMinor: the prose says “…_duration_second” but the metric name is “…_duration_seconds”. Recommend fixing the wording to match.
Apply this diff to the comment text only:
-// The resource spec resync duration metric is a histogram with a base metric name of 'resource_spec_resync_duration_second' +// The resource spec resync duration metric is a histogram with a base metric name of 'resource_spec_resync_duration_seconds' ... -// The resource status resync duration metric is a histogram with a base metric name of 'resource_status_resync_duration_second' +// The resource status resync duration metric is a histogram with a base metric name of 'resource_status_resync_duration_seconds'Also applies to: 142-147
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/cloudevents/generic/metrics_collector.go(11 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: integration
- GitHub Check: build
- GitHub Check: unit
- GitHub Check: verify
🔇 Additional comments (5)
pkg/cloudevents/generic/metrics_collector.go (5)
20-20: Introduce consumer label — OKReplacing the removed cluster label with consumer is consistent with the PR goal in this package.
80-83: Examples updated — OKExamples now show consumer instead of cluster and align with the label sets.
Also applies to: 96-99, 114-123, 148-157
228-238: Resolved — callers pass consumer correctlyBoth call sites pass the consumer as the second argument: pkg/cloudevents/generic/agentclient.go:156 and pkg/cloudevents/generic/sourceclient.go:161.
256-265: Signature change verified — callers updated. Labels match the metric vectors; callers updated in pkg/cloudevents/generic/sourceclient.go:179 and pkg/cloudevents/generic/agentclient.go:168.
241-254: Signature change: sent counter now requires consumer — call sites verifiedImplementation preserves originalSource fallback; repo calls in pkg/cloudevents/generic (sourceclient.go, agentclient.go) pass the consumer as the third argument and match the new parameter order.
Signed-off-by: clyang82 <chuyang@redhat.com>
7ffa823 to
525b638
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/cloudevents/generic/metrics_collector.go (1)
210-216: BUG: Unregister duplicates status metric and misses spec metricSpec histogram is never unregistered; status is unregistered twice.
Apply this:
func UnregisterCloudEventsMetrics(register prometheus.Registerer) { register.Unregister(cloudeventsReceivedCounterMetric) register.Unregister(cloudeventsSentCounterMetric) - register.Unregister(resourceStatusResyncDurationMetric) - register.Unregister(resourceStatusResyncDurationMetric) + register.Unregister(resourceSpecResyncDurationMetric) + register.Unregister(resourceStatusResyncDurationMetric) register.Unregister(clientReconnectedCounterMetric) register.Unregister(workProcessedCounterMetric) }
🧹 Nitpick comments (1)
pkg/cloudevents/generic/metrics_collector.go (1)
96-99: Docs: sent_total examples omit consumer label but code requires itExamples should reflect the declared label set; otherwise they’ll mislead operators.
Apply this doc-only tweak:
-// cloudevents_sent_total{source="source1",original_source="none",type="io.open-cluster-management.works.v1alpha1.manifestbundles",subresource="spec",action="create"} 1 -// cloudevents_sent_total{source="consumer1-work-agent",original_source="source1",type="io.open-cluster-management.works.v1alpha1.manifestbundles",subresource="status",action="update"} 2 +// cloudevents_sent_total{source="source1",original_source="none",consumer="consumer1",type="io.open-cluster-management.works.v1alpha1.manifestbundles",subresource="spec",action="create"} 1 +// cloudevents_sent_total{source="consumer1-work-agent",original_source="source1",consumer="",type="io.open-cluster-management.works.v1alpha1.manifestbundles",subresource="status",action="update"} 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/cloudevents/generic/agentclient.go(3 hunks)pkg/cloudevents/generic/metrics_collector.go(9 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-01T03:34:05.141Z
Learnt from: morvencao
PR: open-cluster-management-io/sdk-go#138
File: pkg/cloudevents/server/grpc/metrics/metrics.go:231-254
Timestamp: 2025-09-01T03:34:05.141Z
Learning: In open-cluster-management.io/sdk-go gRPC CloudEvents metrics, processing duration metrics should only be recorded for unary RPCs, not stream RPCs. Stream RPCs can be long-lived connections that persist as long as the gRPC server runs, making duration metrics confusing and less useful for operators debugging issues.
Applied to files:
pkg/cloudevents/generic/metrics_collector.go
🧬 Code graph analysis (1)
pkg/cloudevents/generic/agentclient.go (1)
pkg/cloudevents/generic/types/types.go (1)
CloudEventsDataType(165-169)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: unit
- GitHub Check: integration
- GitHub Check: verify
🔇 Additional comments (11)
pkg/cloudevents/generic/metrics_collector.go (7)
20-27: Renaming to consumer label looks goodConsistent with PR intent to move off "cluster".
32-38: Received metrics label set update is correctIncluding consumer in the label set aligns with docs.
41-48: Sent metrics label set includes consumer — watch cardinalityOK to include, but see below: With() must always receive a value for every declared label.
51-55: Resync metrics label set update is correct
114-123: Docs: duration metric examples look consistentExamples reflect consumer-based labeling.
Also applies to: 148-157
263-271: Resync (spec) duration label population is fineIncludes consumer consistently.
274-282: Resync (status) duration label population is fineIncludes consumer consistently.
pkg/cloudevents/generic/agentclient.go (4)
114-116: Agent-sent counter call updated — OKPassing empty consumer matches the new API; safe once cardinality fix lands.
135-136: Publish path counter call updated — OK
156-157: Receive path counter call updated — OK
168-169: Confirm whether agent-side duration metrics should expose consumer=clusterNamePR says agent metrics should not carry cluster identity; here consumer is set to c.clusterName. If we intend to remove cluster semantics from agent metrics, pass empty consumer instead.
Suggest:
- updateResourceStatusResyncDurationMetric(evt.Source(), c.clusterName, eventType.CloudEventsDataType.String(), startTime) + updateResourceStatusResyncDurationMetric(evt.Source(), "", eventType.CloudEventsDataType.String(), startTime)If keeping clusterName is desired, please confirm in the PR description and update docs accordingly.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
pkg/cloudevents/generic/metrics_collector.go (1)
279-289: Good fix: Stable label cardinality on countersConsumer is now always present in labels (even if empty) and original_source defaults to "none", preventing With() panics.
Also applies to: 302-316
🧹 Nitpick comments (4)
pkg/cloudevents/generic/metrics_collector.go (1)
67-73: Verify: Agent status-resync histogram still carries consumer labelThe shared resync histogram label set includes consumer, and updateResourceStatusResyncDurationMetric() is called by the agent (see agentclient.go Line 168). If the objective is “no cluster/consumer label from agent,” this contradicts it. Confirm whether:
- agent-side resync duration should omit consumer entirely (new Vec without consumer), or
- keeping consumer on histograms is acceptable and only counters drop it.
If needed, I can propose a split of label sets/metrics for source vs agent resync durations.
Also applies to: 345-353
pkg/cloudevents/generic/sourceclient.go (2)
132-134: Avoid potential panic on clusterName extractionUse safe conversion to prevent a type-assertion panic if the extension is missing or not a string.
- clusterName := evt.Context.GetExtensions()[types.ExtensionClusterName].(string) + clusterName, _ := cloudeventstypes.ToString( + evt.Context.GetExtensions()[types.ExtensionClusterName], + )
244-248: Minor grammar nit in log message- klog.V(4).Infof("there are is no objs from the list, do nothing") + klog.V(4).Infof("no objects returned from the list; nothing to do")pkg/cloudevents/generic/agentclient.go (1)
168-169: Confirm agent histogram labeling vs PR goalupdateResourceStatusResyncDurationMetric(evt.Source(), c.clusterName, ...) will emit consumer=clusterName from the agent. If agent metrics must drop cluster/consumer entirely, consider a consumer-less histogram for the agent path or relabel-drop in the scrape pipeline.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pkg/cloudevents/generic/agentclient.go(3 hunks)pkg/cloudevents/generic/metrics_collector.go(8 hunks)pkg/cloudevents/generic/sourceclient.go(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-01T03:34:05.141Z
Learnt from: morvencao
PR: open-cluster-management-io/sdk-go#138
File: pkg/cloudevents/server/grpc/metrics/metrics.go:231-254
Timestamp: 2025-09-01T03:34:05.141Z
Learning: In open-cluster-management.io/sdk-go gRPC CloudEvents metrics, processing duration metrics should only be recorded for unary RPCs, not stream RPCs. Stream RPCs can be long-lived connections that persist as long as the gRPC server runs, making duration metrics confusing and less useful for operators debugging issues.
Applied to files:
pkg/cloudevents/generic/metrics_collector.go
🧬 Code graph analysis (2)
pkg/cloudevents/generic/sourceclient.go (1)
pkg/cloudevents/generic/types/types.go (1)
CloudEventsDataType(165-169)
pkg/cloudevents/generic/agentclient.go (1)
pkg/cloudevents/generic/types/types.go (1)
CloudEventsDataType(165-169)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: integration
- GitHub Check: unit
- GitHub Check: verify
🔇 Additional comments (5)
pkg/cloudevents/generic/metrics_collector.go (3)
31-38: LGTM: Label schemas cleanly split by emitterSource metrics include consumer; agent metrics exclude it. This matches the PR intent to drop cluster/consumer on agent-originated counters.
Also applies to: 40-47, 48-56, 58-66
97-152: LGTM: New counter families and help textMetric renames and new families look consistent; examples align with label schemas.
243-253: LGTM: Metrics lifecycle coveredRegister/Unregister/Reset updated for all new collectors.
Also applies to: 256-265, 268-277
pkg/cloudevents/generic/sourceclient.go (1)
108-109: LGTM: Telemetry calls updated to source/agent-specific countersArgument ordering matches the new function signatures; consumer is the target cluster as intended.
Also applies to: 133-134, 161-162, 179-181, 293-294
pkg/cloudevents/generic/agentclient.go (1)
114-115: LGTM: Agent telemetry migrated to new countersSent-from-agent and received-by-agent calls look correct; original_source defaulting handled by the metric helper.
Also applies to: 135-136, 156-157
c500578 to
a844be9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/cloudevents/generic/agentclient.go (1)
169-169: Agent is still exporting cluster as “consumer” in resync durationThis contradicts the PR objective (“agent metrics remove cluster”). Either omit the consumer label for agent histograms (preferred via metric split), or at least pass an empty value to avoid per-cluster cardinality here.
- updateResourceStatusResyncDurationMetric(evt.Source(), c.clusterName, eventType.CloudEventsDataType.String(), startTime) + // Agent side should not expose cluster/consumer; pass empty to avoid cardinality. + updateResourceStatusResyncDurationMetric(evt.Source(), "", eventType.CloudEventsDataType.String(), startTime)If you want true removal (no consumer label at all), we should introduce agent-specific histograms without the consumer label and call those here. I can provide that patch.
🧹 Nitpick comments (2)
pkg/cloudevents/generic/metrics_collector.go (2)
67-73: Confirm intent: resync duration metrics still require the consumer labelPR objective says agent-produced metrics should remove the cluster label; here, resync histograms always include consumer. If agents emit these, you'll retain a cluster-derived label in practice. If the intent is “no consumer on agent,” consider splitting resync histograms into agent/server variants (agent: source,type; server: source,consumer,type). Otherwise, pass consumer="" from agent call sites to minimize cardinality.
I can draft a minimal split (new ByAgent/BySource histograms and wrappers) if you confirm the desired surface.
151-156: NIT: comments say “second” but metric is “seconds”Minor doc accuracy fix.
-// The resource spec resync duration metric is a histogram with a base metric name of 'resource_spec_resync_duration_second' +// The resource spec resync duration metric is a histogram with a base metric name of 'resource_spec_resync_duration_seconds' ... -// The resource status resync duration metric is a histogram with a base metric name of 'resource_status_resync_duration_second' +// The resource status resync duration metric is a histogram with a base metric name of 'resource_status_resync_duration_seconds'Also applies to: 185-191
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pkg/cloudevents/generic/agentclient.go(3 hunks)pkg/cloudevents/generic/metrics_collector.go(8 hunks)pkg/cloudevents/generic/sourceclient.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/cloudevents/generic/sourceclient.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-01T03:34:05.141Z
Learnt from: morvencao
PR: open-cluster-management-io/sdk-go#138
File: pkg/cloudevents/server/grpc/metrics/metrics.go:231-254
Timestamp: 2025-09-01T03:34:05.141Z
Learning: In open-cluster-management.io/sdk-go gRPC CloudEvents metrics, processing duration metrics should only be recorded for unary RPCs, not stream RPCs. Stream RPCs can be long-lived connections that persist as long as the gRPC server runs, making duration metrics confusing and less useful for operators debugging issues.
Applied to files:
pkg/cloudevents/generic/metrics_collector.go
🧬 Code graph analysis (1)
pkg/cloudevents/generic/agentclient.go (1)
pkg/cloudevents/generic/types/types.go (1)
CloudEventsDataType(165-169)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: unit
- GitHub Check: integration
- GitHub Check: build
- GitHub Check: verify
🔇 Additional comments (1)
pkg/cloudevents/generic/agentclient.go (1)
114-114: LGTM: agent counters migrated to role-specific helpersCalls switch to increaseCloudEventsSentFromAgentCounter / increaseCloudEventsReceivedByAgentCounter post-successful publish and on receipt. Argument ordering and semantics look correct.
Also applies to: 135-135, 156-156
a844be9 to
4b522ed
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
pkg/cloudevents/generic/metrics_collector.go (2)
257-267: Cardinality fix looks goodAlways including consumer in source-side counters resolves the inconsistent-label panic raised earlier.
Also applies to: 280-294
95-108: Make CloudEvents metric names unique to avoid Prometheus registration panicsAll four CounterVecs use the same Name constants (receivedCounterMetric="received_total", sentCounterMetric="sent_total") in pkg/cloudevents/generic/metrics_collector.go; this will panic if both RegisterClientCloudEventsMetrics and RegisterSourceCloudEventsMetrics run in one process. Give each CounterVec a unique Name and update the doc examples.
Apply:
- Name: receivedCounterMetric, + Name: "received_by_source_total",- Name: receivedCounterMetric, + Name: "received_by_client_total",- Name: sentCounterMetric, + Name: "sent_from_source_total",- Name: sentCounterMetric, + Name: "sent_from_client_total",Update doc examples accordingly:
-// cloudevents_received_total{source="agent1",consumer="consumer1",type="io.open-cluster-management.works.v1alpha1.manifests",subresource="spec",action="create"} 1 -// cloudevents_received_total{source="agent1",consumer="consumer1",type="io.open-cluster-management.works.v1alpha1.manifests",subresource="spec",action="update"} 1 +// cloudevents_received_by_source_total{source="agent1",consumer="consumer1",type="io.open-cluster-management.works.v1alpha1.manifests",subresource="spec",action="create"} 1 +// cloudevents_received_by_source_total{source="agent1",consumer="consumer1",type="io.open-cluster-management.works.v1alpha1.manifests",subresource="spec",action="update"} 1-// cloudevents_received_total{source="source1",type=...,subresource="spec",action="create"} 1 -// cloudevents_received_total{source="source1",type=...,subresource="spec",action="update"} 1 +// cloudevents_received_by_client_total{source="source1",type=...,subresource="spec",action="create"} 1 +// cloudevents_received_by_client_total{source="source1",type=...,subresource="spec",action="update"} 1-// cloudevents_sent_total{source="source1",original_source="none",consumer="consumer1",type=...,subresource="spec",action="create"} 1 +// cloudevents_sent_from_source_total{source="source1",original_source="none",consumer="consumer1",type=...,subresource="spec",action="create"} 1-// cloudevents_sent_total{source="consumer1-work-agent",original_source="source1",type=...,subresource="status",action="update"} 2 +// cloudevents_sent_from_client_total{source="consumer1-work-agent",original_source="source1",type=...,subresource="status",action="update"} 2
🧹 Nitpick comments (1)
pkg/cloudevents/generic/metrics_collector.go (1)
110-116: Unify terminology: rename 'Agent' → 'Client' for helper funcs and commentsRename the two unexported helpers and update their callers.
- pkg/cloudevents/generic/metrics_collector.go: rename increaseCloudEventsReceivedByAgentCounter (def at line 270) → increaseCloudEventsReceivedByClientCounter; change comment "received by agent" → "received by client".
- pkg/cloudevents/generic/metrics_collector.go: rename increaseCloudEventsSentFromAgentCounter (def at line 297) → increaseCloudEventsSentFromClientCounter; change comment "sent from agent" → "sent from client".
- Update callers in pkg/cloudevents/generic/agentclient.go (lines 114, 135, 156).
-// The cloudevents received by agent counter metric... +// The cloudevents received by client counter metric... -func increaseCloudEventsReceivedByAgentCounter(...) { +func increaseCloudEventsReceivedByClientCounter(...) {-// The cloudevents sent from agent counter metric... +// The cloudevents sent from client counter metric... -func increaseCloudEventsSentFromAgentCounter(...) { +func increaseCloudEventsSentFromClientCounter(...) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pkg/cloudevents/generic/agentclient.go(3 hunks)pkg/cloudevents/generic/metrics_collector.go(7 hunks)pkg/cloudevents/generic/sourceclient.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/cloudevents/generic/sourceclient.go
- pkg/cloudevents/generic/agentclient.go
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: morvencao
PR: open-cluster-management-io/sdk-go#138
File: pkg/cloudevents/server/grpc/metrics/metrics.go:231-254
Timestamp: 2025-09-01T03:34:05.141Z
Learning: In open-cluster-management.io/sdk-go gRPC CloudEvents metrics, processing duration metrics should only be recorded for unary RPCs, not stream RPCs. Stream RPCs can be long-lived connections that persist as long as the gRPC server runs, making duration metrics confusing and less useful for operators debugging issues.
📚 Learning: 2025-09-01T03:34:05.141Z
Learnt from: morvencao
PR: open-cluster-management-io/sdk-go#138
File: pkg/cloudevents/server/grpc/metrics/metrics.go:231-254
Timestamp: 2025-09-01T03:34:05.141Z
Learning: In open-cluster-management.io/sdk-go gRPC CloudEvents metrics, processing duration metrics should only be recorded for unary RPCs, not stream RPCs. Stream RPCs can be long-lived connections that persist as long as the gRPC server runs, making duration metrics confusing and less useful for operators debugging issues.
Applied to files:
pkg/cloudevents/generic/metrics_collector.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: unit
- GitHub Check: integration
- GitHub Check: verify
| // cloudeventsResyncMetricsLabels - Array of labels added to cloudevents resync metrics: | ||
| var cloudeventsResyncMetricsLabels = []string{ | ||
| metricsSourceLabel, // source | ||
| metricsClusterLabel, // cluster | ||
| metricsConsumerLabel, // consumer | ||
| metricsDataTypeLabel, // data type, e.g. manifests, manifestbundles | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Agent-status resync metrics still carry “consumer” label; remove it to meet PR goal (agent has no cluster/consumer label)
Per PR objective, agent-produced metrics must drop cluster/consumer entirely. Split spec/status label sets so status (agent) has no consumer dimension.
Apply:
-// cloudeventsResyncMetricsLabels - Array of labels added to cloudevents resync metrics:
-var cloudeventsResyncMetricsLabels = []string{
- metricsSourceLabel, // source
- metricsConsumerLabel, // consumer
- metricsDataTypeLabel, // data type, e.g. manifests, manifestbundles
-}
+// resourceSpecResyncMetricsLabels: server-side (source) resync labels
+var resourceSpecResyncMetricsLabels = []string{
+ metricsSourceLabel, // source
+ metricsConsumerLabel, // consumer
+ metricsDataTypeLabel, // data type
+}
+
+// resourceStatusResyncMetricsLabels: agent-side (client) resync labels (no consumer)
+var resourceStatusResyncMetricsLabels = []string{
+ metricsSourceLabel, // source
+ metricsDataTypeLabel, // data type
+}- resourceSpecResyncDurationMetric.With(labels).Observe(duration.Seconds())
+ resourceSpecResyncDurationMetric.With(labels).Observe(duration.Seconds())- cloudeventsResyncMetricsLabels,
+ resourceSpecResyncMetricsLabels,- cloudeventsResyncMetricsLabels,
+ resourceStatusResyncMetricsLabels, func updateResourceStatusResyncDurationMetric(source, consumer, dataType string, startTime time.Time) {
labels := prometheus.Labels{
metricsSourceLabel: source,
- metricsConsumerLabel: consumer,
metricsDataTypeLabel: dataType,
}
duration := time.Since(startTime)
resourceStatusResyncDurationMetric.With(labels).Observe(duration.Seconds())
}Please also update the status-resync doc examples here to drop consumer=… accordingly.
Also applies to: 185-217, 323-331
🤖 Prompt for AI Agents
In pkg/cloudevents/generic/metrics_collector.go around lines 67-73, the
cloudeventsResyncMetricsLabels array currently includes metricsConsumerLabel but
agent-produced status resync metrics must not have a consumer/cluster label;
remove metricsConsumerLabel from this status label set, create/ensure a separate
spec-resync label array that still includes consumer where appropriate, and
update the other affected ranges (lines ~185-217 and ~323-331) to use the
correct label set for status vs spec metrics (remove any consumer reference for
status metrics). Also update the status-resync documentation examples to drop
consumer=... accordingly.
| // Register the metrics | ||
| func RegisterClientCloudEventsMetrics(register prometheus.Registerer) { | ||
| register.MustRegister(cloudeventsReceivedByClientCounterMetric) | ||
| register.MustRegister(cloudeventsSentFromClientCounterMetric) | ||
| register.MustRegister(resourceStatusResyncDurationMetric) | ||
| register.MustRegister(clientReconnectedCounterMetric) | ||
| register.MustRegister(workProcessedCounterMetric) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Register client reconnect metric in client registrar (and remove from source registrar)
Reconnection is a client concern. Today it’s only registered in the source path.
Apply:
func RegisterClientCloudEventsMetrics(register prometheus.Registerer) {
register.MustRegister(cloudeventsReceivedByClientCounterMetric)
register.MustRegister(cloudeventsSentFromClientCounterMetric)
register.MustRegister(resourceStatusResyncDurationMetric)
+ register.MustRegister(clientReconnectedCounterMetric)
register.MustRegister(workProcessedCounterMetric)
} func RegisterSourceCloudEventsMetrics(register prometheus.Registerer) {
register.MustRegister(cloudeventsReceivedBySourceCounterMetric)
register.MustRegister(cloudeventsSentFromSourceCounterMetric)
register.MustRegister(resourceSpecResyncDurationMetric)
- register.MustRegister(clientReconnectedCounterMetric)
}Also applies to: 249-255
🤖 Prompt for AI Agents
In pkg/cloudevents/generic/metrics_collector.go around lines 241-247 (and
likewise adjust 249-255), the metric that tracks client reconnections is being
registered in the source registrar but belongs to the client registrar; remove
the client reconnect metric registration from the source registrar functions and
add its MustRegister call to RegisterClientCloudEventsMetrics so all
client-related metrics (including the client reconnect metric) are registered
there. Ensure you remove the duplicate registration in the source registrar and
only register the reconnect metric once in the client registrar.
4b522ed to
7b35b3c
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
pkg/cloudevents/generic/agentclient.go (1)
156-169: Fix: status resync duration should not include consumer/clusterPer PR, agent-side metrics must not have a consumer/cluster label. Drop the consumer argument here and in metrics_collector.go signature.
Apply:
- updateResourceStatusResyncDurationMetric(evt.Source(), c.clusterName, eventType.CloudEventsDataType.String(), startTime) + updateResourceStatusResyncDurationMetric(evt.Source(), eventType.CloudEventsDataType.String(), startTime)pkg/cloudevents/generic/metrics_collector_test.go (1)
308-315: Fix status resync histogram and client 'sent' labels: drop consumerStatus resync histogram (agent) must not carry consumer; client 'sent' counter also has no consumer.
- h := resourceStatusResyncDurationMetric.WithLabelValues(c.sourceID, c.clusterName, c.dataType.String()) + h := resourceStatusResyncDurationMetric.WithLabelValues(c.sourceID, c.dataType.String()) ... - sentTotal := cloudeventsSentFromClientCounterMetric.WithLabelValues(testAgentName, noneOriginalSource, c.clusterName, c.dataType.String(), string(types.SubResourceStatus), string(types.ResyncResponseAction)) + sentTotal := cloudeventsSentFromClientCounterMetric.WithLabelValues(testAgentName, noneOriginalSource, c.dataType.String(), string(types.SubResourceStatus), string(types.ResyncResponseAction))pkg/cloudevents/generic/metrics_collector.go (2)
167-184: Use split label sets for histogramsWire spec to resourceSpecResyncMetricsLabels and status to resourceStatusResyncMetricsLabels.
var resourceSpecResyncDurationMetric = prometheus.NewHistogramVec( prometheus.HistogramOpts{ Subsystem: resourcesMetricsSubsystem, Name: specResyncDurationMetric, Help: "The duration of the resource spec resync in seconds.", Buckets: []float64{0.1, 0.2, 0.5, 1.0, 2.0, 10.0, 30.0}, }, - cloudeventsResyncMetricsLabels, + resourceSpecResyncMetricsLabels, ) ... var resourceStatusResyncDurationMetric = prometheus.NewHistogramVec( prometheus.HistogramOpts{ Subsystem: resourcesMetricsSubsystem, Name: statusResyncDurationMetric, Help: "The duration of the resource status resync in seconds.", Buckets: []float64{0.1, 0.2, 0.5, 1.0, 2.0, 10.0, 30.0}, }, - cloudeventsResyncMetricsLabels, + resourceStatusResyncMetricsLabels, )
338-347: Drop consumer from status resync duration function/signatureAlign with agent-side label schema (no consumer).
-func updateResourceStatusResyncDurationMetric(source, consumer, dataType string, startTime time.Time) { +func updateResourceStatusResyncDurationMetric(source, dataType string, startTime time.Time) { labels := prometheus.Labels{ metricsSourceLabel: source, - metricsConsumerLabel: consumer, metricsDataTypeLabel: dataType, } duration := time.Since(startTime) resourceStatusResyncDurationMetric.With(labels).Observe(duration.Seconds()) }
♻️ Duplicate comments (6)
pkg/cloudevents/generic/metrics_collector.go (6)
241-255: Register reconnect metric in client registrar, remove from source registrarThis was previously suggested and remains unaddressed.
func RegisterClientCloudEventsMetrics(register prometheus.Registerer) { register.MustRegister(cloudeventsReceivedByClientCounterMetric) register.MustRegister(cloudeventsSentFromClientCounterMetric) register.MustRegister(resourceStatusResyncDurationMetric) + register.MustRegister(clientReconnectedCounterMetric) register.MustRegister(workProcessedCounterMetric) } func RegisterSourceCloudEventsMetrics(register prometheus.Registerer) { register.MustRegister(cloudeventsReceivedBySourceCounterMetric) register.MustRegister(cloudeventsSentFromSourceCounterMetric) register.MustRegister(resourceSpecResyncDurationMetric) - register.MustRegister(clientReconnectedCounterMetric) }
116-123: Same: unique name for client 'received' countervar cloudeventsReceivedByClientCounterMetric = prometheus.NewCounterVec( prometheus.CounterOpts{ Subsystem: cloudeventsMetricsSubsystem, - Name: receivedCounterMetric, + Name: "received_by_client_total", Help: "The total number of CloudEvents received by client.", }, cloudeventsReceivedByClientMetricsLabels, )
125-136: Same: unique name for source 'sent' countervar cloudeventsSentFromSourceCounterMetric = prometheus.NewCounterVec( prometheus.CounterOpts{ Subsystem: cloudeventsMetricsSubsystem, - Name: sentCounterMetric, + Name: "sent_from_source_total", Help: "The total number of CloudEvents sent from source.", }, cloudeventsSentFromSourceMetricsLabels, )
138-149: Same: unique name for client 'sent' countervar cloudeventsSentFromClientCounterMetric = prometheus.NewCounterVec( prometheus.CounterOpts{ Subsystem: cloudeventsMetricsSubsystem, - Name: sentCounterMetric, + Name: "sent_from_client_total", Help: "The total number of CloudEvents sent from client.", }, cloudeventsSentFromClientMetricsLabels, )
95-109: Make metric names unique to avoid registration panic and align docsBoth received counters share Name="received_total". Use distinct names.
var cloudeventsReceivedBySourceCounterMetric = prometheus.NewCounterVec( prometheus.CounterOpts{ Subsystem: cloudeventsMetricsSubsystem, - Name: receivedCounterMetric, + Name: "received_by_source_total", Help: "The total number of CloudEvents received by source.", }, cloudeventsReceivedBySourceMetricsLabels, )
67-73: Split resync label sets: spec includes consumer; status does notCurrent shared cloudeventsResyncMetricsLabels includes consumer, which conflicts with agent-side (status) metrics. Define separate label arrays and use them for the respective histograms.
-// cloudeventsResyncMetricsLabels - Array of labels added to cloudevents resync metrics: -var cloudeventsResyncMetricsLabels = []string{ - metricsSourceLabel, // source - metricsConsumerLabel, // consumer - metricsDataTypeLabel, // data type, e.g. manifests, manifestbundles -} +// resourceSpecResyncMetricsLabels: source-side (includes consumer) +var resourceSpecResyncMetricsLabels = []string{ + metricsSourceLabel, // source + metricsConsumerLabel, // consumer + metricsDataTypeLabel, // type +} + +// resourceStatusResyncMetricsLabels: client-side (no consumer) +var resourceStatusResyncMetricsLabels = []string{ + metricsSourceLabel, // source + metricsDataTypeLabel, // type +}
🧹 Nitpick comments (3)
pkg/cloudevents/generic/agentclient.go (1)
149-157: Optional: align function naming (Agent vs Client)increaseCloudEventsReceivedByAgentCounter is incrementing the client counter. Consider renaming to increaseCloudEventsReceivedByClientCounter for consistency with metric vars/tests.
pkg/cloudevents/generic/metrics_collector.go (2)
191-201: Fix status resync histogram docs: remove consumer label in examplesAgent-side status resync must not show consumer/cluster.
-// resource_status_resync_duration_seconds_bucket{source="source1",consumer="consumer1",type="io.open-...manifests",le="0.5"} 1 +// resource_status_resync_duration_seconds_bucket{source="source1",type="io.open-...manifests",le="0.5"} 1 -// resource_status_resync_duration_seconds_sum{source="source1",consumer="consumer1",type="io.open-...manifests"} 1.6 +// resource_status_resync_duration_seconds_sum{source="source1",type="io.open-...manifests"} 1.6 -// resource_status_resync_duration_seconds_count{source="source1",consumer="consumer1",type="io.open-...manifests"} 2 +// resource_status_resync_duration_seconds_count{source="source1",type="io.open-...manifests"} 2
95-106: Docs vs code mismatch: fix comment headerComment mentions "received_by_source_total" but Name was "received_total". The Name fix above will resolve this inconsistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
pkg/cloudevents/generic/agentclient.go(3 hunks)pkg/cloudevents/generic/agentclient_test.go(4 hunks)pkg/cloudevents/generic/metrics_collector.go(7 hunks)pkg/cloudevents/generic/metrics_collector_test.go(11 hunks)pkg/cloudevents/generic/sourceclient.go(4 hunks)pkg/cloudevents/generic/sourceclient_test.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/cloudevents/generic/sourceclient.go
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: morvencao
PR: open-cluster-management-io/sdk-go#138
File: pkg/cloudevents/server/grpc/metrics/metrics.go:231-254
Timestamp: 2025-09-01T03:34:05.141Z
Learning: In open-cluster-management.io/sdk-go gRPC CloudEvents metrics, processing duration metrics should only be recorded for unary RPCs, not stream RPCs. Stream RPCs can be long-lived connections that persist as long as the gRPC server runs, making duration metrics confusing and less useful for operators debugging issues.
📚 Learning: 2025-09-01T03:34:05.141Z
Learnt from: morvencao
PR: open-cluster-management-io/sdk-go#138
File: pkg/cloudevents/server/grpc/metrics/metrics.go:231-254
Timestamp: 2025-09-01T03:34:05.141Z
Learning: In open-cluster-management.io/sdk-go gRPC CloudEvents metrics, processing duration metrics should only be recorded for unary RPCs, not stream RPCs. Stream RPCs can be long-lived connections that persist as long as the gRPC server runs, making duration metrics confusing and less useful for operators debugging issues.
Applied to files:
pkg/cloudevents/generic/metrics_collector.go
🧬 Code graph analysis (4)
pkg/cloudevents/generic/sourceclient_test.go (1)
pkg/cloudevents/generic/sourceclient.go (1)
NewCloudEventSourceClient(39-65)
pkg/cloudevents/generic/agentclient.go (1)
pkg/cloudevents/generic/types/types.go (1)
CloudEventsDataType(165-169)
pkg/cloudevents/generic/agentclient_test.go (2)
pkg/cloudevents/generic/agentclient.go (1)
NewCloudEventAgentClient(39-66)pkg/cloudevents/generic/options/fake/fakeoptions.go (1)
NewAgentOptions(17-23)
pkg/cloudevents/generic/metrics_collector_test.go (5)
pkg/cloudevents/generic/metrics_collector.go (2)
ResetSourceCloudEventsMetrics(258-263)ResetClientCloudEventsMetrics(266-271)pkg/cloudevents/generic/sourceclient.go (1)
NewCloudEventSourceClient(39-65)pkg/cloudevents/generic/options/fake/fakeoptions.go (2)
NewAgentOptions(17-23)NewSourceOptions(25-30)pkg/cloudevents/generic/agentclient.go (1)
NewCloudEventAgentClient(39-66)pkg/cloudevents/generic/types/types.go (5)
SubResourceSpec(27-27)ResyncRequestAction(38-38)ResyncResponseAction(41-41)CloudEventsDataType(165-169)SubResourceStatus(30-30)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: unit
- GitHub Check: integration
- GitHub Check: verify
🔇 Additional comments (6)
pkg/cloudevents/generic/sourceclient_test.go (1)
52-52: LGTM: generic type inference is correctCalls compile via inference from lister/codec; no further changes needed.
Also applies to: 111-111, 295-295, 402-402
pkg/cloudevents/generic/agentclient_test.go (1)
63-63: LGTM: constructor calls without explicit type argsInference from lister/codec is sufficient.
Also applies to: 133-133, 287-287, 469-469
pkg/cloudevents/generic/agentclient.go (2)
114-114: LGTM: sent counter now client-scoped (no consumer label)Matches goal: agent-produced metrics drop cluster/consumer.
135-135: LGTM: sent counter (publish path) uses client-scoped labelsArgument order and originalSource handling look correct.
pkg/cloudevents/generic/metrics_collector_test.go (1)
235-242: Spec resync assertions look consistentSource-side metrics include consumer; histogram labels match. No action.
pkg/cloudevents/generic/metrics_collector.go (1)
151-167: Docs OK for spec histogramExamples include consumer; consistent with source-side spec resync. No changes.
| func TestReconnectMetrics(t *testing.T) { | ||
| // reset metrics | ||
| ResetCloudEventsMetrics() | ||
| ResetSourceCloudEventsMetrics() | ||
| ctx, cancel := context.WithCancel(context.Background()) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reset the right registry set for reconnect test
Reconnect metric is client-side. Use client reset to avoid stale samples.
- ResetSourceCloudEventsMetrics()
+ ResetClientCloudEventsMetrics()📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func TestReconnectMetrics(t *testing.T) { | |
| // reset metrics | |
| ResetCloudEventsMetrics() | |
| ResetSourceCloudEventsMetrics() | |
| ctx, cancel := context.WithCancel(context.Background()) | |
| func TestReconnectMetrics(t *testing.T) { | |
| // reset metrics | |
| ResetClientCloudEventsMetrics() | |
| ctx, cancel := context.WithCancel(context.Background()) |
🤖 Prompt for AI Agents
In pkg/cloudevents/generic/metrics_collector_test.go around lines 112 to 115,
the test resets the source metrics but the reconnect metric is client-side;
replace the call to ResetSourceCloudEventsMetrics() with
ResetClientCloudEventsMetrics() (or the correct client metrics reset function)
so the test clears the client registry and avoids stale samples before creating
the client context; keep the rest of the setup intact.
| // ResetSourceCloudEventsMetrics resets all collectors from source | ||
| func ResetSourceCloudEventsMetrics() { | ||
| cloudeventsReceivedBySourceCounterMetric.Reset() | ||
| cloudeventsSentFromSourceCounterMetric.Reset() | ||
| resourceSpecResyncDurationMetric.Reset() | ||
| resourceStatusResyncDurationMetric.Reset() | ||
| clientReconnectedCounterMetric.Reset() | ||
| } | ||
|
|
||
| // ResetClientCloudEventsMetrics resets all collectors from client | ||
| func ResetClientCloudEventsMetrics() { | ||
| cloudeventsReceivedByClientCounterMetric.Reset() | ||
| cloudeventsSentFromClientCounterMetric.Reset() | ||
| resourceStatusResyncDurationMetric.Reset() | ||
| workProcessedCounterMetric.Reset() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reset functions: move reconnect reset to client set
func ResetSourceCloudEventsMetrics() {
cloudeventsReceivedBySourceCounterMetric.Reset()
cloudeventsSentFromSourceCounterMetric.Reset()
resourceSpecResyncDurationMetric.Reset()
- clientReconnectedCounterMetric.Reset()
}
func ResetClientCloudEventsMetrics() {
cloudeventsReceivedByClientCounterMetric.Reset()
cloudeventsSentFromClientCounterMetric.Reset()
resourceStatusResyncDurationMetric.Reset()
+ clientReconnectedCounterMetric.Reset()
workProcessedCounterMetric.Reset()
}📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // ResetSourceCloudEventsMetrics resets all collectors from source | |
| func ResetSourceCloudEventsMetrics() { | |
| cloudeventsReceivedBySourceCounterMetric.Reset() | |
| cloudeventsSentFromSourceCounterMetric.Reset() | |
| resourceSpecResyncDurationMetric.Reset() | |
| resourceStatusResyncDurationMetric.Reset() | |
| clientReconnectedCounterMetric.Reset() | |
| } | |
| // ResetClientCloudEventsMetrics resets all collectors from client | |
| func ResetClientCloudEventsMetrics() { | |
| cloudeventsReceivedByClientCounterMetric.Reset() | |
| cloudeventsSentFromClientCounterMetric.Reset() | |
| resourceStatusResyncDurationMetric.Reset() | |
| workProcessedCounterMetric.Reset() | |
| } | |
| // ResetSourceCloudEventsMetrics resets all collectors from source | |
| func ResetSourceCloudEventsMetrics() { | |
| cloudeventsReceivedBySourceCounterMetric.Reset() | |
| cloudeventsSentFromSourceCounterMetric.Reset() | |
| resourceSpecResyncDurationMetric.Reset() | |
| } | |
| // ResetClientCloudEventsMetrics resets all collectors from client | |
| func ResetClientCloudEventsMetrics() { | |
| cloudeventsReceivedByClientCounterMetric.Reset() | |
| cloudeventsSentFromClientCounterMetric.Reset() | |
| resourceStatusResyncDurationMetric.Reset() | |
| clientReconnectedCounterMetric.Reset() | |
| workProcessedCounterMetric.Reset() | |
| } |
🤖 Prompt for AI Agents
In pkg/cloudevents/generic/metrics_collector.go around lines 257 to 271, the
reconnect metric reset (clientReconnectedCounterMetric.Reset()) is currently
placed in ResetSourceCloudEventsMetrics but belongs to the client group; remove
that Reset() call from ResetSourceCloudEventsMetrics and add
clientReconnectedCounterMetric.Reset() into ResetClientCloudEventsMetrics so all
client-related collectors are reset together.
Signed-off-by: clyang82 <chuyang@redhat.com>
7b35b3c to
6700532
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
pkg/cloudevents/generic/metrics_collector_test.go (2)
115-116: Reset the client registry for reconnect testReconnect is a client-side metric; reset the client registry to avoid stale samples.
Apply:
- ResetSourceCloudEventsMetrics() + ResetClientCloudEventsMetrics()Verify the reset location of the reconnect metric:
#!/bin/bash # Expectation: clientReconnectedCounterMetric.Reset() should live in ResetClientCloudEventsMetrics(). rg -nC2 'func Reset(Source|Client)CloudEventsMetrics' pkg/cloudevents/generic/metrics_collector.go rg -n 'clientReconnectedCounterMetric\.Reset\(' pkg/cloudevents/generic/metrics_collector.go
301-303: Client received counter label set fixed—LGTMMatches the new taxonomy (no cluster label on client-side metrics).
🧹 Nitpick comments (2)
pkg/cloudevents/generic/metrics_collector_test.go (2)
99-101: Replace fixed sleep with an eventually check to de-flakePolling the counters is more robust than a 1s sleep on slower CI.
Apply:
- // wait 1 second for agent receive the resources - time.Sleep(time.Second) + // wait until metrics reflect the published resources (avoid flakiness) + require.Eventually(t, func() bool { + sent := cloudeventsSentFromSourceCounterMetric.WithLabelValues( + c.sourceID, noneOriginalSource, c.clusterName, c.dataType.String(), string(c.subresource), string(c.action), + ) + recv := cloudeventsReceivedByClientCounterMetric.WithLabelValues( + c.sourceID, c.dataType.String(), string(c.subresource), string(c.action), + ) + return int(toFloat64Counter(sent)) == len(c.resources) && + int(toFloat64Counter(recv)) == len(c.resources) + }, 5*time.Second, 10*time.Millisecond)
236-241: Relax the upper bound on resync duration to avoid time-based flakesThe 1s ceiling is brittle under loaded CI; keep >0 and use a looser upper bound.
Apply:
- require.Greater(t, sum, 0.0) - require.Less(t, sum, 1.0) + require.Greater(t, sum, 0.0) + require.Less(t, sum, 5.0)- require.Greater(t, sum, 0.0) - require.Less(t, sum, 1.0) + require.Greater(t, sum, 0.0) + require.Less(t, sum, 5.0)Also applies to: 308-313
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
pkg/cloudevents/generic/agentclient.go(3 hunks)pkg/cloudevents/generic/agentclient_test.go(4 hunks)pkg/cloudevents/generic/metrics_collector.go(7 hunks)pkg/cloudevents/generic/metrics_collector_test.go(11 hunks)pkg/cloudevents/generic/sourceclient.go(4 hunks)pkg/cloudevents/generic/sourceclient_test.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- pkg/cloudevents/generic/sourceclient_test.go
- pkg/cloudevents/generic/sourceclient.go
- pkg/cloudevents/generic/agentclient.go
- pkg/cloudevents/generic/metrics_collector.go
- pkg/cloudevents/generic/agentclient_test.go
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: morvencao
PR: open-cluster-management-io/sdk-go#138
File: pkg/cloudevents/server/grpc/metrics/metrics.go:231-254
Timestamp: 2025-09-01T03:34:05.141Z
Learning: In open-cluster-management.io/sdk-go gRPC CloudEvents metrics, processing duration metrics should only be recorded for unary RPCs, not stream RPCs. Stream RPCs can be long-lived connections that persist as long as the gRPC server runs, making duration metrics confusing and less useful for operators debugging issues.
🧬 Code graph analysis (1)
pkg/cloudevents/generic/metrics_collector_test.go (5)
pkg/cloudevents/generic/metrics_collector.go (2)
ResetSourceCloudEventsMetrics(258-263)ResetClientCloudEventsMetrics(266-271)pkg/cloudevents/generic/sourceclient.go (1)
NewCloudEventSourceClient(39-65)pkg/cloudevents/generic/options/fake/fakeoptions.go (2)
NewAgentOptions(17-23)NewSourceOptions(25-30)pkg/cloudevents/generic/agentclient.go (1)
NewCloudEventAgentClient(39-66)pkg/cloudevents/generic/types/types.go (5)
SubResourceSpec(27-27)ResyncRequestAction(38-38)ResyncResponseAction(41-41)CloudEventsDataType(165-169)SubResourceStatus(30-30)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: integration
- GitHub Check: unit
- GitHub Check: build
- GitHub Check: verify
🔇 Additional comments (4)
pkg/cloudevents/generic/metrics_collector_test.go (4)
63-65: Good: clearing both registries before the test runResetting both source and client registries avoids cross-test leakage.
73-74: Constructor updates look correctNon-generic client constructors align with the refactor; init flow is intact.
Also applies to: 79-80
314-315: Client sent counter label set looks correctNo cluster label; includes agentID, originalSource, type, subresource, action.
308-308: Confirm label cardinality for status resync duration (client-side) — resolvedresourceStatusResyncDurationMetric defines labels "source", "consumer", and "type" (see nearby commented metric lines), so keep c.clusterName in WithLabelValues.
|
/unhold |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: clyang82, qiujian16 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/lgtm |
fae48aa
into
open-cluster-management-io:main
3 participants
Summary
if the metrics are from maestro agent, just remove the
clusterlabel. You can relabel via prometheus or opentelemetry-collector during scrape.if the metrics are from maestro server, change the label from
clustertoconsumer.Related issue(s)
Fixes #. https://issues.redhat.com/browse/ACM-23658
Summary by CodeRabbit