Skip to content

[improve][client] Client add permits metric#22390

Open
crossoverJie wants to merge 5 commits intoapache:masterfrom
crossoverJie:client-metrics-permits
Open

[improve][client] Client add permits metric#22390
crossoverJie wants to merge 5 commits intoapache:masterfrom
crossoverJie:client-metrics-permits

Conversation

@crossoverJie
Copy link
Member

Motivation

Added a new dimensional metric to help better monitor clients.

Modifications

Add pulsar.client.consumer.available_permits.count metric.

Verifying this change

  • Make sure that the change passes the CI checks.

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

If the box was checked, please highlight the changes

  • Dependencies (add or upgrade a dependency)
  • The public API
  • The schema
  • The default values of configurations
  • The threading model
  • The binary protocol
  • The REST endpoints
  • The admin CLI options
  • The metrics
  • Anything that affects deployment

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

Matching PR in forked repository

PR in forked repository: crossoverJie#23

@github-actions github-actions bot added the doc-required Your PR changes impact docs and you will update later. label Apr 1, 2024
@crossoverJie
Copy link
Member Author

@merlimat @asafm PTAL.

"The number of messages currently sitting in the consumer receive queue", topic, attrs);
bytesPrefetchedGauge = ip.newUpDownCounter("pulsar.client.consumer.receive_queue.size", Unit.Bytes,
"The total size in bytes of messages currently sitting in the consumer receive queue", topic, attrs);
messageAvailablePermitsGauge = ip.newUpDownCounter("pulsar.client.consumer.available_permits.count",
Copy link
Contributor

Choose a reason for hiding this comment

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

If you copied from above you need to notice .count was added since there are two sub-metrics for receive_queue. In yours you only have one.
Also, thinking about available. Maybe if we know the limit:

pulsar.client.consumer.permit.remaining
pulsar.client.consumer.permit.limit

if we don't know the limit then maybe just ``pulsar.client.consumer.permits.remaining`

See naming guidelines here https://opentelemetry.io/docs/specs/semconv/general/metrics/

Copy link
Member Author

Choose a reason for hiding this comment

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

If you copied from above you need to notice .count was added since there are two sub-metrics for receive_queue. In yours you only have one.

I noticed that .size is the data of the message body, but permits is just a counter and does not have a size itself, so .size may not be needed.


Also, thinking about available. Maybe if we know the limit:

pulsar.client.consumer.permit.remaining pulsar.client.consumer.permit.limit

pulsar.client.consumer.permit.remaining= getCurrentReceiverQueueSize() / 2- available

pulsar.client.consumer.permit.limit= getCurrentReceiverQueueSize() / 2

Do you mean this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes.

Also I didn't write that size is needed :) Only that we don't need .count and added suggestion how to name it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you for your follow-up, the changes have been made.

PTAL.

@crossoverJie crossoverJie requested a review from asafm April 23, 2024 06:01
# Conflicts:
#	pulsar-client/src/main/java/org/apache/pulsar/client/impl/metrics/UpDownCounter.java
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

doc-required Your PR changes impact docs and you will update later.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants