Catch-all Regex for JXM -> Prom Exporter #12073
Conversation
| # 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+)" |
There was a problem hiding this comment.
Should we change the existing catch all? I feel like the old one might be capturing something which may get lost with this new
It's better imo to include this one as a seperate rule
There was a problem hiding this comment.
Good catch, fixed.
|
Out of the scope of this PR, will these regex increase the memory footprint of the metrics? I've been observing saw-shape memory usage on servers without query load, and suspecting that is caused by the metrics. #8232 listed some previous work to reduce the memory footprint, which might provide some insights. |
| @@ -157,15 +157,37 @@ rules: | |||
|
|
|||
| ## Metrics that fit the catch-all patterns above should not be added to this file. | |||
| ## 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. | |||
| # This is a catch-all pattern for pinot table metrics with offline/realtime suffix without kafka topic | |||
| # Patterns after this line may be skipped. | |||
There was a problem hiding this comment.
nit: I feel that we should add this comment for each catch-all pattern so that people know the potential risks?
| table: "$1" | ||
| tableType: "$2" | ||
| partition: "$3" | ||
| #- pattern: "\"org.apache.pinot.common.metrics\"<type=\"ServerMetrics\", name=\"pinot.server.llcPartitionConsuming.([^\\.]*?)_(OFFLINE|REALTIME)\\-(.+)\\-(\\w+)\"><>(\\w+)" |
There was a problem hiding this comment.
just want to confirm: with those newly-added patterns, those commented out ones will remain the same, right? We don't want to introduce breaking changes 😛
There was a problem hiding this comment.
Yes, that has been tested. Have explained in the PR description.
There was a problem hiding this comment.
Commented metrics are exported successfully
|
| #- pattern: "\"org.apache.pinot.common.metrics\"<type=\"ServerMetrics\", name=\"pinot.server.highestStreamOffsetConsumed.([^\\.]*?)_(OFFLINE|REALTIME)\\-(.+)\\-(\\w+)\"><>(\\w+)" | ||
| # name: "pinot_server_highestStreamOffsetConsumed_$5" | ||
| # cache: true | ||
| # labels: | ||
| # table: "$1" | ||
| # tableType: "$2" | ||
| # topic: "$3" | ||
| # partition: "$4" |
There was a problem hiding this comment.
do we still need this? can we delete?
There was a problem hiding this comment.
and also can we remove other commented code?
It is difficult to analyze why #8232 increased efficiency given it:
I would say it is quite more expensive to process hundreds of very large regexps with the same prefix than a couple of them with correct quantifiers. And it is obviously easier for us to write. |
Recently, we detected that the catch-all label was incorrect - It failed to capture a metric for which a specific regex had not been added. It also does not capture metrics that contain the kafka topic in the name. This PR fixes these problems. Commented regexes are captured with catch-all, I have kept them as they can come in handy later.
Commented metrics are exported successfully: