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 2021/03/03 23:57:09 UTC
[GitHub] [incubator-pinot] jackjlli opened a new pull request #6640: Make Pinot metrics pluggable
jackjlli opened a new pull request #6640:
URL: https://github.com/apache/incubator-pinot/pull/6640
## Description
This PR makes Pinot metrics pluggable. This is useful as more metric systems can be used for pinot.
This PR also merges MetricsHelper into PinotMetricsUtils, as these two classes are pretty much doing the same thing.
## Upgrade Notes
Does this PR prevent a zero down-time upgrade? (Assume upgrade order: Controller, Broker, Server, Minion)
* [ ] Yes (Please label as **<code>backward-incompat</code>**, and complete the section below on Release Notes)
Does this PR fix a zero-downtime upgrade introduced earlier?
* [ ] Yes (Please label this as **<code>backward-incompat</code>**, and complete the section below on Release Notes)
Does this PR otherwise need attention when creating release notes? Things to consider:
- New configuration options
- Deprecation of configurations
- Signature changes to public methods/interfaces
- New plugins added or old plugins removed
* [ ] Yes (Please label this PR as **<code>release-notes</code>** and complete the section on Release Notes)
## Release Notes
If you have tagged this as either backward-incompat or release-notes,
you MUST add text here that you would like to see appear in release notes of the
next release.
If you have a series of commits adding or enabling a feature, then
add this section only in final commit that marks the feature completed.
Refer to earlier release notes to see examples of text
## Documentation
If you have introduced a new feature or configuration, please add it to the documentation as well.
See https://docs.pinot.apache.org/developers/developers-and-contributors/update-document
----------------------------------------------------------------
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] fx19880617 commented on a change in pull request #6640: Make Pinot metrics pluggable
Posted by GitBox <gi...@apache.org>.
fx19880617 commented on a change in pull request #6640:
URL: https://github.com/apache/incubator-pinot/pull/6640#discussion_r588158268
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/metrics/PinotMetricUtils.java
##########
@@ -18,82 +18,186 @@
*/
package org.apache.pinot.common.metrics;
+import com.google.common.base.Preconditions;
+import java.lang.reflect.Constructor;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.TimeUnit;
import java.util.function.Function;
-import org.apache.pinot.common.exception.InvalidConfigException;
+import org.apache.pinot.spi.annotations.metrics.MetricsFactory;
+import org.apache.pinot.spi.annotations.metrics.PinotMetricsFactory;
import org.apache.pinot.spi.env.PinotConfiguration;
import org.apache.pinot.spi.metrics.PinotGauge;
import org.apache.pinot.spi.metrics.PinotJmxReporter;
+import org.apache.pinot.spi.metrics.PinotMeter;
import org.apache.pinot.spi.metrics.PinotMetricName;
import org.apache.pinot.spi.metrics.PinotMetricsRegistry;
-import org.apache.pinot.common.metrics.yammer.YammerGauge;
-import org.apache.pinot.common.metrics.yammer.YammerJmxReporter;
-import org.apache.pinot.common.metrics.yammer.YammerMetricName;
-import org.apache.pinot.common.metrics.yammer.YammerMetricsRegistry;
+import org.apache.pinot.spi.metrics.PinotTimer;
+import org.apache.pinot.spi.utils.PinotReflectionUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
public class PinotMetricUtils {
private static final Logger LOGGER = LoggerFactory.getLogger(PinotMetricUtils.class);
- public static final String LIBRARY_NAME_KEY = "libraryName";
- public static final String YAMMER_KEY = "yammer";
+ private static final String METRICS_PACKAGE_REGEX_PATTERN = ".*\\.plugin\\.metrics\\..*";
- private static String LIBRARY_TO_USE = YAMMER_KEY;
+ private static PinotMetricsFactory _pinotMetricsFactory = null;
- public static void init(PinotConfiguration metricsConfiguration)
- throws InvalidConfigException {
- String libraryName = metricsConfiguration.getProperty(PinotMetricUtils.LIBRARY_NAME_KEY);
- if (libraryName == null) {
- return;
- }
- switch (libraryName) {
- case YAMMER_KEY:
- LIBRARY_TO_USE = YAMMER_KEY;
- break;
- // TODO: support more libraries.
- default:
- throw new InvalidConfigException("PinotMetricsRegistry for " + libraryName + " cannot be initialized.");
+ private static final Map<PinotMetricsRegistry, Boolean> metricsRegistryMap = new ConcurrentHashMap<>();
+ private static final Map<MetricsRegistryRegistrationListener, Boolean> metricsRegistryRegistrationListenersMap =
+ new ConcurrentHashMap<>();
+
+ public static void init(PinotConfiguration metricsConfiguration) {
+ // Initializes PinotMetricsFactory.
+ initializePinotMetricsFactory(metricsConfiguration);
+
+ // Initializes metrics using the metrics configuration.
+ initializeMetrics(metricsConfiguration);
+ registerMetricsRegistry(getPinotMetricsRegistry());
+ }
+
+ /**
+ * Initializes PinotMetricsFactory with metrics configurations.
+ * @param metricsConfiguration The subset of the configuration containing the metrics-related keys
+ */
+ private static void initializePinotMetricsFactory(PinotConfiguration metricsConfiguration) {
+ Set<Class<?>> classes = getPinotMetricsFactoryClasses();
+ for (Class<?> clazz : classes) {
Review comment:
Can we support multiple MetricsFactory classes?
----------------------------------------------------------------
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] fx19880617 commented on a change in pull request #6640: Make Pinot metrics pluggable
Posted by GitBox <gi...@apache.org>.
fx19880617 commented on a change in pull request #6640:
URL: https://github.com/apache/incubator-pinot/pull/6640#discussion_r586908508
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/metrics/PinotMetricUtils.java
##########
@@ -18,82 +18,186 @@
*/
package org.apache.pinot.common.metrics;
+import java.lang.reflect.Constructor;
+import java.util.Arrays;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.TimeUnit;
import java.util.function.Function;
-import org.apache.pinot.common.exception.InvalidConfigException;
+import org.apache.pinot.plugin.metrics.yammer.YammerMetricsFactory;
+import org.apache.pinot.spi.annotations.metrics.MetricsFactory;
+import org.apache.pinot.spi.annotations.metrics.PinotMetricsFactory;
import org.apache.pinot.spi.env.PinotConfiguration;
import org.apache.pinot.spi.metrics.PinotGauge;
import org.apache.pinot.spi.metrics.PinotJmxReporter;
+import org.apache.pinot.spi.metrics.PinotMeter;
import org.apache.pinot.spi.metrics.PinotMetricName;
import org.apache.pinot.spi.metrics.PinotMetricsRegistry;
-import org.apache.pinot.common.metrics.yammer.YammerGauge;
-import org.apache.pinot.common.metrics.yammer.YammerJmxReporter;
-import org.apache.pinot.common.metrics.yammer.YammerMetricName;
-import org.apache.pinot.common.metrics.yammer.YammerMetricsRegistry;
+import org.apache.pinot.spi.metrics.PinotTimer;
+import org.reflections.Reflections;
+import org.reflections.scanners.TypeAnnotationsScanner;
+import org.reflections.util.ClasspathHelper;
+import org.reflections.util.ConfigurationBuilder;
+import org.reflections.util.FilterBuilder;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
public class PinotMetricUtils {
private static final Logger LOGGER = LoggerFactory.getLogger(PinotMetricUtils.class);
- public static final String LIBRARY_NAME_KEY = "libraryName";
- public static final String YAMMER_KEY = "yammer";
+ private static final String METRICS_PACKAGE_REGEX_PATTERN = ".*\\.plugin\\.metrics\\..*";
- private static String LIBRARY_TO_USE = YAMMER_KEY;
+ private static PinotMetricsFactory _pinotMetricsFactory = null;
- public static void init(PinotConfiguration metricsConfiguration)
- throws InvalidConfigException {
- String libraryName = metricsConfiguration.getProperty(PinotMetricUtils.LIBRARY_NAME_KEY);
- if (libraryName == null) {
- return;
- }
- switch (libraryName) {
- case YAMMER_KEY:
- LIBRARY_TO_USE = YAMMER_KEY;
- break;
- // TODO: support more libraries.
- default:
- throw new InvalidConfigException("PinotMetricsRegistry for " + libraryName + " cannot be initialized.");
+ private static final Map<PinotMetricsRegistry, Boolean> metricsRegistryMap = new ConcurrentHashMap<>();
+ private static final Map<MetricsRegistryRegistrationListener, Boolean> metricsRegistryRegistrationListenersMap =
+ new ConcurrentHashMap<>();
+
+ public static void init(PinotConfiguration metricsConfiguration) {
+ // Initializes PinotMetricsFactory.
+ initializePinotMetricsFactory(metricsConfiguration);
+
+ // Initializes metrics using the metrics configuration.
+ initializeMetrics(metricsConfiguration);
+ registerMetricsRegistry(getPinotMetricsRegistry());
+ }
+
+ /**
+ * Initializes PinotMetricsFactory with metrics configurations.
+ * @param metricsConfiguration The subset of the configuration containing the metrics-related keys
+ */
+ private static void initializePinotMetricsFactory(PinotConfiguration metricsConfiguration) {
+ Set<Class<?>> classes = getMetricsClasses();
+ for (Class<?> clazz : classes) {
+ MetricsFactory annotation = clazz.getAnnotation(MetricsFactory.class);
+ if (annotation.enabled()) {
+ try {
+ PinotMetricsFactory pinotMetricsFactory = (PinotMetricsFactory) clazz.newInstance();
+ pinotMetricsFactory.init(metricsConfiguration);
+ registerMetricsFactory(pinotMetricsFactory);
+ } catch (Exception e) {
+ LOGGER.error("Caught exception while initializing pinot metrics registry: {}, skipping it", clazz, e);
+ }
+ }
}
- LOGGER.info("Setting metric library to: " + LIBRARY_TO_USE);
}
- public static PinotMetricsRegistry getPinotMetricsRegistry() {
- switch (LIBRARY_TO_USE) {
- case YAMMER_KEY:
- return new YammerMetricsRegistry();
- //TODO: support more libraries.
- default:
- return new YammerMetricsRegistry();
+ private static Set<Class<?>> getMetricsClasses() {
Review comment:
I found this util function is useful also in pinot minion, shall we move it to `pinot-spi`?
----------------------------------------------------------------
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] jackjlli commented on a change in pull request #6640: Make Pinot metrics pluggable
Posted by GitBox <gi...@apache.org>.
jackjlli commented on a change in pull request #6640:
URL: https://github.com/apache/incubator-pinot/pull/6640#discussion_r588716590
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/metrics/PinotMetricUtils.java
##########
@@ -18,82 +18,186 @@
*/
package org.apache.pinot.common.metrics;
+import com.google.common.base.Preconditions;
+import java.lang.reflect.Constructor;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.TimeUnit;
import java.util.function.Function;
-import org.apache.pinot.common.exception.InvalidConfigException;
+import org.apache.pinot.spi.annotations.metrics.MetricsFactory;
+import org.apache.pinot.spi.annotations.metrics.PinotMetricsFactory;
import org.apache.pinot.spi.env.PinotConfiguration;
import org.apache.pinot.spi.metrics.PinotGauge;
import org.apache.pinot.spi.metrics.PinotJmxReporter;
+import org.apache.pinot.spi.metrics.PinotMeter;
import org.apache.pinot.spi.metrics.PinotMetricName;
import org.apache.pinot.spi.metrics.PinotMetricsRegistry;
-import org.apache.pinot.common.metrics.yammer.YammerGauge;
-import org.apache.pinot.common.metrics.yammer.YammerJmxReporter;
-import org.apache.pinot.common.metrics.yammer.YammerMetricName;
-import org.apache.pinot.common.metrics.yammer.YammerMetricsRegistry;
+import org.apache.pinot.spi.metrics.PinotTimer;
+import org.apache.pinot.spi.utils.PinotReflectionUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
public class PinotMetricUtils {
private static final Logger LOGGER = LoggerFactory.getLogger(PinotMetricUtils.class);
- public static final String LIBRARY_NAME_KEY = "libraryName";
- public static final String YAMMER_KEY = "yammer";
+ private static final String METRICS_PACKAGE_REGEX_PATTERN = ".*\\.plugin\\.metrics\\..*";
- private static String LIBRARY_TO_USE = YAMMER_KEY;
+ private static PinotMetricsFactory _pinotMetricsFactory = null;
- public static void init(PinotConfiguration metricsConfiguration)
- throws InvalidConfigException {
- String libraryName = metricsConfiguration.getProperty(PinotMetricUtils.LIBRARY_NAME_KEY);
- if (libraryName == null) {
- return;
- }
- switch (libraryName) {
- case YAMMER_KEY:
- LIBRARY_TO_USE = YAMMER_KEY;
- break;
- // TODO: support more libraries.
- default:
- throw new InvalidConfigException("PinotMetricsRegistry for " + libraryName + " cannot be initialized.");
+ private static final Map<PinotMetricsRegistry, Boolean> metricsRegistryMap = new ConcurrentHashMap<>();
+ private static final Map<MetricsRegistryRegistrationListener, Boolean> metricsRegistryRegistrationListenersMap =
+ new ConcurrentHashMap<>();
+
+ public static void init(PinotConfiguration metricsConfiguration) {
+ // Initializes PinotMetricsFactory.
+ initializePinotMetricsFactory(metricsConfiguration);
+
+ // Initializes metrics using the metrics configuration.
+ initializeMetrics(metricsConfiguration);
+ registerMetricsRegistry(getPinotMetricsRegistry());
+ }
+
+ /**
+ * Initializes PinotMetricsFactory with metrics configurations.
+ * @param metricsConfiguration The subset of the configuration containing the metrics-related keys
+ */
+ private static void initializePinotMetricsFactory(PinotConfiguration metricsConfiguration) {
+ Set<Class<?>> classes = getPinotMetricsFactoryClasses();
+ for (Class<?> clazz : classes) {
Review comment:
We do have an info log in Line 163 when registering metrics factory to `PinotMetricUtils` class.
And sure, will add one warning log if more than one MetricsFactory is needed.
----------------------------------------------------------------
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] jackjlli commented on a change in pull request #6640: Make Pinot metrics pluggable
Posted by GitBox <gi...@apache.org>.
jackjlli commented on a change in pull request #6640:
URL: https://github.com/apache/incubator-pinot/pull/6640#discussion_r586927692
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/metrics/PinotMetricUtils.java
##########
@@ -18,82 +18,186 @@
*/
package org.apache.pinot.common.metrics;
+import java.lang.reflect.Constructor;
+import java.util.Arrays;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.TimeUnit;
import java.util.function.Function;
-import org.apache.pinot.common.exception.InvalidConfigException;
+import org.apache.pinot.plugin.metrics.yammer.YammerMetricsFactory;
+import org.apache.pinot.spi.annotations.metrics.MetricsFactory;
+import org.apache.pinot.spi.annotations.metrics.PinotMetricsFactory;
import org.apache.pinot.spi.env.PinotConfiguration;
import org.apache.pinot.spi.metrics.PinotGauge;
import org.apache.pinot.spi.metrics.PinotJmxReporter;
+import org.apache.pinot.spi.metrics.PinotMeter;
import org.apache.pinot.spi.metrics.PinotMetricName;
import org.apache.pinot.spi.metrics.PinotMetricsRegistry;
-import org.apache.pinot.common.metrics.yammer.YammerGauge;
-import org.apache.pinot.common.metrics.yammer.YammerJmxReporter;
-import org.apache.pinot.common.metrics.yammer.YammerMetricName;
-import org.apache.pinot.common.metrics.yammer.YammerMetricsRegistry;
+import org.apache.pinot.spi.metrics.PinotTimer;
+import org.reflections.Reflections;
+import org.reflections.scanners.TypeAnnotationsScanner;
+import org.reflections.util.ClasspathHelper;
+import org.reflections.util.ConfigurationBuilder;
+import org.reflections.util.FilterBuilder;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
public class PinotMetricUtils {
private static final Logger LOGGER = LoggerFactory.getLogger(PinotMetricUtils.class);
- public static final String LIBRARY_NAME_KEY = "libraryName";
- public static final String YAMMER_KEY = "yammer";
+ private static final String METRICS_PACKAGE_REGEX_PATTERN = ".*\\.plugin\\.metrics\\..*";
- private static String LIBRARY_TO_USE = YAMMER_KEY;
+ private static PinotMetricsFactory _pinotMetricsFactory = null;
- public static void init(PinotConfiguration metricsConfiguration)
- throws InvalidConfigException {
- String libraryName = metricsConfiguration.getProperty(PinotMetricUtils.LIBRARY_NAME_KEY);
- if (libraryName == null) {
- return;
- }
- switch (libraryName) {
- case YAMMER_KEY:
- LIBRARY_TO_USE = YAMMER_KEY;
- break;
- // TODO: support more libraries.
- default:
- throw new InvalidConfigException("PinotMetricsRegistry for " + libraryName + " cannot be initialized.");
+ private static final Map<PinotMetricsRegistry, Boolean> metricsRegistryMap = new ConcurrentHashMap<>();
+ private static final Map<MetricsRegistryRegistrationListener, Boolean> metricsRegistryRegistrationListenersMap =
+ new ConcurrentHashMap<>();
+
+ public static void init(PinotConfiguration metricsConfiguration) {
+ // Initializes PinotMetricsFactory.
+ initializePinotMetricsFactory(metricsConfiguration);
+
+ // Initializes metrics using the metrics configuration.
+ initializeMetrics(metricsConfiguration);
+ registerMetricsRegistry(getPinotMetricsRegistry());
+ }
+
+ /**
+ * Initializes PinotMetricsFactory with metrics configurations.
+ * @param metricsConfiguration The subset of the configuration containing the metrics-related keys
+ */
+ private static void initializePinotMetricsFactory(PinotConfiguration metricsConfiguration) {
+ Set<Class<?>> classes = getMetricsClasses();
+ for (Class<?> clazz : classes) {
+ MetricsFactory annotation = clazz.getAnnotation(MetricsFactory.class);
+ if (annotation.enabled()) {
+ try {
+ PinotMetricsFactory pinotMetricsFactory = (PinotMetricsFactory) clazz.newInstance();
+ pinotMetricsFactory.init(metricsConfiguration);
+ registerMetricsFactory(pinotMetricsFactory);
+ } catch (Exception e) {
+ LOGGER.error("Caught exception while initializing pinot metrics registry: {}, skipping it", clazz, e);
+ }
+ }
}
- LOGGER.info("Setting metric library to: " + LIBRARY_TO_USE);
}
- public static PinotMetricsRegistry getPinotMetricsRegistry() {
- switch (LIBRARY_TO_USE) {
- case YAMMER_KEY:
- return new YammerMetricsRegistry();
- //TODO: support more libraries.
- default:
- return new YammerMetricsRegistry();
+ private static Set<Class<?>> getMetricsClasses() {
Review comment:
Added a new util class called `PinotReflectionUtils` here.
----------------------------------------------------------------
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] fx19880617 commented on a change in pull request #6640: Make Pinot metrics pluggable
Posted by GitBox <gi...@apache.org>.
fx19880617 commented on a change in pull request #6640:
URL: https://github.com/apache/incubator-pinot/pull/6640#discussion_r588158268
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/metrics/PinotMetricUtils.java
##########
@@ -18,82 +18,186 @@
*/
package org.apache.pinot.common.metrics;
+import com.google.common.base.Preconditions;
+import java.lang.reflect.Constructor;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.TimeUnit;
import java.util.function.Function;
-import org.apache.pinot.common.exception.InvalidConfigException;
+import org.apache.pinot.spi.annotations.metrics.MetricsFactory;
+import org.apache.pinot.spi.annotations.metrics.PinotMetricsFactory;
import org.apache.pinot.spi.env.PinotConfiguration;
import org.apache.pinot.spi.metrics.PinotGauge;
import org.apache.pinot.spi.metrics.PinotJmxReporter;
+import org.apache.pinot.spi.metrics.PinotMeter;
import org.apache.pinot.spi.metrics.PinotMetricName;
import org.apache.pinot.spi.metrics.PinotMetricsRegistry;
-import org.apache.pinot.common.metrics.yammer.YammerGauge;
-import org.apache.pinot.common.metrics.yammer.YammerJmxReporter;
-import org.apache.pinot.common.metrics.yammer.YammerMetricName;
-import org.apache.pinot.common.metrics.yammer.YammerMetricsRegistry;
+import org.apache.pinot.spi.metrics.PinotTimer;
+import org.apache.pinot.spi.utils.PinotReflectionUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
public class PinotMetricUtils {
private static final Logger LOGGER = LoggerFactory.getLogger(PinotMetricUtils.class);
- public static final String LIBRARY_NAME_KEY = "libraryName";
- public static final String YAMMER_KEY = "yammer";
+ private static final String METRICS_PACKAGE_REGEX_PATTERN = ".*\\.plugin\\.metrics\\..*";
- private static String LIBRARY_TO_USE = YAMMER_KEY;
+ private static PinotMetricsFactory _pinotMetricsFactory = null;
- public static void init(PinotConfiguration metricsConfiguration)
- throws InvalidConfigException {
- String libraryName = metricsConfiguration.getProperty(PinotMetricUtils.LIBRARY_NAME_KEY);
- if (libraryName == null) {
- return;
- }
- switch (libraryName) {
- case YAMMER_KEY:
- LIBRARY_TO_USE = YAMMER_KEY;
- break;
- // TODO: support more libraries.
- default:
- throw new InvalidConfigException("PinotMetricsRegistry for " + libraryName + " cannot be initialized.");
+ private static final Map<PinotMetricsRegistry, Boolean> metricsRegistryMap = new ConcurrentHashMap<>();
+ private static final Map<MetricsRegistryRegistrationListener, Boolean> metricsRegistryRegistrationListenersMap =
+ new ConcurrentHashMap<>();
+
+ public static void init(PinotConfiguration metricsConfiguration) {
+ // Initializes PinotMetricsFactory.
+ initializePinotMetricsFactory(metricsConfiguration);
+
+ // Initializes metrics using the metrics configuration.
+ initializeMetrics(metricsConfiguration);
+ registerMetricsRegistry(getPinotMetricsRegistry());
+ }
+
+ /**
+ * Initializes PinotMetricsFactory with metrics configurations.
+ * @param metricsConfiguration The subset of the configuration containing the metrics-related keys
+ */
+ private static void initializePinotMetricsFactory(PinotConfiguration metricsConfiguration) {
+ Set<Class<?>> classes = getPinotMetricsFactoryClasses();
+ for (Class<?> clazz : classes) {
Review comment:
Can we support multiple MetricsFactory classes? What's the behavior?
----------------------------------------------------------------
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] fx19880617 commented on a change in pull request #6640: Make Pinot metrics pluggable
Posted by GitBox <gi...@apache.org>.
fx19880617 commented on a change in pull request #6640:
URL: https://github.com/apache/incubator-pinot/pull/6640#discussion_r588164301
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/annotations/metrics/MetricsFactory.java
##########
@@ -0,0 +1,39 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.spi.annotations.metrics;
+
+import java.lang.annotation.ElementType;
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+import java.lang.annotation.Target;
+
+
+/**
+ * Annotation for Pinot metrics.
+ *
+ * NOTE:
+ * - The annotated class must implement the MinionEventObserverFactory interface
+ * - The annotated class must be under the package of name 'org.apache.pinot.*.metrics.*' to be auto-registered.
Review comment:
Should this be: `org.apache.pinot.*.plugin.metrics.*`?
You set this:
```
METRICS_PACKAGE_REGEX_PATTERN = ".*\\.plugin\\.metrics\\..*"
```
----------------------------------------------------------------
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] jackjlli commented on a change in pull request #6640: Make Pinot metrics pluggable
Posted by GitBox <gi...@apache.org>.
jackjlli commented on a change in pull request #6640:
URL: https://github.com/apache/incubator-pinot/pull/6640#discussion_r588538093
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/metrics/PinotMetricUtils.java
##########
@@ -18,82 +18,186 @@
*/
package org.apache.pinot.common.metrics;
+import com.google.common.base.Preconditions;
+import java.lang.reflect.Constructor;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.TimeUnit;
import java.util.function.Function;
-import org.apache.pinot.common.exception.InvalidConfigException;
+import org.apache.pinot.spi.annotations.metrics.MetricsFactory;
+import org.apache.pinot.spi.annotations.metrics.PinotMetricsFactory;
import org.apache.pinot.spi.env.PinotConfiguration;
import org.apache.pinot.spi.metrics.PinotGauge;
import org.apache.pinot.spi.metrics.PinotJmxReporter;
+import org.apache.pinot.spi.metrics.PinotMeter;
import org.apache.pinot.spi.metrics.PinotMetricName;
import org.apache.pinot.spi.metrics.PinotMetricsRegistry;
-import org.apache.pinot.common.metrics.yammer.YammerGauge;
-import org.apache.pinot.common.metrics.yammer.YammerJmxReporter;
-import org.apache.pinot.common.metrics.yammer.YammerMetricName;
-import org.apache.pinot.common.metrics.yammer.YammerMetricsRegistry;
+import org.apache.pinot.spi.metrics.PinotTimer;
+import org.apache.pinot.spi.utils.PinotReflectionUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
public class PinotMetricUtils {
private static final Logger LOGGER = LoggerFactory.getLogger(PinotMetricUtils.class);
- public static final String LIBRARY_NAME_KEY = "libraryName";
- public static final String YAMMER_KEY = "yammer";
+ private static final String METRICS_PACKAGE_REGEX_PATTERN = ".*\\.plugin\\.metrics\\..*";
- private static String LIBRARY_TO_USE = YAMMER_KEY;
+ private static PinotMetricsFactory _pinotMetricsFactory = null;
- public static void init(PinotConfiguration metricsConfiguration)
- throws InvalidConfigException {
- String libraryName = metricsConfiguration.getProperty(PinotMetricUtils.LIBRARY_NAME_KEY);
- if (libraryName == null) {
- return;
- }
- switch (libraryName) {
- case YAMMER_KEY:
- LIBRARY_TO_USE = YAMMER_KEY;
- break;
- // TODO: support more libraries.
- default:
- throw new InvalidConfigException("PinotMetricsRegistry for " + libraryName + " cannot be initialized.");
+ private static final Map<PinotMetricsRegistry, Boolean> metricsRegistryMap = new ConcurrentHashMap<>();
+ private static final Map<MetricsRegistryRegistrationListener, Boolean> metricsRegistryRegistrationListenersMap =
+ new ConcurrentHashMap<>();
+
+ public static void init(PinotConfiguration metricsConfiguration) {
+ // Initializes PinotMetricsFactory.
+ initializePinotMetricsFactory(metricsConfiguration);
+
+ // Initializes metrics using the metrics configuration.
+ initializeMetrics(metricsConfiguration);
+ registerMetricsRegistry(getPinotMetricsRegistry());
+ }
+
+ /**
+ * Initializes PinotMetricsFactory with metrics configurations.
+ * @param metricsConfiguration The subset of the configuration containing the metrics-related keys
+ */
+ private static void initializePinotMetricsFactory(PinotConfiguration metricsConfiguration) {
+ Set<Class<?>> classes = getPinotMetricsFactoryClasses();
+ for (Class<?> clazz : classes) {
Review comment:
In theory, yes we can support multiple MetricsFactory classes. But I don't think it's necessary for most of the use cases, since usually there is only one metrics system got hook to Pinot cluster. I think it'll make more sense to replace a old metric with a new one, rather than keeping multiple running ones. That's why I only put one static MetricsFactory constant in `PinotMetricUtils` class.
----------------------------------------------------------------
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] fx19880617 commented on a change in pull request #6640: Make Pinot metrics pluggable
Posted by GitBox <gi...@apache.org>.
fx19880617 commented on a change in pull request #6640:
URL: https://github.com/apache/incubator-pinot/pull/6640#discussion_r588161265
##########
File path: pinot-distribution/pinot-assembly.xml
##########
@@ -123,11 +123,17 @@
</file>
<!-- End Include Pinot Input Format Plugins-->
<!-- Start Include Pinot Minion Tasks Plugins-->
+ <file>
Review comment:
Switched the comments of minion and metrics plugins
##########
File path: pinot-distribution/pinot-assembly.xml
##########
@@ -123,11 +123,17 @@
</file>
<!-- End Include Pinot Input Format Plugins-->
<!-- Start Include Pinot Minion Tasks Plugins-->
+ <file>
Review comment:
Switch the comments of minion and metrics plugins
----------------------------------------------------------------
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] jackjlli commented on a change in pull request #6640: Make Pinot metrics pluggable
Posted by GitBox <gi...@apache.org>.
jackjlli commented on a change in pull request #6640:
URL: https://github.com/apache/incubator-pinot/pull/6640#discussion_r586927835
##########
File path: pinot-plugins/pinot-metrics/pinot-yammer/pom.xml
##########
@@ -0,0 +1,69 @@
+<?xml version="1.0"?>
+<!--
+
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+
+-->
+<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+ <modelVersion>4.0.0</modelVersion>
+ <parent>
+ <artifactId>pinot-metrics</artifactId>
+ <groupId>org.apache.pinot</groupId>
+ <version>0.7.0-SNAPSHOT</version>
+ <relativePath>..</relativePath>
+ </parent>
+
+ <artifactId>pinot-yammer</artifactId>
+ <name>Pinot Yammer Metrics</name>
+ <url>https://pinot.apache.org/</url>
+ <properties>
+ <pinot.root>${basedir}/../../..</pinot.root>
+ <phase.prop>package</phase.prop>
+ </properties>
+
+ <build>
+ <plugins>
+ <plugin>
+ <artifactId>maven-jar-plugin</artifactId>
+ <executions>
+ <execution>
+ <goals>
+ <goal>test-jar</goal>
+ </goals>
+ </execution>
+ </executions>
+ </plugin>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-enforcer-plugin</artifactId>
+ </plugin>
+ </plugins>
+ </build>
+
+ <dependencies>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-spi</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>com.yammer.metrics</groupId>
+ <artifactId>metrics-core</artifactId>
Review comment:
Good idea. Updated.
----------------------------------------------------------------
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] jackjlli commented on pull request #6640: Make Pinot metrics pluggable
Posted by GitBox <gi...@apache.org>.
jackjlli commented on pull request #6640:
URL: https://github.com/apache/incubator-pinot/pull/6640#issuecomment-790198656
> Also, add the backward-incompatible tag and mention that for users who override the `plugins.include` env variable, they need to add `pinot-yammer` to it in order to get metrics reported.
>
> Or we can try to look for `pinot-yammer` in PluginManager init() method if no metrics plugin is specified, then it should be backward compatible.
>
For now I put pinot-yammer under pinot-common.
> Also, add the backward-incompatible tag and mention that for users who override the `plugins.include` env variable, they need to add `pinot-yammer` to it in order to get metrics reported.
>
> Or we can try to look for `pinot-yammer` in PluginManager init() method if no metrics plugin is specified, then it should be backward compatible.
PR descriptions updated and pinot-assembly.xml adjusted.
----------------------------------------------------------------
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] fx19880617 commented on pull request #6640: Make Pinot metrics pluggable
Posted by GitBox <gi...@apache.org>.
fx19880617 commented on pull request #6640:
URL: https://github.com/apache/incubator-pinot/pull/6640#issuecomment-790176864
please also update assemble file in pinot-distribution to ensure the shaded jar is packaged correctly. Ref: https://github.com/apache/incubator-pinot/pull/6618/files#diff-c003a08155cf1408ff891cb4ab85245b5c6e49f737aa4eca86c893f6f87a1d8b
----------------------------------------------------------------
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] jackjlli commented on a change in pull request #6640: Make Pinot metrics pluggable
Posted by GitBox <gi...@apache.org>.
jackjlli commented on a change in pull request #6640:
URL: https://github.com/apache/incubator-pinot/pull/6640#discussion_r588542959
##########
File path: pinot-distribution/pinot-assembly.xml
##########
@@ -123,11 +123,17 @@
</file>
<!-- End Include Pinot Input Format Plugins-->
<!-- Start Include Pinot Minion Tasks Plugins-->
+ <file>
Review comment:
Good catch! π Updated.
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/annotations/metrics/MetricsFactory.java
##########
@@ -0,0 +1,39 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.spi.annotations.metrics;
+
+import java.lang.annotation.ElementType;
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+import java.lang.annotation.Target;
+
+
+/**
+ * Annotation for Pinot metrics.
+ *
+ * NOTE:
+ * - The annotated class must implement the MinionEventObserverFactory interface
+ * - The annotated class must be under the package of name 'org.apache.pinot.*.metrics.*' to be auto-registered.
Review comment:
Updated.
----------------------------------------------------------------
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] fx19880617 commented on pull request #6640: Make Pinot metrics pluggable
Posted by GitBox <gi...@apache.org>.
fx19880617 commented on pull request #6640:
URL: https://github.com/apache/incubator-pinot/pull/6640#issuecomment-790179122
Also, add the backward-incompatible tag and mention that for users who override the `plugins.include` env variable, they need to add `pinot-yammer` to it in order to get metrics reported.
Or we can try to look for `pinot-yammer` in PluginManager init() method if no metrics plugin is specified, then it should be backward compatible
.
----------------------------------------------------------------
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] fx19880617 commented on pull request #6640: Make Pinot metrics pluggable
Posted by GitBox <gi...@apache.org>.
fx19880617 commented on pull request #6640:
URL: https://github.com/apache/incubator-pinot/pull/6640#issuecomment-791305421
@chenboat This is an important step towards pluggable metrics. I think you can create pinot-dropwizard very soon.
----------------------------------------------------------------
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] fx19880617 commented on a change in pull request #6640: Make Pinot metrics pluggable
Posted by GitBox <gi...@apache.org>.
fx19880617 commented on a change in pull request #6640:
URL: https://github.com/apache/incubator-pinot/pull/6640#discussion_r586909138
##########
File path: pinot-plugins/pinot-metrics/pinot-yammer/pom.xml
##########
@@ -0,0 +1,69 @@
+<?xml version="1.0"?>
+<!--
+
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+
+-->
+<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+ <modelVersion>4.0.0</modelVersion>
+ <parent>
+ <artifactId>pinot-metrics</artifactId>
+ <groupId>org.apache.pinot</groupId>
+ <version>0.7.0-SNAPSHOT</version>
+ <relativePath>..</relativePath>
+ </parent>
+
+ <artifactId>pinot-yammer</artifactId>
+ <name>Pinot Yammer Metrics</name>
+ <url>https://pinot.apache.org/</url>
+ <properties>
+ <pinot.root>${basedir}/../../..</pinot.root>
+ <phase.prop>package</phase.prop>
+ </properties>
+
+ <build>
+ <plugins>
+ <plugin>
+ <artifactId>maven-jar-plugin</artifactId>
+ <executions>
+ <execution>
+ <goals>
+ <goal>test-jar</goal>
+ </goals>
+ </execution>
+ </executions>
+ </plugin>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-enforcer-plugin</artifactId>
+ </plugin>
+ </plugins>
+ </build>
+
+ <dependencies>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-spi</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>com.yammer.metrics</groupId>
+ <artifactId>metrics-core</artifactId>
Review comment:
shall we remove yammer from root pom and specify a version here?
----------------------------------------------------------------
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] fx19880617 commented on a change in pull request #6640: Make Pinot metrics pluggable
Posted by GitBox <gi...@apache.org>.
fx19880617 commented on a change in pull request #6640:
URL: https://github.com/apache/incubator-pinot/pull/6640#discussion_r588708121
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/metrics/PinotMetricUtils.java
##########
@@ -18,82 +18,186 @@
*/
package org.apache.pinot.common.metrics;
+import com.google.common.base.Preconditions;
+import java.lang.reflect.Constructor;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.TimeUnit;
import java.util.function.Function;
-import org.apache.pinot.common.exception.InvalidConfigException;
+import org.apache.pinot.spi.annotations.metrics.MetricsFactory;
+import org.apache.pinot.spi.annotations.metrics.PinotMetricsFactory;
import org.apache.pinot.spi.env.PinotConfiguration;
import org.apache.pinot.spi.metrics.PinotGauge;
import org.apache.pinot.spi.metrics.PinotJmxReporter;
+import org.apache.pinot.spi.metrics.PinotMeter;
import org.apache.pinot.spi.metrics.PinotMetricName;
import org.apache.pinot.spi.metrics.PinotMetricsRegistry;
-import org.apache.pinot.common.metrics.yammer.YammerGauge;
-import org.apache.pinot.common.metrics.yammer.YammerJmxReporter;
-import org.apache.pinot.common.metrics.yammer.YammerMetricName;
-import org.apache.pinot.common.metrics.yammer.YammerMetricsRegistry;
+import org.apache.pinot.spi.metrics.PinotTimer;
+import org.apache.pinot.spi.utils.PinotReflectionUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
public class PinotMetricUtils {
private static final Logger LOGGER = LoggerFactory.getLogger(PinotMetricUtils.class);
- public static final String LIBRARY_NAME_KEY = "libraryName";
- public static final String YAMMER_KEY = "yammer";
+ private static final String METRICS_PACKAGE_REGEX_PATTERN = ".*\\.plugin\\.metrics\\..*";
- private static String LIBRARY_TO_USE = YAMMER_KEY;
+ private static PinotMetricsFactory _pinotMetricsFactory = null;
- public static void init(PinotConfiguration metricsConfiguration)
- throws InvalidConfigException {
- String libraryName = metricsConfiguration.getProperty(PinotMetricUtils.LIBRARY_NAME_KEY);
- if (libraryName == null) {
- return;
- }
- switch (libraryName) {
- case YAMMER_KEY:
- LIBRARY_TO_USE = YAMMER_KEY;
- break;
- // TODO: support more libraries.
- default:
- throw new InvalidConfigException("PinotMetricsRegistry for " + libraryName + " cannot be initialized.");
+ private static final Map<PinotMetricsRegistry, Boolean> metricsRegistryMap = new ConcurrentHashMap<>();
+ private static final Map<MetricsRegistryRegistrationListener, Boolean> metricsRegistryRegistrationListenersMap =
+ new ConcurrentHashMap<>();
+
+ public static void init(PinotConfiguration metricsConfiguration) {
+ // Initializes PinotMetricsFactory.
+ initializePinotMetricsFactory(metricsConfiguration);
+
+ // Initializes metrics using the metrics configuration.
+ initializeMetrics(metricsConfiguration);
+ registerMetricsRegistry(getPinotMetricsRegistry());
+ }
+
+ /**
+ * Initializes PinotMetricsFactory with metrics configurations.
+ * @param metricsConfiguration The subset of the configuration containing the metrics-related keys
+ */
+ private static void initializePinotMetricsFactory(PinotConfiguration metricsConfiguration) {
+ Set<Class<?>> classes = getPinotMetricsFactoryClasses();
+ for (Class<?> clazz : classes) {
Review comment:
Agreed, in this case, can you:
1. add a warning log if there are multiple PinotMetricsFactory Classes
2. add an info log of which PinotMetricsFactory we set for the static MetricsFactory constant.
This will help with running and tests new plugins.
----------------------------------------------------------------
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] jackjlli merged pull request #6640: Make Pinot metrics pluggable
Posted by GitBox <gi...@apache.org>.
jackjlli merged pull request #6640:
URL: https://github.com/apache/incubator-pinot/pull/6640
----------------------------------------------------------------
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] codecov-io edited a comment on pull request #6640: Make Pinot metrics pluggable
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #6640:
URL: https://github.com/apache/incubator-pinot/pull/6640#issuecomment-790374407
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6640?src=pr&el=h1) Report
> Merging [#6640](https://codecov.io/gh/apache/incubator-pinot/pull/6640?src=pr&el=desc) (5a3c787) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/1beaab59b73f26c4e35f3b9bc856b03806cddf5a?el=desc) (1beaab5) will **decrease** coverage by `22.52%`.
> The diff coverage is `42.89%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/6640?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #6640 +/- ##
===========================================
- Coverage 66.44% 43.91% -22.53%
===========================================
Files 1075 1361 +286
Lines 54773 66517 +11744
Branches 8168 9702 +1534
===========================================
- Hits 36396 29214 -7182
- Misses 15700 34852 +19152
+ Partials 2677 2451 -226
```
| Flag | Coverage Ξ | |
|---|---|---|
| integration | `43.91% <42.89%> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/6640?src=pr&el=tree) | Coverage Ξ | |
|---|---|---|
| [...ot/broker/broker/AllowAllAccessControlFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6640/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL0FsbG93QWxsQWNjZXNzQ29udHJvbEZhY3RvcnkuamF2YQ==) | `100.00% <ΓΈ> (ΓΈ)` | |
| [...t/broker/broker/BasicAuthAccessControlFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6640/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL0Jhc2ljQXV0aEFjY2Vzc0NvbnRyb2xGYWN0b3J5LmphdmE=) | `0.00% <0.00%> (ΓΈ)` | |
| [.../helix/BrokerUserDefinedMessageHandlerFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6640/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0Jyb2tlclVzZXJEZWZpbmVkTWVzc2FnZUhhbmRsZXJGYWN0b3J5LmphdmE=) | `52.83% <0.00%> (-13.84%)` | :arrow_down: |
| [...org/apache/pinot/broker/queryquota/HitCounter.java](https://codecov.io/gh/apache/incubator-pinot/pull/6640/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcXVlcnlxdW90YS9IaXRDb3VudGVyLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...che/pinot/broker/queryquota/MaxHitRateTracker.java](https://codecov.io/gh/apache/incubator-pinot/pull/6640/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcXVlcnlxdW90YS9NYXhIaXRSYXRlVHJhY2tlci5qYXZh) | `0.00% <0.00%> (ΓΈ)` | |
| [...ache/pinot/broker/queryquota/QueryQuotaEntity.java](https://codecov.io/gh/apache/incubator-pinot/pull/6640/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcXVlcnlxdW90YS9RdWVyeVF1b3RhRW50aXR5LmphdmE=) | `0.00% <0.00%> (-50.00%)` | :arrow_down: |
| [...ker/routing/instanceselector/InstanceSelector.java](https://codecov.io/gh/apache/incubator-pinot/pull/6640/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL0luc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `100.00% <ΓΈ> (ΓΈ)` | |
| [...ceselector/StrictReplicaGroupInstanceSelector.java](https://codecov.io/gh/apache/incubator-pinot/pull/6640/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL1N0cmljdFJlcGxpY2FHcm91cEluc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `0.00% <0.00%> (ΓΈ)` | |
| [...roker/routing/segmentpruner/TimeSegmentPruner.java](https://codecov.io/gh/apache/incubator-pinot/pull/6640/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9zZWdtZW50cHJ1bmVyL1RpbWVTZWdtZW50UHJ1bmVyLmphdmE=) | `0.00% <0.00%> (ΓΈ)` | |
| [...roker/routing/segmentpruner/interval/Interval.java](https://codecov.io/gh/apache/incubator-pinot/pull/6640/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9zZWdtZW50cHJ1bmVyL2ludGVydmFsL0ludGVydmFsLmphdmE=) | `0.00% <0.00%> (ΓΈ)` | |
| ... and [1391 more](https://codecov.io/gh/apache/incubator-pinot/pull/6640/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6640?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Ξ = absolute <relative> (impact)`, `ΓΈ = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6640?src=pr&el=footer). Last update [5137025...5a3c787](https://codecov.io/gh/apache/incubator-pinot/pull/6640?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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] codecov-io commented on pull request #6640: Make Pinot metrics pluggable
Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #6640:
URL: https://github.com/apache/incubator-pinot/pull/6640#issuecomment-790374407
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6640?src=pr&el=h1) Report
> Merging [#6640](https://codecov.io/gh/apache/incubator-pinot/pull/6640?src=pr&el=desc) (b190a45) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/1beaab59b73f26c4e35f3b9bc856b03806cddf5a?el=desc) (1beaab5) will **decrease** coverage by `0.62%`.
> The diff coverage is `62.61%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/6640?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #6640 +/- ##
==========================================
- Coverage 66.44% 65.82% -0.63%
==========================================
Files 1075 1361 +286
Lines 54773 66515 +11742
Branches 8168 9697 +1529
==========================================
+ Hits 36396 43786 +7390
- Misses 15700 19607 +3907
- Partials 2677 3122 +445
```
| Flag | Coverage Ξ | |
|---|---|---|
| unittests | `65.82% <62.61%> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/6640?src=pr&el=tree) | Coverage Ξ | |
|---|---|---|
| [...e/pinot/broker/api/resources/PinotBrokerDebug.java](https://codecov.io/gh/apache/incubator-pinot/pull/6640/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYXBpL3Jlc291cmNlcy9QaW5vdEJyb2tlckRlYnVnLmphdmE=) | `0.00% <0.00%> (-79.32%)` | :arrow_down: |
| [...pinot/broker/api/resources/PinotClientRequest.java](https://codecov.io/gh/apache/incubator-pinot/pull/6640/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYXBpL3Jlc291cmNlcy9QaW5vdENsaWVudFJlcXVlc3QuamF2YQ==) | `0.00% <0.00%> (-27.28%)` | :arrow_down: |
| [...ot/broker/broker/AllowAllAccessControlFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6640/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL0FsbG93QWxsQWNjZXNzQ29udHJvbEZhY3RvcnkuamF2YQ==) | `71.42% <ΓΈ> (-28.58%)` | :arrow_down: |
| [.../helix/BrokerUserDefinedMessageHandlerFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6640/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0Jyb2tlclVzZXJEZWZpbmVkTWVzc2FnZUhhbmRsZXJGYWN0b3J5LmphdmE=) | `33.96% <0.00%> (-32.71%)` | :arrow_down: |
| [...ker/routing/instanceselector/InstanceSelector.java](https://codecov.io/gh/apache/incubator-pinot/pull/6640/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL0luc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `100.00% <ΓΈ> (ΓΈ)` | |
| [...ava/org/apache/pinot/client/AbstractResultSet.java](https://codecov.io/gh/apache/incubator-pinot/pull/6640/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0Fic3RyYWN0UmVzdWx0U2V0LmphdmE=) | `66.66% <ΓΈ> (+9.52%)` | :arrow_up: |
| [...n/java/org/apache/pinot/client/BrokerResponse.java](https://codecov.io/gh/apache/incubator-pinot/pull/6640/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0Jyb2tlclJlc3BvbnNlLmphdmE=) | `100.00% <ΓΈ> (ΓΈ)` | |
| [.../main/java/org/apache/pinot/client/Connection.java](https://codecov.io/gh/apache/incubator-pinot/pull/6640/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0Nvbm5lY3Rpb24uamF2YQ==) | `35.55% <ΓΈ> (-13.29%)` | :arrow_down: |
| [...org/apache/pinot/client/DynamicBrokerSelector.java](https://codecov.io/gh/apache/incubator-pinot/pull/6640/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0R5bmFtaWNCcm9rZXJTZWxlY3Rvci5qYXZh) | `82.85% <ΓΈ> (+10.12%)` | :arrow_up: |
| [...n/java/org/apache/pinot/client/ExecutionStats.java](https://codecov.io/gh/apache/incubator-pinot/pull/6640/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0V4ZWN1dGlvblN0YXRzLmphdmE=) | `68.88% <ΓΈ> (ΓΈ)` | |
| ... and [1246 more](https://codecov.io/gh/apache/incubator-pinot/pull/6640/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6640?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Ξ = absolute <relative> (impact)`, `ΓΈ = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6640?src=pr&el=footer). Last update [3fb68ee...b190a45](https://codecov.io/gh/apache/incubator-pinot/pull/6640?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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