Skip to content

Conversation

RobertIndie
Copy link
Member

Motivation

Fixes #1393

The metric pulsar_client_consumer_acks was not being incremented when using the AckIDList method, which is a bug that affects observability and monitoring of consumer acknowledgment behavior. This issue prevents proper tracking of batch acknowledgment operations.

Modifications

  • Fixed Missing Metric: Added pc.metrics.AcksCounter.Add(float64(len(msgIDs))) in consumer_partition.go to properly track acknowledgments in AckIDList method
  • Added Processing Time Tracking: Implemented processing time observation to measure the time between message receipt and acknowledgment for better performance monitoring
  • Enhanced Test Coverage: Added TestAckIDListWillIncreaseAckCounterMetrics test to verify that the ack counter metrics are properly incremented when using AckIDList
  • Test Infrastructure: Created test_setup_test.go with Prometheus metrics server to support metric testing
  • Script Improvements: Updated test service startup script with better error handling and timing

The fix ensures that the pulsar_client_consumer_acks metric correctly reflects all acknowledgment operations, including batch acknowledgments via AckIDList, providing consistent observability across all consumer acknowledgment methods.

Verifying this change

  • Make sure that the change passes the CI checks.

(Please pick either of the following options)

This change is a trivial rework / code cleanup without any test coverage.

(or)

This change is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(example:)

  • Added integration tests for end-to-end deployment with large payloads (10MB)
  • Extended integration test for recovery after broker failure

Does this pull request potentially affect one of the following parts:

If yes was chosen, please highlight the changes

  • Dependencies (does it add or upgrade a dependency): (yes / no)
  • The public API: (yes / no)
  • The schema: (yes / no / don't know)
  • The default values of configurations: (yes / no)
  • The wire protocol: (yes / no)

Documentation

  • Does this pull request introduce a new feature? (yes / no)
  • If yes, how is the feature documented? (not applicable / docs / GoDocs / not documented)
  • If a feature is not applicable for documentation, explain why?
  • If a feature is not documented yet in this PR, please create a followup issue for adding the documentation

@RobertIndie RobertIndie requested a review from Copilot July 22, 2025 04:23
@RobertIndie RobertIndie self-assigned this Jul 22, 2025
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes a bug where the pulsar_client_consumer_acks metric was not being tracked when using the AckIDList method. The fix ensures consistent metric tracking across all consumer acknowledgment methods for better observability.

  • Added missing metrics tracking for AckIDList method in consumer
  • Enhanced test infrastructure with Prometheus metrics server for metric validation
  • Cleaned up test service startup script by removing unused environment variables

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

File Description
pulsar/consumer_partition.go Added metrics tracking for ack counter and processing time in AckIDList method
pulsar/consumer_test.go Added test to verify AckIDList metrics tracking functionality
pulsar/test_setup_test.go Created test infrastructure with Prometheus metrics server for metric testing
scripts/pulsar-test-service-start.sh Removed unused PULSAR_MEM and PULSAR_STANDALONE_USE_ZOOKEEPER environment variables

@RobertIndie RobertIndie marked this pull request as draft July 22, 2025 05:48
@RobertIndie RobertIndie marked this pull request as ready for review July 22, 2025 10:35
@BewareMyPower BewareMyPower added this to the v0.16.0 milestone Jul 24, 2025
@BewareMyPower BewareMyPower merged commit 0dee113 into apache:master Jul 24, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] The metric pulsar_client_consumer_acks doesn't count for the AckIDList
2 participants