zhtaoxiang commented on code in PR #12739:
URL: https://github.com/apache/pinot/pull/12739#discussion_r1543315070
##########
pinot-common/src/main/java/org/apache/pinot/common/metrics/AbstractMetrics.java:
##########
@@ -796,6 +809,7 @@ private void removeGaugeFromMetricRegistry(String
metricName) {
protected abstract G[] getGauges();
protected String getTableName(String tableName) {
+ tableName = StringUtils.replaceOnce(tableName, ".", "#");
Review Comment:
I think if we replace the first `.` in the `tableName`, all table names in
`_allowedTables` should also be updated
##########
docker/images/pinot/etc/jmx_prometheus_javaagent/configs/server.yml:
##########
@@ -159,42 +175,47 @@ rules:
## In case a metric does not fit the catch-all patterns, add them before
this comment
# This is a catch-all pattern for pinot table metrics with offline/realtime
suffix without the topic
# Patterns after this line may be skipped.
-- pattern: "\"?org\\.apache\\.pinot\\.common\\.metrics\"?<type=\"?\\w+\"?,
name=\"?pinot\\.(\\w+)\\.(\\w+)_(OFFLINE|REALTIME)\\.(\\w+)\"?><>(\\w+)"
- name: "pinot_$1_$4_$5"
+- pattern: "\"?org\\.apache\\.pinot\\.common\\.metrics\"?<type=\"?\\w+\"?,
name=\"?pinot\\.(\\w+)\\.((\\w+)#)*(\\w+)_(OFFLINE|REALTIME)\\.(\\w+)\"?><>(\\w+)"
+ name: "pinot_$1_$6_$7"
cache: true
labels:
- table: "$2"
- tableType: "$3"
+ database: "$3"
+ table: "$4"
+ tableType: "$5"
#when there is partition but no topic in the metric
-- pattern: "\"?org\\.apache\\.pinot\\.common\\.metrics\"?<type=\"?\\w+\"?,
name=\"?pinot\\.(\\w+)\\.(\\w+)\\.(\\w+)_(OFFLINE|REALTIME)\\.(\\w+)\"?><>(\\w+)"
- name: "pinot_$1_$2_$6"
+- pattern: "\"?org\\.apache\\.pinot\\.common\\.metrics\"?<type=\"?\\w+\"?,
name=\"?pinot\\.(\\w+)\\.(\\w+)\\.((\\w+)#)*(\\w+)_(OFFLINE|REALTIME)\\.(\\w+)\"?><>(\\w+)"
+ name: "pinot_$1_$2_$8"
cache: true
labels:
- table: "$3"
- tableType: "$4"
- partition: "$5"
+ database: "$4"
+ table: "$5"
+ tableType: "$6"
+ partition: "$7"
#when there is partition and topic in the metric
-- pattern: "\"?org\\.apache\\.pinot\\.common\\.metrics\"?<type=\"?\\w+\"?,
name=\"?pinot\\.(\\w+)\\.(\\w+)\\.(\\w+)_(OFFLINE|REALTIME)\\-(.+)\\-(\\w+)\"?><>(\\w+)"
- name: "pinot_$1_$2_$7"
+- pattern: "\"?org\\.apache\\.pinot\\.common\\.metrics\"?<type=\"?\\w+\"?,
name=\"?pinot\\.(\\w+)\\.(\\w+)\\.((\\w+)#)*(\\w+)_(OFFLINE|REALTIME)\\-(.+)\\-(\\w+)\"?><>(\\w+)"
+ name: "pinot_$1_$2_$9"
cache: true
labels:
- table: "$3"
- tableType: "$4"
- topic: "$5"
- partition: "$6"
+ database: "$4"
+ table: "$5"
+ tableType: "$6"
+ topic: "$7"
+ partition: "$8"
#when there is no partition in the metric
-- pattern: "\"?org\\.apache\\.pinot\\.common\\.metrics\"?<type=\"?\\w+\"?,
name=\"?pinot\\.(\\w+)\\.(\\w+)\\.(\\w+)_(OFFLINE|REALTIME)\\\"?><>(\\w+)"
- name: "pinot_$1_$2_$5"
+- pattern: "\"?org\\.apache\\.pinot\\.common\\.metrics\"?<type=\"?\\w+\"?,
name=\"?pinot\\.(\\w+)\\.(\\w+)\\.((\\w+)#)*(\\w+)_(OFFLINE|REALTIME)\\\"?><>(\\w+)"
+ name: "pinot_$1_$2_$7"
cache: true
labels:
- table: "$3"
- tableType: "$4"
+ database: "$4"
+ table: "$5"
+ tableType: "$6"
# This is a catch-all pattern for pinot table metrics. Patterns after this
line may be skipped.
-- pattern: "\"?org\\.apache\\.pinot\\.common\\.metrics\"?<type=\"?\\w+\"?,
name=\"?pinot\\.(\\w+)\\.(\\w+)\\.(\\w+)\"?><>(\\w+)"
- name: "pinot_$1_$3_$4"
+- pattern: "\"?org\\.apache\\.pinot\\.common\\.metrics\"?<type=\"?\\w+\"?,
name=\"?pinot\\.(\\w+)\\.((\\w+)#)*(\\w+)\\.(\\w+)\"?><>(\\w+)"
+ name: "pinot_$1_$5_$6"
cache: true
labels:
- table: "$2"
+ database: "$3"
+ table: "$4"
# This is a catch-all pattern for pinot controller metrics not related to
tables. Patterns after this line may be skipped.
- pattern: "\"?org\\.apache\\.pinot\\.common\\.metrics\"?<type=\"?\\w+\"?,
name=\"?pinot\\.(\\w+)\\.(\\w+)\"?><>(\\w+)"
Review Comment:
and here
##########
pinot-common/src/main/java/org/apache/pinot/common/metrics/ValidationMetrics.java:
##########
@@ -241,7 +242,7 @@ public void cleanupSegmentCountGauge(final String resource)
{
@VisibleForTesting
public static String makeGaugeName(final String resource, final String
gaugeName) {
- return "pinot.controller." + resource + "." + gaugeName;
+ return "pinot.controller." + StringUtils.replaceOnce(resource, ".", "#") +
"." + gaugeName;
Review Comment:
Could you please help me understand:
1. Why do we need to replace the first `.`? Is it because the first part
before the first `.` is the database name? Is this assumption true?
2. If we need to make the change for gauge, how about meter and timer?
##########
docker/images/pinot/etc/jmx_prometheus_javaagent/configs/minion.yml:
##########
@@ -36,18 +38,20 @@ rules:
# This is a catch-all pattern for pinot table metrics with offline/realtime
suffix.
# Patterns after this line may be skipped.
-- pattern: "\"?org\\.apache\\.pinot\\.common\\.metrics\"?<type=\"?\\w+\"?,
name=\"?pinot\\.(\\w+)\\.(\\w+)_(OFFLINE|REALTIME)\\.(\\w+)\"?><>(\\w+)"
- name: "pinot_$1_$4_$5"
+- pattern: "\"?org\\.apache\\.pinot\\.common\\.metrics\"?<type=\"?\\w+\"?,
name=\"?pinot\\.(\\w+)\\.((\\w+)#)*(\\w+)_(OFFLINE|REALTIME)\\.(\\w+)\"?><>(\\w+)"
+ name: "pinot_$1_$6_$7"
cache: true
labels:
- table: "$2"
- tableType: "$3"
+ database: "$3"
+ table: "$4"
+ tableType: "$5"
# This is a catch-all pattern for pinot table metrics. Patterns after this
line may be skipped.
-- pattern: "\"?org\\.apache\\.pinot\\.common\\.metrics\"?<type=\"?\\w+\"?,
name=\"?pinot\\.(\\w+)\\.(\\w+)\\.(\\w+)\"?><>(\\w+)"
- name: "pinot_$1_$3_$4"
+- pattern: "\"?org\\.apache\\.pinot\\.common\\.metrics\"?<type=\"?\\w+\"?,
name=\"?pinot\\.(\\w+)\\.((\\w+)#)*(\\w+)\\.(\\w+)\"?><>(\\w+)"
+ name: "pinot_$1_$5_$6"
cache: true
labels:
- table: "$2"
+ database: "$3"
+ table: "$4"
# This is a catch-all pattern for pinot controller metrics not related to
tables. Patterns after this line may be skipped.
- pattern: "\"?org\\.apache\\.pinot\\.common\\.metrics\"?<type=\"?\\w+\"?,
name=\"?pinot\\.(\\w+)\\.(\\w+)\"?><>(\\w+)"
Review Comment:
same here.
##########
docker/images/pinot/etc/jmx_prometheus_javaagent/configs/controller.yml:
##########
@@ -209,41 +235,46 @@ rules:
## In case a metric does not fit the catch-all patterns, add them before
this comment
# This is a catch-all pattern for pinot table metrics with offline/realtime
suffix without kafka topic
# Patterns after this line may be skipped.
-- pattern: "\"?org\\.apache\\.pinot\\.common\\.metrics\"?<type=\"?\\w+\"?,
name=\"?pinot\\.(\\w+)\\.(\\w+)_(OFFLINE|REALTIME)\\.(\\w+)\"?><>(\\w+)"
- name: "pinot_$1_$4_$5"
+- pattern: "\"?org\\.apache\\.pinot\\.common\\.metrics\"?<type=\"?\\w+\"?,
name=\"?pinot\\.(\\w+)\\.((\\w+)#)*(\\w+)_(OFFLINE|REALTIME)\\.(\\w+)\"?><>(\\w+)"
+ name: "pinot_$1_$6_$7"
cache: true
labels:
- table: "$2"
- tableType: "$3"
-- pattern: "\"?org\\.apache\\.pinot\\.common\\.metrics\"?<type=\"?\\w+\"?,
name=\"?pinot\\.(\\w+)\\.(\\w+)\\.(\\w+)_(OFFLINE|REALTIME)\\.(\\w+)\"?><>(\\w+)"
- name: "pinot_$1_$2_$6"
+ database: "$3"
+ table: "$4"
+ tableType: "$5"
+- pattern: "\"?org\\.apache\\.pinot\\.common\\.metrics\"?<type=\"?\\w+\"?,
name=\"?pinot\\.(\\w+)\\.(\\w+)\\.((\\w+)#)*(\\w+)_(OFFLINE|REALTIME)\\.(\\w+)\"?><>(\\w+)"
+ name: "pinot_$1_$2_$8"
cache: true
labels:
- table: "$3"
- tableType: "$4"
- partition: "$5"
+ database: "$4"
+ table: "$5"
+ tableType: "$6"
+ partition: "$7"
#This is a catch-all pattern for pinot table metrics with offline/realtime
suffix that also contain kafka topic
-- pattern: "\"?org\\.apache\\.pinot\\.common\\.metrics\"?<type=\"?\\w+\"?,
name=\"?pinot\\.(\\w+)\\.(\\w+)\\.(\\w+)_(OFFLINE|REALTIME)\\-(.+)\\-(\\w+)\"?><>(\\w+)"
- name: "pinot_$1_$2_$7"
+- pattern: "\"?org\\.apache\\.pinot\\.common\\.metrics\"?<type=\"?\\w+\"?,
name=\"?pinot\\.(\\w+)\\.(\\w+)\\.((\\w+)#)*(\\w+)_(OFFLINE|REALTIME)\\-(.+)\\-(\\w+)\"?><>(\\w+)"
+ name: "pinot_$1_$2_$9"
cache: true
labels:
- table: "$3"
- tableType: "$4"
- topic: "$5"
- partition: "$6"
+ database: "$4"
+ table: "$5"
+ tableType: "$6"
+ topic: "$7"
+ partition: "$8"
#when there is no partition in the metric
-- pattern: "\"?org\\.apache\\.pinot\\.common\\.metrics\"?<type=\"?\\w+\"?,
name=\"?pinot\\.(\\w+)\\.(\\w+)\\.(\\w+)_(OFFLINE|REALTIME)\\\"?><>(\\w+)"
- name: "pinot_$1_$2_$5"
+- pattern: "\"?org\\.apache\\.pinot\\.common\\.metrics\"?<type=\"?\\w+\"?,
name=\"?pinot\\.(\\w+)\\.(\\w+)\\.((\\w+)#)*(\\w+)_(OFFLINE|REALTIME)\\\"?><>(\\w+)"
+ name: "pinot_$1_$2_$7"
cache: true
labels:
- table: "$3"
- tableType: "$4"
+ database: "$4"
+ table: "$5"
+ tableType: "$6"
# This is a catch-all pattern for pinot table metrics. Patterns after this
line may be skipped.
-- pattern: "\"?org\\.apache\\.pinot\\.common\\.metrics\"?<type=\"?\\w+\"?,
name=\"?pinot\\.(\\w+)\\.(\\w+)\\.(\\w+)\"?><>(\\w+)"
- name: "pinot_$1_$3_$4"
+- pattern: "\"?org\\.apache\\.pinot\\.common\\.metrics\"?<type=\"?\\w+\"?,
name=\"?pinot\\.(\\w+)\\.((\\w+)#)*(\\w+)\\.(\\w+)\"?><>(\\w+)"
+ name: "pinot_$1_$5_$6"
cache: true
labels:
- table: "$2"
+ database: "$3"
+ table: "$4"
# This is a catch-all pattern for pinot controller metrics not related to
tables. Patterns after this line may be skipped.
- pattern: "\"?org\\.apache\\.pinot\\.common\\.metrics\"?<type=\"?\\w+\"?,
name=\"?pinot\\.(\\w+)\\.(\\w+)\"?><>(\\w+)"
Review Comment:
similarly, do we need to update this rule?
##########
docker/images/pinot/etc/jmx_prometheus_javaagent/configs/broker.yml:
##########
@@ -159,41 +183,46 @@ rules:
## In case a metric does not fit the catch-all patterns, add them before
this comment
# This is a catch-all pattern for pinot table metrics with offline/realtime
suffix without kafka topic
# Patterns after this line may be skipped.
-- pattern: "\"?org\\.apache\\.pinot\\.common\\.metrics\"?<type=\"?\\w+\"?,
name=\"?pinot\\.(\\w+)\\.(\\w+)_(OFFLINE|REALTIME)\\.(\\w+)\"?><>(\\w+)"
- name: "pinot_$1_$4_$5"
+- pattern: "\"?org\\.apache\\.pinot\\.common\\.metrics\"?<type=\"?\\w+\"?,
name=\"?pinot\\.(\\w+)\\.((\\w+)#)*(\\w+)_(OFFLINE|REALTIME)\\.(\\w+)\"?><>(\\w+)"
+ name: "pinot_$1_$6_$7"
cache: true
labels:
- table: "$2"
- tableType: "$3"
-- pattern: "\"?org\\.apache\\.pinot\\.common\\.metrics\"?<type=\"?\\w+\"?,
name=\"?pinot\\.(\\w+)\\.(\\w+)\\.(\\w+)_(OFFLINE|REALTIME)\\.(\\w+)\"?><>(\\w+)"
- name: "pinot_$1_$2_$6"
+ database: "$3"
+ table: "$4"
+ tableType: "$5"
+- pattern: "\"?org\\.apache\\.pinot\\.common\\.metrics\"?<type=\"?\\w+\"?,
name=\"?pinot\\.(\\w+)\\.(\\w+)\\.((\\w+)#)*(\\w+)_(OFFLINE|REALTIME)\\.(\\w+)\"?><>(\\w+)"
+ name: "pinot_$1_$2_$8"
cache: true
labels:
- table: "$3"
- tableType: "$4"
- partition: "$5"
+ database: "$4"
+ table: "$5"
+ tableType: "$6"
+ partition: "$7"
#when there is no partition in the metric
-- pattern: "\"?org\\.apache\\.pinot\\.common\\.metrics\"?<type=\"?\\w+\"?,
name=\"?pinot\\.(\\w+)\\.(\\w+)\\.(\\w+)_(OFFLINE|REALTIME)\\\"?><>(\\w+)"
- name: "pinot_$1_$2_$5"
+- pattern: "\"?org\\.apache\\.pinot\\.common\\.metrics\"?<type=\"?\\w+\"?,
name=\"?pinot\\.(\\w+)\\.(\\w+)\\.((\\w+)#)*(\\w+)_(OFFLINE|REALTIME)\\\"?><>(\\w+)"
+ name: "pinot_$1_$2_$7"
cache: true
labels:
- table: "$3"
- tableType: "$4"
+ database: "$4"
+ table: "$5"
+ tableType: "$6"
#This is a catch-all pattern for pinot table metrics with offline/realtime
suffix that also contain kafka topic
-- pattern: "\"?org\\.apache\\.pinot\\.common\\.metrics\"?<type=\"?\\w+\"?,
name=\"?pinot\\.(\\w+)\\.(\\w+)\\.(\\w+)_(OFFLINE|REALTIME)\\-(.+)\\-(\\w+)\"?><>(\\w+)"
- name: "pinot_$1_$2_$7"
+- pattern: "\"?org\\.apache\\.pinot\\.common\\.metrics\"?<type=\"?\\w+\"?,
name=\"?pinot\\.(\\w+)\\.(\\w+)\\.((\\w+)#)*(\\w+)_(OFFLINE|REALTIME)\\-(.+)\\-(\\w+)\"?><>(\\w+)"
+ name: "pinot_$1_$2_$9"
cache: true
labels:
- table: "$3"
- tableType: "$4"
- topic: "$5"
- partition: "$6"
+ database: "$4"
+ table: "$5"
+ tableType: "$6"
+ topic: "$7"
+ partition: "$8"
# This is a catch-all pattern for pinot table metrics. Patterns after this
line may be skipped.
-- pattern: "\"?org\\.apache\\.pinot\\.common\\.metrics\"?<type=\"?\\w+\"?,
name=\"?pinot\\.(\\w+)\\.(\\w+)\\.(\\w+)\"?><>(\\w+)"
- name: "pinot_$1_$3_$4"
+- pattern: "\"?org\\.apache\\.pinot\\.common\\.metrics\"?<type=\"?\\w+\"?,
name=\"?pinot\\.(\\w+)\\.((\\w+)#)*(\\w+)\\.(\\w+)\"?><>(\\w+)"
+ name: "pinot_$1_$5_$6"
cache: true
labels:
- table: "$2"
+ database: "$3"
+ table: "$4"
# This is a catch-all pattern for pinot controller metrics not related to
tables. Patterns after this line may be skipped.
- pattern: "\"?org\\.apache\\.pinot\\.common\\.metrics\"?<type=\"?\\w+\"?,
name=\"?pinot\\.(\\w+)\\.(\\w+)\"?><>(\\w+)"
Review Comment:
do we also need to update this rule?
--
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]
