You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "shounakmk219 (via GitHub)" <gi...@apache.org> on 2024/03/28 06:43:17 UTC
[PR] Update metric rules to emit database as a label [pinot]
shounakmk219 opened a new pull request, #12739:
URL: https://github.com/apache/pinot/pull/12739
# Description
This PR
1. Fixes the broken metrics for table names with database context by masking the `.` with `#` while including table name in the metric name.
2. Extracts the database name from metric name and adds it as a metric label named `database` while emitting
3. Address the bug which ran `ControllerPeriodicTasks` only on tables under `default` database
# labels
`bugfix` `observability`
# release-notes
Table related metrics will include database name as a metric label
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Update metric rules to emit database as a label [pinot]
Posted by "shounakmk219 (via GitHub)" <gi...@apache.org>.
shounakmk219 commented on code in PR #12739:
URL: https://github.com/apache/pinot/pull/12739#discussion_r1547113167
##########
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) {
- return _isTableLevelMetricsEnabled || _allowedTables.contains(tableName) ? tableName : "allTables";
+ tableName = _isTableLevelMetricsEnabled || _allowedTables.contains(tableName) ? tableName : "allTables";
+ return StringUtils.replaceOnce(tableName, ".", "#");
Review Comment:
Sure, will do
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Update metric rules to emit database as a label [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #12739:
URL: https://github.com/apache/pinot/pull/12739#discussion_r1552489465
##########
docker/images/pinot/etc/jmx_prometheus_javaagent/configs/broker.yml:
##########
@@ -1,76 +1,90 @@
rules:
-- pattern: "\"org.apache.pinot.common.metrics\"<type=\"BrokerMetrics\", name=\"pinot.broker.([^\\.]*?).authorization\"><>(\\w+)"
- name: "pinot_broker_authorization_$2"
+- pattern: "\"org.apache.pinot.common.metrics\"<type=\"BrokerMetrics\", name=\"pinot.broker.(([^\\.]+)\\.)?([^\\.]*?).authorization\"><>(\\w+)"
Review Comment:
`\\` is not needed to escape `.` (notice that we don't escape other `.`)
I think we can simplify the matching with `(([^.]+).)?([^.]+)`. Can you please try it and see if it works?
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Update metric rules to emit database as a label [pinot]
Posted by "zhtaoxiang (via GitHub)" <gi...@apache.org>.
zhtaoxiang commented on PR #12739:
URL: https://github.com/apache/pinot/pull/12739#issuecomment-2031038813
Can you please also explain how we tested it in the description section? Otherwise, looks good to me.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Update metric rules to emit database as a label [pinot]
Posted by "shounakmk219 (via GitHub)" <gi...@apache.org>.
shounakmk219 commented on code in PR #12739:
URL: https://github.com/apache/pinot/pull/12739#discussion_r1545989048
##########
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:
Make sense, will add java doc
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Update metric rules to emit database as a label [pinot]
Posted by "zhtaoxiang (via GitHub)" <gi...@apache.org>.
zhtaoxiang commented on code in PR #12739:
URL: https://github.com/apache/pinot/pull/12739#discussion_r1547100308
##########
pinot-common/src/main/java/org/apache/pinot/common/metrics/ValidationMetrics.java:
##########
@@ -133,124 +134,126 @@ public ValidationMetrics(PinotMetricsRegistry metricsRegistry) {
/**
* Updates the missing segment count gauge.
*
- * @param resource The resource for which the gauge is updated
+ * @param tableNameWithType The table name with type for which the gauge is updated
Review Comment:
since we are changing the comment and parameter names now: can we change to `tableNameWithTypeAndDb?
##########
pinot-common/src/main/java/org/apache/pinot/common/metrics/ValidationMetrics.java:
##########
@@ -133,124 +134,126 @@ public ValidationMetrics(PinotMetricsRegistry metricsRegistry) {
/**
* Updates the missing segment count gauge.
*
- * @param resource The resource for which the gauge is updated
+ * @param tableNameWithType The table name with type for which the gauge is updated
Review Comment:
since we are changing the comment and parameter names now: can we change to `tableNameWithTypeAndDb`?
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Update metric rules to emit database as a label [pinot]
Posted by "shounakmk219 (via GitHub)" <gi...@apache.org>.
shounakmk219 commented on code in PR #12739:
URL: https://github.com/apache/pinot/pull/12739#discussion_r1543349277
##########
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:
Its not related to table so can be skipped.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Update metric rules to emit database as a label [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #12739:
URL: https://github.com/apache/pinot/pull/12739#discussion_r1554045076
##########
docker/images/pinot/etc/jmx_prometheus_javaagent/configs/broker.yml:
##########
@@ -1,76 +1,90 @@
rules:
-- pattern: "\"org.apache.pinot.common.metrics\"<type=\"BrokerMetrics\", name=\"pinot.broker.([^\\.]*?).authorization\"><>(\\w+)"
- name: "pinot_broker_authorization_$2"
+- pattern: "\"org.apache.pinot.common.metrics\"<type=\"BrokerMetrics\", name=\"pinot.broker.(([^\\.]+)\\.)?([^\\.]*?).authorization\"><>(\\w+)"
Review Comment:
So here I can see 3 things we need to figure out:
1. Do we need to escape `.` when it is within `[]`? I tried it in java regex and `[^\\.]` and `[^.]` are equivalent.
2. Do we need to escape `.` when it is not in the matching parenthesis?
3. Do we need `*?` over `*`? IMO if `.` is handled correctly, greedy one (`*`) should be better
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Update metric rules to emit database as a label [pinot]
Posted by "zhtaoxiang (via GitHub)" <gi...@apache.org>.
zhtaoxiang commented on code in PR #12739:
URL: https://github.com/apache/pinot/pull/12739#discussion_r1552893564
##########
docker/images/pinot/etc/jmx_prometheus_javaagent/configs/broker.yml:
##########
@@ -1,76 +1,90 @@
rules:
-- pattern: "\"org.apache.pinot.common.metrics\"<type=\"BrokerMetrics\", name=\"pinot.broker.([^\\.]*?).authorization\"><>(\\w+)"
- name: "pinot_broker_authorization_$2"
+- pattern: "\"org.apache.pinot.common.metrics\"<type=\"BrokerMetrics\", name=\"pinot.broker.(([^\\.]+)\\.)?([^\\.]*?).authorization\"><>(\\w+)"
Review Comment:
@shounakmk219 I think your understanding about `.` is correct. We wanted to fix it, we didn't because the reason you mentioned, it just worked.
For `*?`, I don't know about promethues, but `*` vs `*?` is mainly about greedy and non greedy matching. See https://stackoverflow.com/questions/3075130/what-is-the-difference-between-and-regular-expressions
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Update metric rules to emit database as a label [pinot]
Posted by "shounakmk219 (via GitHub)" <gi...@apache.org>.
shounakmk219 commented on code in PR #12739:
URL: https://github.com/apache/pinot/pull/12739#discussion_r1543350090
##########
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:
same as above
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Update metric rules to emit database as a label [pinot]
Posted by "shounakmk219 (via GitHub)" <gi...@apache.org>.
shounakmk219 commented on code in PR #12739:
URL: https://github.com/apache/pinot/pull/12739#discussion_r1543355213
##########
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:
Good catch. Will update the logic.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Update metric rules to emit database as a label [pinot]
Posted by "shounakmk219 (via GitHub)" <gi...@apache.org>.
shounakmk219 commented on PR #12739:
URL: https://github.com/apache/pinot/pull/12739#issuecomment-2037786187
> Are we able to filter on empty database?
@Jackie-Jiang `database = ""` should work for default db filtering.
> Will it be empty string or `null`?
The `database` label will be missing on metric
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Update metric rules to emit database as a label [pinot]
Posted by "zhtaoxiang (via GitHub)" <gi...@apache.org>.
zhtaoxiang commented on code in PR #12739:
URL: https://github.com/apache/pinot/pull/12739#discussion_r1548459792
##########
docker/images/pinot/etc/jmx_prometheus_javaagent/configs/broker.yml:
##########
@@ -1,76 +1,90 @@
rules:
-- pattern: "\"org.apache.pinot.common.metrics\"<type=\"BrokerMetrics\", name=\"pinot.broker.([^\\.]*?).authorization\"><>(\\w+)"
- name: "pinot_broker_authorization_$2"
+- pattern: "\"org.apache.pinot.common.metrics\"<type=\"BrokerMetrics\", name=\"pinot.broker.(([^\\.]+?)#)*([^\\.]*?).authorization\"><>(\\w+)"
Review Comment:
1. I think this means that regular table name does not contain `#`, is this the case?
2. For table name with `.`, there is a way to match it, we don't necessarily need to change it to `#`
##########
docker/images/pinot/etc/jmx_prometheus_javaagent/configs/broker.yml:
##########
@@ -1,76 +1,90 @@
rules:
-- pattern: "\"org.apache.pinot.common.metrics\"<type=\"BrokerMetrics\", name=\"pinot.broker.([^\\.]*?).authorization\"><>(\\w+)"
- name: "pinot_broker_authorization_$2"
+- pattern: "\"org.apache.pinot.common.metrics\"<type=\"BrokerMetrics\", name=\"pinot.broker.(([^\\.]+?)#)*([^\\.]*?).authorization\"><>(\\w+)"
+ name: "pinot_broker_authorization_$4"
cache: true
labels:
- table: "$1"
-- pattern: "\"org.apache.pinot.common.metrics\"<type=\"BrokerMetrics\", name=\"pinot.broker.([^\\.]*?)\\.documentsScanned\"><>(\\w+)"
- name: "pinot_broker_documentsScanned_$2"
+ database: "$2"
Review Comment:
I think database is a logical concept, can we keep it as part of table name? If we want to filter database, we can do it on the metric query side.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Update metric rules to emit database as a label [pinot]
Posted by "shounakmk219 (via GitHub)" <gi...@apache.org>.
shounakmk219 commented on PR #12739:
URL: https://github.com/apache/pinot/pull/12739#issuecomment-2031052011
@zhtaoxiang Updated the PR description with testing performed.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Update metric rules to emit database as a label [pinot]
Posted by "shounakmk219 (via GitHub)" <gi...@apache.org>.
shounakmk219 commented on code in PR #12739:
URL: https://github.com/apache/pinot/pull/12739#discussion_r1543366902
##########
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:
1. yes that's the assumption and I have cross checked all the usages and all the caller functions are passing table name as resource.
2. meter and timer use the `AbstractMetrics.getTableName` method which also has the handling
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Update metric rules to emit database as a label [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #12739:
URL: https://github.com/apache/pinot/pull/12739#issuecomment-2024563481
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/12739?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: Patch coverage is `50.00000%` with `1 lines` in your changes are missing coverage. Please review.
> Project coverage is 45.90%. Comparing base [(`59551e4`)](https://app.codecov.io/gh/apache/pinot/commit/59551e45224f1535c4863fd577622b37366ccc97?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`086cad2`)](https://app.codecov.io/gh/apache/pinot/pull/12739?dropdown=coverage&src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 180 commits behind head on master.
| [Files](https://app.codecov.io/gh/apache/pinot/pull/12739?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...apache/pinot/common/metrics/ValidationMetrics.java](https://app.codecov.io/gh/apache/pinot/pull/12739?src=pr&el=tree&filepath=pinot-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcommon%2Fmetrics%2FValidationMetrics.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9WYWxpZGF0aW9uTWV0cmljcy5qYXZh) | 0.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/12739?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #12739 +/- ##
=============================================
- Coverage 61.75% 45.90% -15.85%
- Complexity 207 907 +700
=============================================
Files 2436 1855 -581
Lines 133233 98021 -35212
Branches 20636 15835 -4801
=============================================
- Hits 82274 44995 -37279
- Misses 44911 49688 +4777
+ Partials 6048 3338 -2710
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/12739/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/12739/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration](https://app.codecov.io/gh/apache/pinot/pull/12739/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/12739/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/12739/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/12739/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/12739/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `45.90% <50.00%> (-15.73%)` | :arrow_down: |
| [skip-bytebuffers-false](https://app.codecov.io/gh/apache/pinot/pull/12739/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `45.90% <50.00%> (-15.85%)` | :arrow_down: |
| [skip-bytebuffers-true](https://app.codecov.io/gh/apache/pinot/pull/12739/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/12739/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `45.90% <50.00%> (-15.85%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/12739/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `45.90% <50.00%> (-15.85%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/12739/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `45.90% <50.00%> (-0.99%)` | :arrow_down: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/12739/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/pinot/pull/12739?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Update metric rules to emit database as a label [pinot]
Posted by "zhtaoxiang (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Update metric rules to emit database as a label [pinot]
Posted by "shounakmk219 (via GitHub)" <gi...@apache.org>.
shounakmk219 commented on code in PR #12739:
URL: https://github.com/apache/pinot/pull/12739#discussion_r1543369785
##########
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:
we can skip as its not related to the table name
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Update metric rules to emit database as a label [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #12739:
URL: https://github.com/apache/pinot/pull/12739#discussion_r1552815537
##########
docker/images/pinot/etc/jmx_prometheus_javaagent/configs/broker.yml:
##########
@@ -1,76 +1,90 @@
rules:
-- pattern: "\"org.apache.pinot.common.metrics\"<type=\"BrokerMetrics\", name=\"pinot.broker.([^\\.]*?).authorization\"><>(\\w+)"
- name: "pinot_broker_authorization_$2"
+- pattern: "\"org.apache.pinot.common.metrics\"<type=\"BrokerMetrics\", name=\"pinot.broker.(([^\\.]+)\\.)?([^\\.]*?).authorization\"><>(\\w+)"
Review Comment:
Interesting. Maybe Prometheus regex works differently from Java regex?
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Update metric rules to emit database as a label [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #12739:
URL: https://github.com/apache/pinot/pull/12739#discussion_r1552109371
##########
docker/images/pinot/etc/jmx_prometheus_javaagent/configs/broker.yml:
##########
@@ -1,76 +1,90 @@
rules:
-- pattern: "\"org.apache.pinot.common.metrics\"<type=\"BrokerMetrics\", name=\"pinot.broker.([^\\.]*?).authorization\"><>(\\w+)"
- name: "pinot_broker_authorization_$2"
+- pattern: "\"org.apache.pinot.common.metrics\"<type=\"BrokerMetrics\", name=\"pinot.broker.(([^\\.]+)\\.)?([^\\.]*?).authorization\"><>(\\w+)"
Review Comment:
Trying to understand this regex:
1. What does `*?` stands for?
2. Seems like we don't need `\\` before `.`
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Update metric rules to emit database as a label [pinot]
Posted by "shounakmk219 (via GitHub)" <gi...@apache.org>.
shounakmk219 commented on code in PR #12739:
URL: https://github.com/apache/pinot/pull/12739#discussion_r1557429067
##########
docker/images/pinot/etc/jmx_prometheus_javaagent/configs/broker.yml:
##########
@@ -1,76 +1,90 @@
rules:
-- pattern: "\"org.apache.pinot.common.metrics\"<type=\"BrokerMetrics\", name=\"pinot.broker.([^\\.]*?).authorization\"><>(\\w+)"
- name: "pinot_broker_authorization_$2"
+- pattern: "\"org.apache.pinot.common.metrics\"<type=\"BrokerMetrics\", name=\"pinot.broker.(([^\\.]+)\\.)?([^\\.]*?).authorization\"><>(\\w+)"
Review Comment:
Came across a case where the the unescapped `.` is breaking the metric parsing
Metric endpoint result :
```
pinot_controller_segmentCount_Value{database="ncludingReplaced",table="ncludingReplaced.test",tableType="OFFLINE",} 1.0
```
Actual metric :
```
pinot.controller.segmentCountIncludingReplaced.test_OFFLINE
```
Rule it was supposed to match :
```
pattern: "\"?org\\.apache\\.pinot\\.common\\.metrics\"?<type=\"?\\w+\"?, name=\"?pinot\\.(\\w+)\\.(\\w+)\\.((\\w+)\\.)?(\\w+)_(OFFLINE|REALTIME)\\\"?><>(\\w+)"
```
Rule it actually matched :
```
pattern: "\"org.apache.pinot.common.metrics\"<type=\"ControllerMetrics\", name=\"pinot.controller.segmentCount.(([^\\.]+)\\.)?([^\\.]*?)_(OFFLINE|REALTIME)\"><>(\\w+)"
```
This answers your question @Jackie-Jiang , we need to escape `.` at all places in the pattern string to parse it as a dot literal.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Update metric rules to emit database as a label [pinot]
Posted by "shounakmk219 (via GitHub)" <gi...@apache.org>.
shounakmk219 commented on code in PR #12739:
URL: https://github.com/apache/pinot/pull/12739#discussion_r1547114035
##########
pinot-common/src/main/java/org/apache/pinot/common/metrics/ValidationMetrics.java:
##########
@@ -133,124 +134,126 @@ public ValidationMetrics(PinotMetricsRegistry metricsRegistry) {
/**
* Updates the missing segment count gauge.
*
- * @param resource The resource for which the gauge is updated
+ * @param tableNameWithType The table name with type for which the gauge is updated
Review Comment:
Good question. All internal pinot references to tableName/ rawTableName / tableNameWithType are prefixed with database name by design so we are skipping specifying it in the variable name. External table name inputs are translated at entry itself to maintain the convention for internal use.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Update metric rules to emit database as a label [pinot]
Posted by "shounakmk219 (via GitHub)" <gi...@apache.org>.
shounakmk219 commented on PR #12739:
URL: https://github.com/apache/pinot/pull/12739#issuecomment-2036327619
@zhtaoxiang Updated the rules to work with dot in table name without the need of masking it.
Table name label will include the database prefix as suggested by @Jackie-Jiang but the database label will be missing for metrics on default database as there is no way to provide default value on missing values in the rules.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Update metric rules to emit database as a label [pinot]
Posted by "shounakmk219 (via GitHub)" <gi...@apache.org>.
shounakmk219 commented on code in PR #12739:
URL: https://github.com/apache/pinot/pull/12739#discussion_r1552758099
##########
docker/images/pinot/etc/jmx_prometheus_javaagent/configs/broker.yml:
##########
@@ -1,76 +1,90 @@
rules:
-- pattern: "\"org.apache.pinot.common.metrics\"<type=\"BrokerMetrics\", name=\"pinot.broker.([^\\.]*?).authorization\"><>(\\w+)"
- name: "pinot_broker_authorization_$2"
+- pattern: "\"org.apache.pinot.common.metrics\"<type=\"BrokerMetrics\", name=\"pinot.broker.(([^\\.]+)\\.)?([^\\.]*?).authorization\"><>(\\w+)"
Review Comment:
Only the expression between `()` is considered as regex imo that's why we don’t escape the `.` outside the brackets.
Tried testing without escaping the `.` inside brackets and its behaving as a match all expression, hence we need to escape the `.` inside brackets.
Regarding `*?`, I can raise a separate PR to replace it with `+`.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Update metric rules to emit database as a label [pinot]
Posted by "shounakmk219 (via GitHub)" <gi...@apache.org>.
shounakmk219 commented on code in PR #12739:
URL: https://github.com/apache/pinot/pull/12739#discussion_r1552758099
##########
docker/images/pinot/etc/jmx_prometheus_javaagent/configs/broker.yml:
##########
@@ -1,76 +1,90 @@
rules:
-- pattern: "\"org.apache.pinot.common.metrics\"<type=\"BrokerMetrics\", name=\"pinot.broker.([^\\.]*?).authorization\"><>(\\w+)"
- name: "pinot_broker_authorization_$2"
+- pattern: "\"org.apache.pinot.common.metrics\"<type=\"BrokerMetrics\", name=\"pinot.broker.(([^\\.]+)\\.)?([^\\.]*?).authorization\"><>(\\w+)"
Review Comment:
Only the expression between `()` is considered as regex that's why we don’t escape the `.` outside the brackets.
Tried testing without escaping the `.` inside brackets and its behaving as a match all expression, hence we need to escape the `.` inside brackets.
Regarding `*?`, I can raise a separate PR to replace it with `+`.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Update metric rules to emit database as a label [pinot]
Posted by "shounakmk219 (via GitHub)" <gi...@apache.org>.
shounakmk219 commented on code in PR #12739:
URL: https://github.com/apache/pinot/pull/12739#discussion_r1548851976
##########
docker/images/pinot/etc/jmx_prometheus_javaagent/configs/broker.yml:
##########
@@ -1,76 +1,90 @@
rules:
-- pattern: "\"org.apache.pinot.common.metrics\"<type=\"BrokerMetrics\", name=\"pinot.broker.([^\\.]*?).authorization\"><>(\\w+)"
- name: "pinot_broker_authorization_$2"
+- pattern: "\"org.apache.pinot.common.metrics\"<type=\"BrokerMetrics\", name=\"pinot.broker.(([^\\.]+?)#)*([^\\.]*?).authorization\"><>(\\w+)"
+ name: "pinot_broker_authorization_$4"
cache: true
labels:
- table: "$1"
-- pattern: "\"org.apache.pinot.common.metrics\"<type=\"BrokerMetrics\", name=\"pinot.broker.([^\\.]*?)\\.documentsScanned\"><>(\\w+)"
- name: "pinot_broker_documentsScanned_$2"
+ database: "$2"
Review Comment:
Makes sense. While keeping it as part of the table let's also emit it as a label separately to ease the filtering maybe?
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Update metric rules to emit database as a label [pinot]
Posted by "shounakmk219 (via GitHub)" <gi...@apache.org>.
shounakmk219 commented on code in PR #12739:
URL: https://github.com/apache/pinot/pull/12739#discussion_r1543349757
##########
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 as above
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Update metric rules to emit database as a label [pinot]
Posted by "shounakmk219 (via GitHub)" <gi...@apache.org>.
shounakmk219 commented on code in PR #12739:
URL: https://github.com/apache/pinot/pull/12739#discussion_r1552121824
##########
docker/images/pinot/etc/jmx_prometheus_javaagent/configs/broker.yml:
##########
@@ -1,76 +1,90 @@
rules:
-- pattern: "\"org.apache.pinot.common.metrics\"<type=\"BrokerMetrics\", name=\"pinot.broker.([^\\.]*?).authorization\"><>(\\w+)"
- name: "pinot_broker_authorization_$2"
+- pattern: "\"org.apache.pinot.common.metrics\"<type=\"BrokerMetrics\", name=\"pinot.broker.(([^\\.]+)\\.)?([^\\.]*?).authorization\"><>(\\w+)"
Review Comment:
Not really sure about `*?`.
`(([^\\.]+)\\.)?` is the new regex being added to match database prefix with `.` . I think `\\` is needed to escape `.` as that's a literal.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Update metric rules to emit database as a label [pinot]
Posted by "shounakmk219 (via GitHub)" <gi...@apache.org>.
shounakmk219 commented on code in PR #12739:
URL: https://github.com/apache/pinot/pull/12739#discussion_r1552865521
##########
docker/images/pinot/etc/jmx_prometheus_javaagent/configs/broker.yml:
##########
@@ -1,76 +1,90 @@
rules:
-- pattern: "\"org.apache.pinot.common.metrics\"<type=\"BrokerMetrics\", name=\"pinot.broker.([^\\.]*?).authorization\"><>(\\w+)"
- name: "pinot_broker_authorization_$2"
+- pattern: "\"org.apache.pinot.common.metrics\"<type=\"BrokerMetrics\", name=\"pinot.broker.(([^\\.]+)\\.)?([^\\.]*?).authorization\"><>(\\w+)"
Review Comment:
My understanding is wrong mostly. The `.` requires escaping throughout I suppose. The only reason its working right now is because its surrounded by specific patterns, so the match all `.` is only matching actual `.` character here.
If you see the catch all rules, there we are escaping the `.` even outside the brackets.
We need to fix this. Will open a separate PR on this.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Update metric rules to emit database as a label [pinot]
Posted by "shounakmk219 (via GitHub)" <gi...@apache.org>.
shounakmk219 commented on code in PR #12739:
URL: https://github.com/apache/pinot/pull/12739#discussion_r1552920282
##########
docker/images/pinot/etc/jmx_prometheus_javaagent/configs/broker.yml:
##########
@@ -1,76 +1,90 @@
rules:
-- pattern: "\"org.apache.pinot.common.metrics\"<type=\"BrokerMetrics\", name=\"pinot.broker.([^\\.]*?).authorization\"><>(\\w+)"
- name: "pinot_broker_authorization_$2"
+- pattern: "\"org.apache.pinot.common.metrics\"<type=\"BrokerMetrics\", name=\"pinot.broker.(([^\\.]+)\\.)?([^\\.]*?).authorization\"><>(\\w+)"
Review Comment:
Nice, then in our case existing non-greedy approach will be optimal as this is supposed to only match the table name which is usually not very long. So no change on `*?`.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Update metric rules to emit database as a label [pinot]
Posted by "KKcorps (via GitHub)" <gi...@apache.org>.
KKcorps merged PR #12739:
URL: https://github.com/apache/pinot/pull/12739
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Update metric rules to emit database as a label [pinot]
Posted by "zhtaoxiang (via GitHub)" <gi...@apache.org>.
zhtaoxiang commented on code in PR #12739:
URL: https://github.com/apache/pinot/pull/12739#discussion_r1545985390
##########
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:
If that is the case, then everyone needs to follow this usage pattern. If anyone breaks this pattern, then the logic does not work.
Is it possible to put some logic to enforce the usage pattern? Or at least put some java doc so that everyone will follow this usage pattern?
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Update metric rules to emit database as a label [pinot]
Posted by "zhtaoxiang (via GitHub)" <gi...@apache.org>.
zhtaoxiang commented on code in PR #12739:
URL: https://github.com/apache/pinot/pull/12739#discussion_r1547100308
##########
pinot-common/src/main/java/org/apache/pinot/common/metrics/ValidationMetrics.java:
##########
@@ -133,124 +134,126 @@ public ValidationMetrics(PinotMetricsRegistry metricsRegistry) {
/**
* Updates the missing segment count gauge.
*
- * @param resource The resource for which the gauge is updated
+ * @param tableNameWithType The table name with type for which the gauge is updated
Review Comment:
since we are changing the comment and parameter names now: can we change to table name with type and database?
##########
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) {
- return _isTableLevelMetricsEnabled || _allowedTables.contains(tableName) ? tableName : "allTables";
+ tableName = _isTableLevelMetricsEnabled || _allowedTables.contains(tableName) ? tableName : "allTables";
+ return StringUtils.replaceOnce(tableName, ".", "#");
Review Comment:
can we please add some comments to explain why we need this change? this will make the code easy to read and understand
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org