xiangfu0 commented on code in PR #19872:
URL: https://github.com/apache/pinot/pull/19872#discussion_r4278319904
##########
docker/images/pinot/etc/jmx_prometheus_javaagent/configs-dropwizard/server.yml:
##########
@@ -0,0 +1,182 @@
+rules:
+# Dropwizard gauges expose both Value and Number; retain the existing Value
series only.
+- pattern: "\"?org\\.apache\\.pinot\\.common\\.metrics\"?<name=.*,
type=\"?gauges\"?><>Number"
+ name: ""
+# OPEN_STRUCT observability. These MUST precede the generic gauge/meter rules
below: the column segment
+# would otherwise be consumed by the "tableNameWithType + partitionId" rule
and exported as partition="<column>",
+# and the column-level meters by the "rawTableName" rule as
table="<table>.<column>".
+# Per-key gauge. The key is a raw user-supplied JSON key, so it is captured
last with a greedy ([^"]+) — it may
+# contain '$', '.', '-' or spaces, all of which are legal in a Prometheus
label value. The closing quote is
+# excluded because the trailing "? is optional and a greedy (.+) would
otherwise swallow it. The column name
+# cannot contain '.' or '$', so ([^.$"]+) splits the two unambiguously. The
sparse column renders as key="__sparse__".
+- pattern:
"\"?org\\.apache\\.pinot\\.common\\.metrics\"?<name=\"?pinot\\.server\\.(openStructLastSegmentKeyDocCount)\\.(([^.]+)\\.)?([^.]*)_(OFFLINE|REALTIME)\\.([^.$\"]+)\\$([^\"]+)\"?,
type=\"?(?:gauges|meters|timers|counters|histograms)\"?><>(\\w+)"
+ name: "pinot_server_$1_$8"
+ cache: true
+ labels:
+ database: "$3"
+ table: "$2$4"
+ tableType: "$5"
+ column: "$6"
+ key: "$7"
+# Column-level OPEN_STRUCT gauges.
+- pattern:
"\"?org\\.apache\\.pinot\\.common\\.metrics\"?<name=\"?pinot\\.server\\.(openStructLastSegmentDenseKeyCount|openStructLastSegmentSparseKeyCount|openStructLastSegmentKeyCount|openStructLastSegmentDocCount)\\.(([^.]+)\\.)?([^.]*)_(OFFLINE|REALTIME)\\.([^.\"]+)\"?,
type=\"?(?:gauges|meters|timers|counters|histograms)\"?><>(\\w+)"
+ name: "pinot_server_$1_$7"
+ cache: true
+ labels:
+ database: "$3"
+ table: "$2$4"
+ tableType: "$5"
+ column: "$6"
+# Column-level OPEN_STRUCT meters. Meter names put the metric name last,
unlike gauges.
+- pattern:
"\"?org\\.apache\\.pinot\\.common\\.metrics\"?<name=\"?pinot\\.server\\.(([^.]+)\\.)?([^.]*)_(OFFLINE|REALTIME)\\.([^.]+)\\.(openStructTypeCoercionFailures|openStructTypeInferenceFailures)\"?,
type=\"?(?:gauges|meters|timers|counters|histograms)\"?><>(\\w+)"
+ name: "pinot_server_$6_$7"
+ cache: true
+ labels:
+ database: "$2"
+ table: "$1$3"
+ tableType: "$4"
+ column: "$5"
+# Gauges that accept tableNameWithType
+- pattern:
"\"?org\\.apache\\.pinot\\.common\\.metrics\"?<name=\"?pinot\\.(\\w+)\\.(\\w+)\\.((\\w+)\\.)?(\\w+)_(OFFLINE|REALTIME)\\\"?,
type=\"?\\w+\"?><>(\\w+)"
Review Comment:
Minor/consistency: a few server rules use `type="?\w+"?` here while the rest
of this file (and all of broker/controller/minion) use the explicit
`(?:gauges|meters|timers|counters|histograms)` alternation. No behavioral
difference today since `\w+` still matches all five categories — just tidier to
make them consistent.
##########
pinot-common/src/test/java/org/apache/pinot/common/metrics/prometheus/PinotPrometheusMetricsTest.java:
##########
@@ -95,6 +95,7 @@ public void setupTest() {
_pinotMetricsFactory = getPinotMetricsFactory();
pinotConfiguration.setProperty(CONFIG_OF_METRICS_FACTORY_CLASS_NAME,
_pinotMetricsFactory.getClass().getCanonicalName());
+ _pinotMetricsFactory.init(pinotConfiguration);
Review Comment:
Good catch adding this. Worth noting for reviewers why it's needed: the
test's own `_pinotMetricsFactory` instance is separate from the one
`PinotMetricUtils.init` creates via reflection, so without this `init()` its
`_domainName` stays null and `makePinotJmxReporter` would emit under a null
domain — the exporter regex keys on `org.apache.pinot.common.metrics`, so the
assertions would be vacuous. Legit fix.
##########
docker/images/pinot/etc/jmx_prometheus_javaagent/configs-dropwizard/server.yml:
##########
@@ -0,0 +1,182 @@
+rules:
+# Dropwizard gauges expose both Value and Number; retain the existing Value
series only.
+- pattern: "\"?org\\.apache\\.pinot\\.common\\.metrics\"?<name=.*,
type=\"?gauges\"?><>Number"
Review Comment:
Minor: this Number-suppression rule is missing `cache: true`, while the
equivalent rule in broker.yml, controller.yml, and minion.yml all have it. The
suppression itself works fine — this just means it's re-evaluated per scrape
for every numeric gauge MBean. Worth adding `cache: true` for parity and a
small scrape-time win.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
