You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2020/09/19 01:44:06 UTC
[GitHub] [incubator-pinot] sajjad-moradi opened a new pull request #6037: Add whitelist for emitting table level metrics
sajjad-moradi opened a new pull request #6037:
URL: https://github.com/apache/incubator-pinot/pull/6037
## Description
Currently there is one config to enable/disable emitting table level metrics for all tables in a cluster. For big clusters, let's say the ones with a few thousands tables, this is problematic as one might need to set up alerts/monitoring on some the tables that have higher priority than others in the cluster. This PR introduces a config for whitelisted tables to address this problem. Table level metrics are still emitted for whitelisted tables even if emitting table level metrics is disabled for the whole cluster.
## Testing Done
For the following scenarios, deployed pinot locally; ran quick-start-batch script; and took a snapshot of metrics using `jvisualvm`.
1) `enableTableLevelMetrics = false` and added `baseballStats` to the whitelist
2) `enableTableLevelMetrics = false` and empty whitelist
3) `enableTableLevelMetrics = true`
----------------------------------------------------------------
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.
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
[GitHub] [incubator-pinot] kishoreg commented on pull request #6037: Add list of allowed tables for emitting table level metrics
Posted by GitBox <gi...@apache.org>.
kishoreg commented on pull request #6037:
URL: https://github.com/apache/incubator-pinot/pull/6037#issuecomment-696321992
> > Aren’t we trying to address the limitations of monitoring systems in Pinot?
>
> I'm not aware that. Could you elaborate a bit more? what's exactly the plan and what's the timeline for it?
> The solution presented here is regarding a large cluster at Linkedin for which table level metrics are disabled for all tables. The current situation is risky as for some high priority tables, we don't get alerted. This PR immediately alleviates the existing issue.
Sorry, I was referring to the changes in this PR. Ideally, we should be logging metrics for all tables/resources. It's up to the operators to set alerts on the right tables that are important for the business.
By adding this new config, we are adding a workaround in Pinot to overcome the limitation with the metrics system which cannot handle thousands of tables.
----------------------------------------------------------------
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.
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
[GitHub] [incubator-pinot] sajjad-moradi commented on a change in pull request #6037: Add list of allowed tables for emitting table level metrics
Posted by GitBox <gi...@apache.org>.
sajjad-moradi commented on a change in pull request #6037:
URL: https://github.com/apache/incubator-pinot/pull/6037#discussion_r493019744
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/CommonConstants.java
##########
@@ -279,7 +281,10 @@
}
public static final String DEFAULT_METRICS_PREFIX = "pinot.server.";
- public static final boolean DEFAULT_METRICS_GLOBAL_ENABLED = false;
+ public static final String CONFIG_OF_ENABLE_TABLE_LEVEL_METRICS = "pinot.server.enableTableLevelMetrics";
Review comment:
Other than keeping similar config in one place, there's also this todo in server conf: `TODO: Replace with constants in CommonConstants`.
Having said that the main reason to move to CommonConstant is that in the existing code, the default value for enabling table level metrics is defined in two places: ServerConf and CommonConstants. ServerConf lives in pinot-broker module and CommonConstant in pinot-common. Both are needed as ServerConf is used in ServerInstance defined in pinot-broker module and CommonConstant is used in ServerMetrics which is defined in pinot-common module.
Having it only defined in CommonConstant removes this duplicity.
----------------------------------------------------------------
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.
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
[GitHub] [incubator-pinot] mcvsubbu commented on a change in pull request #6037: Add list of allowed tables for emitting table level metrics
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #6037:
URL: https://github.com/apache/incubator-pinot/pull/6037#discussion_r492959921
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/CommonConstants.java
##########
@@ -279,7 +281,10 @@
}
public static final String DEFAULT_METRICS_PREFIX = "pinot.server.";
- public static final boolean DEFAULT_METRICS_GLOBAL_ENABLED = false;
+ public static final String CONFIG_OF_ENABLE_TABLE_LEVEL_METRICS = "pinot.server.enableTableLevelMetrics";
Review comment:
I think you are trying to keep the new configs you introduced in the same place. It is unfortunate that we don't have a BrokerConf (as yet), but let us keep the server-specific configs in the ServerConf file instead of moving that up to common constants.
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/metrics/AbstractMetrics.java
##########
@@ -47,21 +53,36 @@
private final Map<String, AtomicLong> _gaugeValues = new ConcurrentHashMap<String, AtomicLong>();
- protected final boolean _global;
+ private final boolean _isTableLevelMetricsEnabled;
Review comment:
thanks for changing this 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.
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
[GitHub] [incubator-pinot] mcvsubbu commented on a change in pull request #6037: Add list of allowed tables for emitting table level metrics
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #6037:
URL: https://github.com/apache/incubator-pinot/pull/6037#discussion_r492959921
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/CommonConstants.java
##########
@@ -279,7 +281,10 @@
}
public static final String DEFAULT_METRICS_PREFIX = "pinot.server.";
- public static final boolean DEFAULT_METRICS_GLOBAL_ENABLED = false;
+ public static final String CONFIG_OF_ENABLE_TABLE_LEVEL_METRICS = "pinot.server.enableTableLevelMetrics";
Review comment:
I think you are trying to keep the new configs you introduced in the same place. It is unfortunate that we don't have a BrokerConf (as yet), but let us keep the server-specific configs in the ServerConf file instead of moving that up to common constants.
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/metrics/AbstractMetrics.java
##########
@@ -47,21 +53,36 @@
private final Map<String, AtomicLong> _gaugeValues = new ConcurrentHashMap<String, AtomicLong>();
- protected final boolean _global;
+ private final boolean _isTableLevelMetricsEnabled;
Review comment:
thanks for changing this 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.
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
[GitHub] [incubator-pinot] sajjad-moradi commented on pull request #6037: Add list of allowed tables for emitting table level metrics
Posted by GitBox <gi...@apache.org>.
sajjad-moradi commented on pull request #6037:
URL: https://github.com/apache/incubator-pinot/pull/6037#issuecomment-696225516
> Aren’t we trying to address the limitations of monitoring systems in Pinot?
I'm not aware that. Could you elaborate a bit more? what's exactly the plan and what's the timeline for it?
The solution presented here is regarding a large cluster at Linkedin for which table level metrics are disabled for all tables. The current situation is risky as for some high priority tables, we don't get alerted. This PR immediately alleviates the existing issue.
----------------------------------------------------------------
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.
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
[GitHub] [incubator-pinot] kishoreg commented on pull request #6037: Add list of allowed tables for emitting table level metrics
Posted by GitBox <gi...@apache.org>.
kishoreg commented on pull request #6037:
URL: https://github.com/apache/incubator-pinot/pull/6037#issuecomment-696321992
> > Aren’t we trying to address the limitations of monitoring systems in Pinot?
>
> I'm not aware that. Could you elaborate a bit more? what's exactly the plan and what's the timeline for it?
> The solution presented here is regarding a large cluster at Linkedin for which table level metrics are disabled for all tables. The current situation is risky as for some high priority tables, we don't get alerted. This PR immediately alleviates the existing issue.
Sorry, I was referring to the changes in this PR. Ideally, we should be logging metrics for all tables/resources. It's up to the operators to set alerts on the right tables that are important for the business.
By adding this new config, we are adding a workaround in Pinot to overcome the limitation with the metrics system which cannot handle thousands of tables.
----------------------------------------------------------------
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.
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
[GitHub] [incubator-pinot] sajjad-moradi commented on a change in pull request #6037: Add list of allowed tables for emitting table level metrics
Posted by GitBox <gi...@apache.org>.
sajjad-moradi commented on a change in pull request #6037:
URL: https://github.com/apache/incubator-pinot/pull/6037#discussion_r493019744
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/CommonConstants.java
##########
@@ -279,7 +281,10 @@
}
public static final String DEFAULT_METRICS_PREFIX = "pinot.server.";
- public static final boolean DEFAULT_METRICS_GLOBAL_ENABLED = false;
+ public static final String CONFIG_OF_ENABLE_TABLE_LEVEL_METRICS = "pinot.server.enableTableLevelMetrics";
Review comment:
Other than keeping similar config in one place, there's also this todo in server conf: `TODO: Replace with constants in CommonConstants`.
Having said that the main reason to move to CommonConstant is that in the existing code, the default value for enabling table level metrics is defined in two places: ServerConf and CommonConstants. ServerConf lives in pinot-broker module and CommonConstant in pinot-common. Both are needed as ServerConf is used in ServerInstance defined in pinot-broker module and CommonConstant is used in ServerMetrics which is defined in pinot-common module.
Having it only defined in CommonConstant removes this duplicity.
----------------------------------------------------------------
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.
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
[GitHub] [incubator-pinot] sajjad-moradi commented on pull request #6037: Add list of allowed tables for emitting table level metrics
Posted by GitBox <gi...@apache.org>.
sajjad-moradi commented on pull request #6037:
URL: https://github.com/apache/incubator-pinot/pull/6037#issuecomment-696225516
> Aren’t we trying to address the limitations of monitoring systems in Pinot?
I'm not aware that. Could you elaborate a bit more? what's exactly the plan and what's the timeline for it?
The solution presented here is regarding a large cluster at Linkedin for which table level metrics are disabled for all tables. The current situation is risky as for some high priority tables, we don't get alerted. This PR immediately alleviates the existing issue.
----------------------------------------------------------------
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.
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
[GitHub] [incubator-pinot] mcvsubbu merged pull request #6037: Add list of allowed tables for emitting table level metrics
Posted by GitBox <gi...@apache.org>.
mcvsubbu merged pull request #6037:
URL: https://github.com/apache/incubator-pinot/pull/6037
----------------------------------------------------------------
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.
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