asafm commented on code in PR #22058:
URL: https://github.com/apache/pulsar/pull/22058#discussion_r1510290496
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java:
##########
@@ -403,15 +411,41 @@ public BrokerService(PulsarService pulsar, EventLoopGroup
eventLoopGroup) throws
this.defaultServerBootstrap = defaultServerBootstrap();
this.pendingLookupRequests =
ObserverGauge.build("pulsar_broker_lookup_pending_requests", "-")
- .supplier(() ->
pulsar.getConfig().getMaxConcurrentLookupRequest()
- - lookupRequestSemaphore.get().availablePermits())
+ .supplier(this::getPendingLookupRequest)
.register();
+ this.pendingLookupRequestsCounter =
pulsar.getOpenTelemetry().getMeter()
+ .gaugeBuilder("pulsar.broker.lookup.pending.request.usage")
+ .ofLongs()
+ .setDescription("The number of pending lookup requests in the
broker. "
+ + "When it reaches threshold
\"maxConcurrentLookupRequest\" defined in broker.conf, "
+ + "new requests are rejected.")
+ .buildWithCallback(measurement ->
measurement.record(getPendingLookupRequest()));
+ this.pendingLookupRequestsLimit = pulsar.getOpenTelemetry().getMeter()
+ .gaugeBuilder("pulsar.broker.lookup.pending.request.limit")
+ .ofLongs()
+ .setDescription("The maximum number of pending lookup requests
in the broker. "
+ + "Equal to \"maxConcurrentLookupRequest\" defined in
broker.conf.")
+ .buildWithCallback(
+ measurement ->
measurement.record(pulsar.getConfig().getMaxConcurrentLookupRequest()));
this.pendingTopicLoadRequests = ObserverGauge.build(
- "pulsar_broker_topic_load_pending_requests", "-")
- .supplier(() ->
pulsar.getConfig().getMaxConcurrentTopicLoadRequest()
- - topicLoadRequestSemaphore.get().availablePermits())
+ "pulsar_broker_topic_load_pending_requests", "-")
+ .supplier(this::getPendingTopicLoadRequests)
.register();
+ this.pendingTopicLoadRequestsCounter =
pulsar.getOpenTelemetry().getMeter()
+ .gaugeBuilder("pulsar.broker.topic.load.pending.request.usage")
Review Comment:
@codelipenghui Couple of questions here:
1. Namespace matters. We try to avoid writing in "English": Topic concurrent
load --> `pulsar.broker.topic.concurrent.load"...
Namespace (prefix) means we might have other metrics under it. I don't
see any other metrics for `topic.concurrent`. Hence to me it makes more sense
to have `pulsar.broker.topic.load.concurrent` and then `usage` and `limit`,
where usage is the current number of concurrent load operations, and limit is
the defined limit.
2. Regarding the word `operation` suggested. Let's discuss this.
Lookup is a request right? It's coming from the user in `/lookup` or in
the `lookup` command in the binary protocol. Yet, @dragosvictor mentioned that
lookup is actually called from within other requests like get topic of
namespaces, get partition metadata, etc. Perhaps the correct thing here as
@dragosvictor said is that lookup we measure is actually an operation.
Same with lopic load. It is not happening due to a request, but an
internal decision, hence it is an operation.
So perhaps the names should actually be:
- `pulsar.broker.topic.operations.lookup.concurrent.usage`
- `pulsar.broker.topic.operations.lookup.concurrent.limit`
- `pulsar.broker.topic.operations.load.concurrent.usage`
- `pulsar.broker.topic.operations.load.concurrent.usage`
@lhotari @codelipenghui Would love your opinion about this
--
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]
