heesung-sn commented on code in PR #19440:
URL: https://github.com/apache/pulsar/pull/19440#discussion_r1101841211
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/data/BrokerLoadData.java:
##########
@@ -187,4 +191,35 @@ public String toString(ServiceConfiguration conf) {
);
}
+ public List<Metrics> toMetrics(String advertisedBrokerAddress) {
+ var metrics = new ArrayList<Metrics>();
+ var dimensions = new HashMap<String, String>();
+ dimensions.put("metric", "loadBalancing");
+ dimensions.put("broker", advertisedBrokerAddress);
Review Comment:
> What I want to express is that the instance label is similar to the broker
dim.
Do you mean these metrics from this PR should not contain the `broker` label
because it is redundant?
> Operators can find the broker-id from the pod name too. For now, I don't
see a particular reason we have to emit brokerUrl for all metrics.
Yes, as I expressed above, this `broker` label might be redundant, as the
metrics' source brokers can be identified by other labels such as `pod-name`
and `instance` labels.
> This is for the backward compatibility as the existing resource usage
metrics have the `broker` label.
However, as I said above, some existing pulsar metrics explicitly emit this
`broker` label, and I kept them here for the backward compatibility.
Since I already added the `broker` label for all metrics from this PR. At
least we are consistent. Please let me know if we want to remove `broker` label
in this PR or not. IMHO, this additional `broker` label does not look harmful,
providing the `advertisedAddress` info.
--
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]
