asafm commented on code in PR #22058:
URL: https://github.com/apache/pulsar/pull/22058#discussion_r1511099159
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/namespace/NamespaceService.java:
##########
@@ -175,6 +187,25 @@ public NamespaceService(PulsarService pulsar) {
this.bundleSplitListeners = new CopyOnWriteArrayList<>();
this.localBrokerDataCache =
pulsar.getLocalMetadataStore().getMetadataCache(LocalBrokerData.class);
this.redirectManager = new RedirectManager(pulsar);
+
+ var meter = pulsar.getOpenTelemetry().getMeter();
+ this.lookupRedirectsCounter = meter
+ .counterBuilder("pulsar.broker.lookup.redirect")
+ .setDescription("The number of lookup redirected requests")
+ .build();
+ this.lookupFailuresCounter = meter
+ .counterBuilder("pulsar.broker.lookup.failure")
+ .setDescription("The number of lookup failures")
+ .build();
+ this.lookupAnswersCounter = meter
+ .counterBuilder("pulsar.broker.lookup.answer")
+ .setDescription("The number of lookup responses (i.e. not
redirected requests)")
+ .build();
+ this.lookupLatencyHistogram = meter
+ .histogramBuilder("pulsar.broker.lookup.latency")
Review Comment:
Ok, after a good discussion with @codelipenghui, with came up with the
following instrument names:
```
pulsar.broker.topic.load.duration
pulsar.broker.topic.load.concurrent.usage
pulsar.broker.topic.load.concurrent.limit
pulsar.broker.request.topic.lookup.duration
pulsar.broker.request.topic.lookup.concurrent.usage
pulsar.broker.request.topic.lookup.concurrent.limit
```
Why?
1. The 3rd name is usually a category. For example, messaging, transaction,
offloader, etc. After some discussion, we figured "request" could be a decent
category for requests like lookup.
* The code that uses the lookup semaphore needs to be corrected. The only
ones who should be using it are the actual lookup HTTP request and the binary
command lookup. The rest are wrong, and they should be removed.
2. Internal operations such as topic load can be implicitly assumed, so
we've removed them from the instrument naming.
Regarding the attributes, we were thinking of uniting the two attributes
into one:
```
Attributes:
pulsar.lookup.response = broker, redirect, failure
```
The reasoning was that, like the HTTP status code, pulsar lookup response
(code) can signify different responses. We can revisit that as we advance to
other requests in the future - we have marked all of this experimental.
@dragosvictor @codelipenghui @lhotari
--
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]
