Copilot commented on code in PR #1113:
URL:
https://github.com/apache/skywalking-banyandb/pull/1113#discussion_r3197990211
##########
banyand/metadata/service/server.go:
##########
@@ -192,6 +192,11 @@ func (s *server) SetMetricsRegistry(omr
observability.MetricsRegistry) {
s.Service.SetMetricsRegistry(omr)
}
+// MetricsRegistry returns the metrics registry held by this server or the
embedded client.
+func (s *server) MetricsRegistry() observability.MetricsRegistry {
+ return s.omr
Review Comment:
MetricsRegistry()’s docstring says it returns the registry held by this
server *or the embedded client*, but the implementation only returns s.omr. If
the embedded Service has a registry set (or future changes set it there) while
s.omr is nil, callers will incorrectly see metrics disabled. Consider returning
s.omr when non-nil, otherwise delegating to s.Service.MetricsRegistry() (or
adjust the comment to match the behavior).
##########
banyand/queue/pub/batch.go:
##########
@@ -310,7 +357,11 @@ func (bp *batchPublisher) retrySend(ctx context.Context,
stream clusterv1.Servic
cancel()
if sendErr == nil {
- // Success
+ if bp.hasMetrics() {
+ bp.pub.metrics.sendAttemptsTotal.Inc(1, topic,
node)
+
bp.pub.metrics.sendBytesTotal.Inc(float64(len(r.Body)), topic, node)
+ }
+ // Success writing to the local stream; end-to-end ack
is observed in listenBatchResponse.
Review Comment:
sendAttemptsTotal is only incremented on successful Send, but the metric
name "send_attempts_total" implies it counts all attempts (including failed
attempts). Either increment it per Send invocation (before calling stream.Send)
and add a separate success counter, or rename it to reflect that it counts
successful sends.
##########
banyand/queue/pub/pub.go:
##########
@@ -387,6 +420,18 @@ func (p *pub) PreRun(context.Context) error {
p.log = logger.GetLogger("server-queue-pub-" + p.prefix)
+ if p.metrics == nil {
+ if svc, ok := p.metadata.(metadata.Service); ok {
+ if omr := svc.MetricsRegistry(); omr != nil {
+ p.metrics =
newPubMetrics(omr.With(queuePubScope))
+ } else {
+ p.log.Warn().Msg("queue_pub metrics disabled:
MetricsRegistry returned nil")
+ }
+ } else {
+ p.log.Warn().Msg("queue_pub metrics disabled: metadata
does not implement metadata.Service")
Review Comment:
When p.metadata is nil (e.g., NewWithoutMetadata()), this logs a warning
that metadata does not implement metadata.Service, which is misleading and can
add noise at startup. Consider explicitly handling p.metadata == nil (skip
logging or log a more accurate message) and only warning when a non-nil
metadata repo is provided but lacks metrics support.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
