michaeljmarshall commented on code in PR #18451:
URL: https://github.com/apache/pulsar/pull/18451#discussion_r1024425379
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/stats/prometheus/PrometheusMetricStreams.java:
##########
@@ -65,11 +65,19 @@ void releaseAll() {
metricStreamMap.clear();
}
- private SimpleTextOutputStream initGaugeType(String metricName) {
+ private SimpleTextOutputStream initMetricType(String metricName) {
+ String metricTypeDef = String.format("# TYPE %s %s\n", metricName,
metricType(metricName));
Review Comment:
Nit: this might have unintended consequences for performance. With this
change, we're creating the string for every call to this method instead of only
when the metric has not yet been initialized. I'm not sure how much it matters
in this section, but I know the prometheus metrics generation has been
optimized because it was generating unnecessary load.
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/stats/prometheus/PrometheusMetricStreams.java:
##########
@@ -65,11 +65,19 @@ void releaseAll() {
metricStreamMap.clear();
}
- private SimpleTextOutputStream initGaugeType(String metricName) {
+ private SimpleTextOutputStream initMetricType(String metricName) {
+ String metricTypeDef = String.format("# TYPE %s %s\n", metricName,
metricType(metricName));
return metricStreamMap.computeIfAbsent(metricName, s -> {
SimpleTextOutputStream stream = new
SimpleTextOutputStream(PulsarByteBufAllocator.DEFAULT.directBuffer());
- stream.write("# TYPE ").write(metricName).write(" gauge\n");
+ stream.write(metricTypeDef);
return stream;
});
}
+
+ private String metricType(String metricName) {
+ if (metricName.endsWith("_total")) {
+ return "counter";
+ }
+ return "gauge";
+ }
Review Comment:
I wonder if this change should be a separate, larger PR. I agree that we
need to fix the `TYPE` annotations so that things are not all marked as
`gauge`, but it seems like a separate issue than fixing the bytes metric. (The
main reason I think we should split it out is so we can create an optimized
solution. I am wondering if it would be better to create an enum, or use
Prometheus's enums, and have the caller indicate the metric type.) Also, is
there any way changing this annotation will "break" user integration? If so,
we'll want to make sure we correctly notify users that this bug fix will change
the behavior.
--
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]
