Metric for count of tables configured with various tier backends #12940
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #12940 +/- ##
============================================
+ Coverage 61.75% 62.18% +0.43%
+ Complexity 207 198 -9
============================================
Files 2436 2502 +66
Lines 133233 136607 +3374
Branches 20636 21149 +513
============================================
+ Hits 82274 84945 +2671
- Misses 44911 45384 +473
- Partials 6048 6278 +230
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| @@ -135,6 +137,13 @@ protected void postprocess(Context context) { | |||
| _controllerMetrics.setValueOfGlobalGauge(ControllerGauge.UPSERT_TABLE_COUNT, context._upsertTableCount); | |||
| _controllerMetrics.setValueOfGlobalGauge(ControllerGauge.DISABLED_TABLE_COUNT, context._disabledTables.size()); | |||
|
|
|||
| // metric for total number of tables using a particular tier backend | |||
| context._tierBackendTableCountMap.forEach((tier, count) -> | |||
There was a problem hiding this comment.
Suggest to have a static method to generate tier metric name, similar to getTableFullMeterName
| @@ -135,6 +137,13 @@ protected void postprocess(Context context) { | |||
| _controllerMetrics.setValueOfGlobalGauge(ControllerGauge.UPSERT_TABLE_COUNT, context._upsertTableCount); | |||
| _controllerMetrics.setValueOfGlobalGauge(ControllerGauge.DISABLED_TABLE_COUNT, context._disabledTables.size()); | |||
|
|
|||
| // metric for total number of tables using a particular tier backend | |||
| context._tierBackendTableCountMap.forEach((tier, count) -> | |||
| _controllerMetrics.setOrUpdateGauge(tier + ControllerGauge.TIER_BACKEND_TABLE_COUNT.getGaugeName(), count)); | |||
There was a problem hiding this comment.
You also need to handle metrics deletion right?
There was a problem hiding this comment.
Good point! Tried to address it by maintaining the gauge names. Could not think of a cleaner way to handle it.
| @@ -766,6 +767,10 @@ private String composeTableGaugeName(final String tableName, final String key, f | |||
| return gauge.getGaugeName() + "." + getTableName(tableName) + "." + key; | |||
| } | |||
|
|
|||
| public String composePluginGaugeName(String pluginName, Gauge gauge) { | |||
| return pluginName + StringUtils.capitalize(gauge.getGaugeName()); | |||
There was a problem hiding this comment.
do you want to add a dot int between ?
pluginName + "." + StringUtils.capitalize(gauge.getGaugeName());
There was a problem hiding this comment.
Makes sense, will do.
| @@ -135,6 +138,17 @@ protected void postprocess(Context context) { | |||
| _controllerMetrics.setValueOfGlobalGauge(ControllerGauge.UPSERT_TABLE_COUNT, context._upsertTableCount); | |||
| _controllerMetrics.setValueOfGlobalGauge(ControllerGauge.DISABLED_TABLE_COUNT, context._disabledTables.size()); | |||
|
|
|||
| _tierBackendGauges.forEach(_controllerMetrics::removeGauge); | |||
There was a problem hiding this comment.
Do you want to clear _tierBackendGauges as well?
Another approach is to not remove all the gauges but find the diff:
You can keep track all the valid metric names from this loop in a set: context._tierBackendTableCountMap.forEach((tier, count) -> {...}
say newTierBackendGauges, then call
_tierBackendGauges. retainAll(newTierBackendGauges) to get the gauge names not existed.
Then call removeGauge and last thing is
_tierBackendGauges.clear();
_tierBackendGauges.putAll(newTierBackendGauges)
There was a problem hiding this comment.
I was using _tierBackendGauges just to remove all the existing tier gauges. It will be a superset of all the encountered tier backends. Anyways the loop on context._tierBackendTableCountMap takes care of recreating the required gauges.
There was a problem hiding this comment.
then you need to add _tierBackendGauges.clear() to remove all the existing gauges right? Otherwise this set is increasing all the time.
There was a problem hiding this comment.
Yeah but there will be 1 entry per tier backend plugin which won't be much so I thought we can avoid the complication of managing the previous/new diff and just delete all gauges for the tiers which the controller has witnessed since boot up. What do you think?
…/pinot into tier-backend-metric
…nakmk219/pinot into tier-backend-metric" This reverts commit 9646c83, reversing changes made to a3f3d10.
Metric for
pinot.controller.XYZtierBackendTableCountpinot.controller.tierBackendTableCount