You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@phoenix.apache.org by karanmehta93 <gi...@git.apache.org> on 2018/07/27 08:58:20 UTC
[GitHub] phoenix pull request #315: PHOENIX-3655 Global Phoenix Client Metrics for PQ...
GitHub user karanmehta93 opened a pull request:
https://github.com/apache/phoenix/pull/315
PHOENIX-3655 Global Phoenix Client Metrics for PQS
@joshelser Please review.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/karanmehta93/phoenix 4.x-HBase-1.4
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/phoenix/pull/315.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #315
----
commit a8f7f25b57e9e7f6abbbfc2c70bb9648ca872761
Author: Karan Mehta <ka...@...>
Date: 2018-07-27T08:57:33Z
PHOENIX-3655 Global Phoenix Client Metrics for PQS
----
---
[GitHub] phoenix pull request #315: PHOENIX-3655 Global Phoenix Client Metrics for PQ...
Posted by karanmehta93 <gi...@git.apache.org>.
Github user karanmehta93 commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/315#discussion_r206973402
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/monitoring/GlobalMetricRegistriesAdapter.java ---
@@ -0,0 +1,167 @@
+/*
+ * 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.phoenix.monitoring;
+
+import java.util.Iterator;
+import java.util.Map;
+import java.util.Map.Entry;
+import org.apache.commons.lang.StringUtils;
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
+import org.apache.hadoop.hbase.metrics.Counter;
+import org.apache.hadoop.hbase.metrics.Gauge;
+import org.apache.hadoop.hbase.metrics.Histogram;
+import org.apache.hadoop.hbase.metrics.Meter;
+import org.apache.hadoop.hbase.metrics.Metric;
+import org.apache.hadoop.hbase.metrics.MetricRegistry;
+import org.apache.hadoop.hbase.metrics.MetricRegistryInfo;
+import org.apache.hadoop.hbase.metrics.Timer;
+import org.apache.hadoop.metrics2.MetricsCollector;
+import org.apache.hadoop.metrics2.MetricsInfo;
+import org.apache.hadoop.metrics2.MetricsRecordBuilder;
+import org.apache.hadoop.metrics2.MetricsSource;
+import org.apache.hadoop.metrics2.lib.DefaultMetricsSystem;
+import org.apache.hadoop.metrics2.lib.Interns;
+import org.apache.hadoop.metrics2.lib.MutableHistogram;
+import org.apache.hadoop.metrics2.source.JvmMetrics;
+
+/**
+ * Contents mostly copied from GlobalMetricRegistriesAdapter class from hbase-hadoop2-compat
+ * The adapter attaches HBase's MetricRegistry to Hadoop's DefaultMetricsSystem
+ * Doesn't handle dynamic attach/detach of registries
+ */
+public class GlobalMetricRegistriesAdapter {
+
+ private static final Log LOG = LogFactory.getLog(GlobalMetricRegistriesAdapter.class);
+ private static GlobalMetricRegistriesAdapter INSTANCE;
+
+ private GlobalMetricRegistriesAdapter() {
+ DefaultMetricsSystem.initialize("HBase");
+ JvmMetrics.initSingleton("HBase", "");
--- End diff --
I think its hard to differentiate, but my concern is why do we need to differentiate. We should get that info from the application JVM itself right, whether its PQS or some app server using Phoenix Client.
---
[GitHub] phoenix pull request #315: PHOENIX-3655 Global Phoenix Client Metrics for PQ...
Posted by karanmehta93 <gi...@git.apache.org>.
Github user karanmehta93 commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/315#discussion_r205901029
--- Diff: phoenix-core/src/it/java/org/apache/phoenix/monitoring/PhoenixMetricsIT.java ---
@@ -204,6 +219,39 @@ private static void resetGlobalMetrics() {
}
}
+ private boolean verifyMetricsFromSink() throws InterruptedException {
+ Map<String, Long> expectedMetrics = new HashMap<>();
+ for (GlobalMetric m : PhoenixRuntime.getGlobalPhoenixClientMetrics()) {
+ expectedMetrics.put(m.getMetricType().name(), m.getTotalSum());
+ }
+
+ for (int i = 0; i < MAX_RETRIES; i++) {
+ LOG.info("Verifying Global Metrics from Hadoop Sink, Retry: " + (i + 1));
+ if (verifyMetricsFromSinkOnce(expectedMetrics)) {
+ LOG.info("Values from Hadoop Metrics Sink match actual values");
+ return true;
+ }
+ Thread.sleep(1000);
+ }
+ return false;
+ }
+
+ private boolean verifyMetricsFromSinkOnce(Map<String, Long> expectedMetrics) {
+ synchronized (GlobalPhoenixMetricsTestSink.lock) {
+ for (AbstractMetric metric : GlobalPhoenixMetricsTestSink.metrics) {
+ if (expectedMetrics.containsKey(metric.name())) {
--- End diff --
I will also soon add a test that disables these metrics as well.
> I'd switch this around to iterate over your expectations, ensuring that they all exist.
The reason why I didn't do it that way because I didn't wanted to iterate over the it for every expected metric. It doesn't really matter since its a test though. One approach can be to remove each entry from the map and check if the size is 0 at the end. How does that sound?
---
[GitHub] phoenix pull request #315: PHOENIX-3655 Global Phoenix Client Metrics for PQ...
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/315#discussion_r206597366
--- Diff: phoenix-core/src/it/java/org/apache/phoenix/monitoring/PhoenixMetricsIT.java ---
@@ -204,6 +230,45 @@ private static void resetGlobalMetrics() {
}
}
+ // Phoenix Client Metrics are transported via Hadoop-metrics2 sink
+ // The test sink is defined at GlobalPhoenixMetricsTestSink
+ // Configuration for Hadoop-metrics2 comes from hadoop-metrics2.properties file located in test/resources
+ private boolean verifyMetricsFromSink() throws InterruptedException {
+ Map<String, Long> expectedMetrics = new HashMap<>();
+ for (GlobalMetric m : PhoenixRuntime.getGlobalPhoenixClientMetrics()) {
+ expectedMetrics.put(m.getMetricType().name(), m.getTotalSum());
+ }
+
+ for (int i = 0; i < MAX_RETRIES; i++) {
+ LOG.info("Verifying Global Metrics from Hadoop Sink, Retry: " + (i + 1));
+ if (verifyMetricsFromSinkOnce(expectedMetrics)) {
+ LOG.info("Values from Hadoop Metrics Sink match actual values");
+ return true;
+ }
+ Thread.sleep(1000);
+ }
+ return false;
+ }
+
+ private boolean verifyMetricsFromSinkOnce(Map<String, Long> expectedMetrics) {
+ synchronized (GlobalPhoenixMetricsTestSink.lock) {
+ for (AbstractMetric metric : GlobalPhoenixMetricsTestSink.metrics) {
+ if (expectedMetrics.containsKey(metric.name())) {
--- End diff --
I thought there was an issue with unboxing the value from `expectedMetrics.get(metric.name())`.
I think it would be cleaner to do:
```
Long value = expectedMetrics.get(metric.name());
if (value != null) {
long expectedValue = value.longValue();
...
```
---
[GitHub] phoenix issue #315: PHOENIX-3655 Global Phoenix Client Metrics for PQS
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on the issue:
https://github.com/apache/phoenix/pull/315
> It's just that some project using PQS wants to change configuration via code, It wont be possible since HBaseConfiguration.create() will only read XML on the class path
Downstream users would instantiate QueryServer directly, not call `main()`. Still, the bigger issue is the ConfigurationFactory being ineffective.
---
[GitHub] phoenix issue #315: PHOENIX-3655 Global Phoenix Client Metrics for PQS
Posted by karanmehta93 <gi...@git.apache.org>.
Github user karanmehta93 commented on the issue:
https://github.com/apache/phoenix/pull/315
@joshelser I was trying out this patch with an internal PQS version. All these metrics are pushed to JMX and we monitor those via OpenTSDB tcollectors. I haven't done any perf-test here, but I can create a "rate of" on the metrics to track the increase in values over time. Still working on some more metrics patches internally before I can see this end to end. From @twdsilva and @apurtell experience, seems like these metrics should help monitor the health and usage of PQS.
---
[GitHub] phoenix issue #315: PHOENIX-3655 Global Phoenix Client Metrics for PQS
Posted by karanmehta93 <gi...@git.apache.org>.
Github user karanmehta93 commented on the issue:
https://github.com/apache/phoenix/pull/315
@joshelser I think the only question pending in this PR is if we want JVMMetrics or not, which are crucial from PQS POV, otherwise not really for a regular client. We can discuss what approach to finalize and then I can commit this PR. Wdyt?
---
[GitHub] phoenix pull request #315: PHOENIX-3655 Global Phoenix Client Metrics for PQ...
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/315#discussion_r205884792
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/monitoring/GlobalClientMetrics.java ---
@@ -108,40 +114,88 @@
GLOBAL_HBASE_COUNT_ROWS_SCANNED(COUNT_ROWS_SCANNED),
GLOBAL_HBASE_COUNT_ROWS_FILTERED(COUNT_ROWS_FILTERED);
-
+ private static final Logger LOG = LoggerFactory.getLogger(GlobalClientMetrics.class);
private static final boolean isGlobalMetricsEnabled = QueryServicesOptions.withDefaults().isGlobalMetricsEnabled();
+ private MetricType metricType;
private GlobalMetric metric;
- public void update(long value) {
+ static {
+ initPhoenixGlobalClientMetrics();
if (isGlobalMetricsEnabled) {
- metric.change(value);
+ MetricRegistry metricRegistry = createMetricRegistry();
+ registerPhoenixMetricsToRegistry(metricRegistry);
+ GlobalMetricRegistriesAdapter.getInstance().registerMetricRegistry(metricRegistry);
+ }
+ }
+
+ private static void initPhoenixGlobalClientMetrics() {
+ for (GlobalClientMetrics globalMetric : GlobalClientMetrics.values()) {
+ globalMetric.metric = isGlobalMetricsEnabled ?
+ new GlobalMetricImpl(globalMetric.metricType) : new NoOpGlobalMetricImpl();
+ }
+ }
+
+ private static void registerPhoenixMetricsToRegistry(MetricRegistry metricRegistry) {
+ for (GlobalClientMetrics globalMetric : GlobalClientMetrics.values()) {
+ metricRegistry.register(globalMetric.metricType.columnName(),
+ new PhoenixGlobalMetricGauge(globalMetric.metric));
+ }
+ }
+
+ private static MetricRegistry createMetricRegistry() {
+ LOG.info("Creating Metric Registry for Phoenix Global Metrics");
+ MetricRegistryInfo registryInfo = new MetricRegistryInfo("PHOENIX", "Phoenix Global Metrics",
--- End diff --
What does "Global" mean in this context? Is "Phoenix Client Metrics" more reasonable? Maybe "Phoenix,sub=CLIENT" down below?
---
[GitHub] phoenix pull request #315: PHOENIX-3655 Global Phoenix Client Metrics for PQ...
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/315#discussion_r206971126
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/monitoring/GlobalMetricRegistriesAdapter.java ---
@@ -0,0 +1,167 @@
+/*
+ * 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.phoenix.monitoring;
+
+import java.util.Iterator;
+import java.util.Map;
+import java.util.Map.Entry;
+import org.apache.commons.lang.StringUtils;
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
+import org.apache.hadoop.hbase.metrics.Counter;
+import org.apache.hadoop.hbase.metrics.Gauge;
+import org.apache.hadoop.hbase.metrics.Histogram;
+import org.apache.hadoop.hbase.metrics.Meter;
+import org.apache.hadoop.hbase.metrics.Metric;
+import org.apache.hadoop.hbase.metrics.MetricRegistry;
+import org.apache.hadoop.hbase.metrics.MetricRegistryInfo;
+import org.apache.hadoop.hbase.metrics.Timer;
+import org.apache.hadoop.metrics2.MetricsCollector;
+import org.apache.hadoop.metrics2.MetricsInfo;
+import org.apache.hadoop.metrics2.MetricsRecordBuilder;
+import org.apache.hadoop.metrics2.MetricsSource;
+import org.apache.hadoop.metrics2.lib.DefaultMetricsSystem;
+import org.apache.hadoop.metrics2.lib.Interns;
+import org.apache.hadoop.metrics2.lib.MutableHistogram;
+import org.apache.hadoop.metrics2.source.JvmMetrics;
+
+/**
+ * Contents mostly copied from GlobalMetricRegistriesAdapter class from hbase-hadoop2-compat
+ * The adapter attaches HBase's MetricRegistry to Hadoop's DefaultMetricsSystem
+ * Doesn't handle dynamic attach/detach of registries
+ */
+public class GlobalMetricRegistriesAdapter {
+
+ private static final Log LOG = LogFactory.getLog(GlobalMetricRegistriesAdapter.class);
+ private static GlobalMetricRegistriesAdapter INSTANCE;
+
+ private GlobalMetricRegistriesAdapter() {
+ DefaultMetricsSystem.initialize("HBase");
+ JvmMetrics.initSingleton("HBase", "");
--- End diff --
How will you differentiate this "client" (PQS) from other clients?
---
[GitHub] phoenix issue #315: PHOENIX-3655 Global Phoenix Client Metrics for PQS
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on the issue:
https://github.com/apache/phoenix/pull/315
Just used JVisualVM to look at these metrics via the MBeans directly. Worked just fine.
I think naming/convention when being collected from a distributed environment needs to be thought about more though.
---
[GitHub] phoenix issue #315: PHOENIX-3655 Global Phoenix Client Metrics for PQS
Posted by karanmehta93 <gi...@git.apache.org>.
Github user karanmehta93 commented on the issue:
https://github.com/apache/phoenix/pull/315
Thanks @xcangCRM and @joshelser for the review. I have addressed your review comments and added tag support to differentiate between various types of client when publishing metrics to JMX.
---
[GitHub] phoenix pull request #315: PHOENIX-3655 Global Phoenix Client Metrics for PQ...
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/315#discussion_r206259129
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/monitoring/GlobalClientMetrics.java ---
@@ -108,40 +114,88 @@
GLOBAL_HBASE_COUNT_ROWS_SCANNED(COUNT_ROWS_SCANNED),
GLOBAL_HBASE_COUNT_ROWS_FILTERED(COUNT_ROWS_FILTERED);
-
+ private static final Logger LOG = LoggerFactory.getLogger(GlobalClientMetrics.class);
private static final boolean isGlobalMetricsEnabled = QueryServicesOptions.withDefaults().isGlobalMetricsEnabled();
+ private MetricType metricType;
private GlobalMetric metric;
- public void update(long value) {
+ static {
+ initPhoenixGlobalClientMetrics();
if (isGlobalMetricsEnabled) {
- metric.change(value);
+ MetricRegistry metricRegistry = createMetricRegistry();
+ registerPhoenixMetricsToRegistry(metricRegistry);
+ GlobalMetricRegistriesAdapter.getInstance().registerMetricRegistry(metricRegistry);
+ }
+ }
+
+ private static void initPhoenixGlobalClientMetrics() {
+ for (GlobalClientMetrics globalMetric : GlobalClientMetrics.values()) {
+ globalMetric.metric = isGlobalMetricsEnabled ?
+ new GlobalMetricImpl(globalMetric.metricType) : new NoOpGlobalMetricImpl();
+ }
+ }
+
+ private static void registerPhoenixMetricsToRegistry(MetricRegistry metricRegistry) {
+ for (GlobalClientMetrics globalMetric : GlobalClientMetrics.values()) {
+ metricRegistry.register(globalMetric.metricType.columnName(),
+ new PhoenixGlobalMetricGauge(globalMetric.metric));
+ }
+ }
+
+ private static MetricRegistry createMetricRegistry() {
+ LOG.info("Creating Metric Registry for Phoenix Global Metrics");
+ MetricRegistryInfo registryInfo = new MetricRegistryInfo("PHOENIX", "Phoenix Global Metrics",
--- End diff --
> Global here refers that it is an aggregation across all clients (or all Phoenix Connections).
In my mind, if you show me "Phoenix Client Metrics", I would naturally assume that they are for all clients that have metrics enabled (as opposed to breakouts by individual clients). As such, the word "Global" to me is just filler, but maybe that's just my interpretation of it.
---
[GitHub] phoenix issue #315: PHOENIX-3655 Global Phoenix Client Metrics for PQS
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on the issue:
https://github.com/apache/phoenix/pull/315
> I think the only question pending in this PR is if we want JVMMetrics or not, which are crucial from PQS POV, otherwise not really for a regular client.
Agreed. What about just making a new configuration key in QueryServices/QueryServicesOptions for including JVM metrics in client-metrics which defaults to `false`? Then, have PQS always set this to be `true` via code.
Going to pull this down right now and look at it too out of curiosity.
---
[GitHub] phoenix pull request #315: PHOENIX-3655 Global Phoenix Client Metrics for PQ...
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/315#discussion_r206598366
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/monitoring/GlobalMetricRegistriesAdapter.java ---
@@ -0,0 +1,167 @@
+/*
+ * 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.phoenix.monitoring;
+
+import java.util.Iterator;
+import java.util.Map;
+import java.util.Map.Entry;
+import org.apache.commons.lang.StringUtils;
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
+import org.apache.hadoop.hbase.metrics.Counter;
+import org.apache.hadoop.hbase.metrics.Gauge;
+import org.apache.hadoop.hbase.metrics.Histogram;
+import org.apache.hadoop.hbase.metrics.Meter;
+import org.apache.hadoop.hbase.metrics.Metric;
+import org.apache.hadoop.hbase.metrics.MetricRegistry;
+import org.apache.hadoop.hbase.metrics.MetricRegistryInfo;
+import org.apache.hadoop.hbase.metrics.Timer;
+import org.apache.hadoop.metrics2.MetricsCollector;
+import org.apache.hadoop.metrics2.MetricsInfo;
+import org.apache.hadoop.metrics2.MetricsRecordBuilder;
+import org.apache.hadoop.metrics2.MetricsSource;
+import org.apache.hadoop.metrics2.lib.DefaultMetricsSystem;
+import org.apache.hadoop.metrics2.lib.Interns;
+import org.apache.hadoop.metrics2.lib.MutableHistogram;
+import org.apache.hadoop.metrics2.source.JvmMetrics;
+
+/**
+ * Contents mostly copied from GlobalMetricRegistriesAdapter class from hbase-hadoop2-compat
+ * The adapter attaches HBase's MetricRegistry to Hadoop's DefaultMetricsSystem
+ * Doesn't handle dynamic attach/detach of registries
+ */
+public class GlobalMetricRegistriesAdapter {
+
+ private static final Log LOG = LogFactory.getLog(GlobalMetricRegistriesAdapter.class);
+ private static GlobalMetricRegistriesAdapter INSTANCE;
+
+ private GlobalMetricRegistriesAdapter() {
+ DefaultMetricsSystem.initialize("HBase");
+ JvmMetrics.initSingleton("HBase", "");
--- End diff --
I think collecting JVM metrics from client applications is heavy-handed. I'd suggest you drop this.
---
[GitHub] phoenix pull request #315: PHOENIX-3655 Global Phoenix Client Metrics for PQ...
Posted by karanmehta93 <gi...@git.apache.org>.
Github user karanmehta93 commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/315#discussion_r207001759
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/monitoring/GlobalMetricRegistriesAdapter.java ---
@@ -0,0 +1,167 @@
+/*
+ * 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.phoenix.monitoring;
+
+import java.util.Iterator;
+import java.util.Map;
+import java.util.Map.Entry;
+import org.apache.commons.lang.StringUtils;
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
+import org.apache.hadoop.hbase.metrics.Counter;
+import org.apache.hadoop.hbase.metrics.Gauge;
+import org.apache.hadoop.hbase.metrics.Histogram;
+import org.apache.hadoop.hbase.metrics.Meter;
+import org.apache.hadoop.hbase.metrics.Metric;
+import org.apache.hadoop.hbase.metrics.MetricRegistry;
+import org.apache.hadoop.hbase.metrics.MetricRegistryInfo;
+import org.apache.hadoop.hbase.metrics.Timer;
+import org.apache.hadoop.metrics2.MetricsCollector;
+import org.apache.hadoop.metrics2.MetricsInfo;
+import org.apache.hadoop.metrics2.MetricsRecordBuilder;
+import org.apache.hadoop.metrics2.MetricsSource;
+import org.apache.hadoop.metrics2.lib.DefaultMetricsSystem;
+import org.apache.hadoop.metrics2.lib.Interns;
+import org.apache.hadoop.metrics2.lib.MutableHistogram;
+import org.apache.hadoop.metrics2.source.JvmMetrics;
+
+/**
+ * Contents mostly copied from GlobalMetricRegistriesAdapter class from hbase-hadoop2-compat
+ * The adapter attaches HBase's MetricRegistry to Hadoop's DefaultMetricsSystem
+ * Doesn't handle dynamic attach/detach of registries
+ */
+public class GlobalMetricRegistriesAdapter {
+
+ private static final Log LOG = LogFactory.getLog(GlobalMetricRegistriesAdapter.class);
+ private static GlobalMetricRegistriesAdapter INSTANCE;
+
+ private GlobalMetricRegistriesAdapter() {
+ DefaultMetricsSystem.initialize("HBase");
+ JvmMetrics.initSingleton("HBase", "");
--- End diff --
I am sorry, I didn't you on this one. Could you elaborate?
Also, I realized that stopping JVM metrics from Phoenix Level is not an option here. Since phoenix embeds hbase-client which instantiates JVM Metrics by default. Checkout this stack trace.
`
init:57, BaseSourceImpl$DefaultMetricsSystemInitializer (org.apache.hadoop.hbase.metrics)
<init>:112, BaseSourceImpl (org.apache.hadoop.hbase.metrics)
<init>:56, MetricsZooKeeperSourceImpl (org.apache.hadoop.hbase.zookeeper)
<init>:51, MetricsZooKeeperSourceImpl (org.apache.hadoop.hbase.zookeeper)
newInstance0:-1, NativeConstructorAccessorImpl (sun.reflect)
newInstance:62, NativeConstructorAccessorImpl (sun.reflect)
newInstance:45, DelegatingConstructorAccessorImpl (sun.reflect)
newInstance:423, Constructor (java.lang.reflect)
newInstance:442, Class (java.lang)
nextService:380, ServiceLoader$LazyIterator (java.util)
next:404, ServiceLoader$LazyIterator (java.util)
next:480, ServiceLoader$1 (java.util)
getInstance:59, CompatibilitySingletonFactory (org.apache.hadoop.hbase)
<init>:38, MetricsZooKeeper (org.apache.hadoop.hbase.zookeeper)
<init>:130, RecoverableZooKeeper (org.apache.hadoop.hbase.zookeeper)
connect:143, ZKUtil (org.apache.hadoop.hbase.zookeeper)
<init>:181, ZooKeeperWatcher (org.apache.hadoop.hbase.zookeeper)
<init>:155, ZooKeeperWatcher (org.apache.hadoop.hbase.zookeeper)
<init>:43, ZooKeeperKeepAliveConnection (org.apache.hadoop.hbase.client)
getKeepAliveZooKeeperWatcher:1737, ConnectionManager$HConnectionImplementation (org.apache.hadoop.hbase.client)
getClusterId:104, ZooKeeperRegistry (org.apache.hadoop.hbase.client)
retrieveClusterId:945, ConnectionManager$HConnectionImplementation (org.apache.hadoop.hbase.client)
<init>:721, ConnectionManager$HConnectionImplementation (org.apache.hadoop.hbase.client)
newInstance0:-1, NativeConstructorAccessorImpl (sun.reflect)
newInstance:62, NativeConstructorAccessorImpl (sun.reflect)
newInstance:45, DelegatingConstructorAccessorImpl (sun.reflect)
newInstance:423, Constructor (java.lang.reflect)
createConnection:238, ConnectionFactory (org.apache.hadoop.hbase.client)
createConnection:439, ConnectionManager (org.apache.hadoop.hbase.client)
createConnectionInternal:348, ConnectionManager (org.apache.hadoop.hbase.client)
createConnection:144, HConnectionManager (org.apache.hadoop.hbase.client)
createConnection:47, HConnectionFactory$HConnectionFactoryImpl (org.apache.phoenix.query)
openConnection:428, ConnectionQueryServicesImpl (org.apache.phoenix.query)
access$400:270, ConnectionQueryServicesImpl (org.apache.phoenix.query)
call:2539, ConnectionQueryServicesImpl$12 (org.apache.phoenix.query)
call:2515, ConnectionQueryServicesImpl$12 (org.apache.phoenix.query)
call:76, PhoenixContextExecutor (org.apache.phoenix.util)
init:2515, ConnectionQueryServicesImpl (org.apache.phoenix.query)
getConnectionQueryServices:255, PhoenixDriver (org.apache.phoenix.jdbc)
createConnection:150, PhoenixEmbeddedDriver (org.apache.phoenix.jdbc)
connect:221, PhoenixDriver (org.apache.phoenix.jdbc)
getConnection:664, DriverManager (java.sql)
getConnection:208, DriverManager (java.sql)
createConnection:640, JdbcMeta (org.apache.calcite.avatica.jdbc)
openConnection:625, JdbcMeta (org.apache.calcite.avatica.jdbc)
apply:285, LocalService (org.apache.calcite.avatica.remote)
accept:1770, Service$OpenConnectionRequest (org.apache.calcite.avatica.remote)
accept:1750, Service$OpenConnectionRequest (org.apache.calcite.avatica.remote)
apply:94, AbstractHandler (org.apache.calcite.avatica.remote)
apply:46, ProtobufHandler (org.apache.calcite.avatica.remote)
handle:127, AvaticaProtobufHandler (org.apache.calcite.avatica.server)
handle:52, HandlerList (org.eclipse.jetty.server.handler)
handle:97, HandlerWrapper (org.eclipse.jetty.server.handler)
handle:499, Server (org.eclipse.jetty.server)
handle:311, HttpChannel (org.eclipse.jetty.server)
onFillable:257, HttpConnection (org.eclipse.jetty.server)
run:544, AbstractConnection$2 (org.eclipse.jetty.io)
runJob:635, QueuedThreadPool (org.eclipse.jetty.util.thread)
run:555, QueuedThreadPool$3 (org.eclipse.jetty.util.thread)
run:748, Thread (java.lang)
`
---
[GitHub] phoenix issue #315: PHOENIX-3655 Global Phoenix Client Metrics for PQS
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on the issue:
https://github.com/apache/phoenix/pull/315
One final request, @karanmehta93 -- can you set `phoenix.client.metrics.tag` in QueryServer.java proactively, please?
You have my +1 after that.
---
[GitHub] phoenix issue #315: PHOENIX-3655 Global Phoenix Client Metrics for PQS
Posted by karanmehta93 <gi...@git.apache.org>.
Github user karanmehta93 commented on the issue:
https://github.com/apache/phoenix/pull/315
@joshelser I don't see an easy way of doing it, since we read the tag from `metricTag = QueryServicesOptions.withDefaults().getClientMetricTag();`, which internally uses `InstanceResolver`, which depends on implementations of `ConfigurationFactory`. Ideally, I need to add META-INF/<file> to provide that factory and make it work. I don't think that we want to enforce that. If there is any other option, let me know.
---
[GitHub] phoenix pull request #315: PHOENIX-3655 Global Phoenix Client Metrics for PQ...
Posted by karanmehta93 <gi...@git.apache.org>.
Github user karanmehta93 commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/315#discussion_r207049490
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/monitoring/GlobalMetricRegistriesAdapter.java ---
@@ -0,0 +1,167 @@
+/*
+ * 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.phoenix.monitoring;
+
+import java.util.Iterator;
+import java.util.Map;
+import java.util.Map.Entry;
+import org.apache.commons.lang.StringUtils;
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
+import org.apache.hadoop.hbase.metrics.Counter;
+import org.apache.hadoop.hbase.metrics.Gauge;
+import org.apache.hadoop.hbase.metrics.Histogram;
+import org.apache.hadoop.hbase.metrics.Meter;
+import org.apache.hadoop.hbase.metrics.Metric;
+import org.apache.hadoop.hbase.metrics.MetricRegistry;
+import org.apache.hadoop.hbase.metrics.MetricRegistryInfo;
+import org.apache.hadoop.hbase.metrics.Timer;
+import org.apache.hadoop.metrics2.MetricsCollector;
+import org.apache.hadoop.metrics2.MetricsInfo;
+import org.apache.hadoop.metrics2.MetricsRecordBuilder;
+import org.apache.hadoop.metrics2.MetricsSource;
+import org.apache.hadoop.metrics2.lib.DefaultMetricsSystem;
+import org.apache.hadoop.metrics2.lib.Interns;
+import org.apache.hadoop.metrics2.lib.MutableHistogram;
+import org.apache.hadoop.metrics2.source.JvmMetrics;
+
+/**
+ * Contents mostly copied from GlobalMetricRegistriesAdapter class from hbase-hadoop2-compat
+ * The adapter attaches HBase's MetricRegistry to Hadoop's DefaultMetricsSystem
+ * Doesn't handle dynamic attach/detach of registries
+ */
+public class GlobalMetricRegistriesAdapter {
+
+ private static final Log LOG = LogFactory.getLog(GlobalMetricRegistriesAdapter.class);
+ private static GlobalMetricRegistriesAdapter INSTANCE;
+
+ private GlobalMetricRegistriesAdapter() {
+ DefaultMetricsSystem.initialize("HBase");
+ JvmMetrics.initSingleton("HBase", "");
--- End diff --
So if I understand this correctly, I will include a new property which takes a csv of tags as input. Later on, I can split by comma and add those tags to `HBaseMetrics2HadoopMetricsAdapter#snapshotAllMetrics()`. @joshelser
---
[GitHub] phoenix pull request #315: PHOENIX-3655 Global Phoenix Client Metrics for PQ...
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/315#discussion_r206259200
--- Diff: phoenix-core/src/test/resources/hadoop-metrics2.properties ---
@@ -32,10 +32,9 @@
# [prefix].[source|sink].[instance].[options]
# See javadoc of package-info.java for org.apache.hadoop.metrics2 for detail
-
-# Don't attempt to start jmx mbeans for all sources.
-# For right now, all metrics are exported to a phoenix table
-*.source.start_mbeans=false
+hbase.source.start_mbeans=true
+hbase.sink.sink0.class=org.apache.phoenix.monitoring.GlobalPhoenixMetricsTestSink
--- End diff --
Haha, perfect :)
---
[GitHub] phoenix issue #315: PHOENIX-3655 Global Phoenix Client Metrics for PQS
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on the issue:
https://github.com/apache/phoenix/pull/315
> All these metrics are pushed to JMX and we monitor those via OpenTSDB tcollectors.
Cool! I was just looking for a sanity check that you are seeing all of the metrics you expect coming out of PQS and that they are "useful". No need to perform any perf testing for me.
---
[GitHub] phoenix issue #315: PHOENIX-3655 Global Phoenix Client Metrics for PQS
Posted by karanmehta93 <gi...@git.apache.org>.
Github user karanmehta93 commented on the issue:
https://github.com/apache/phoenix/pull/315
> Isn't this as simple as changing QueryServer#main(String[]) to use HBaseFactoryProvider.getConfigurationFactory().getConfiguration(); instead of HBaseConfiguration.create()?
Yes I want to do that for sure here, and was also planning to file a Jira for that, but seems like I get the change done here.
You also want me to set the value of `phoenix.client.metrics.tag` to `PQS` right? For that to happen, I will need a factory, because no matter what conf we pass to the tool, that one is not propagated to phoenix-core. `QueryServicesOptions.withDefaults()` always asks the `HBaseFactoryProvider.getConfigurationFactory().getConfiguration()` for the values. Am I missing something here?
---
[GitHub] phoenix issue #315: PHOENIX-3655 Global Phoenix Client Metrics for PQS
Posted by karanmehta93 <gi...@git.apache.org>.
Github user karanmehta93 commented on the issue:
https://github.com/apache/phoenix/pull/315
Addressed your comments @joshelser, Please review!
---
[GitHub] phoenix pull request #315: PHOENIX-3655 Global Phoenix Client Metrics for PQ...
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/315#discussion_r205884608
--- Diff: phoenix-core/src/test/resources/hadoop-metrics2.properties ---
@@ -32,10 +32,9 @@
# [prefix].[source|sink].[instance].[options]
# See javadoc of package-info.java for org.apache.hadoop.metrics2 for detail
-
-# Don't attempt to start jmx mbeans for all sources.
-# For right now, all metrics are exported to a phoenix table
-*.source.start_mbeans=false
+hbase.source.start_mbeans=true
+hbase.sink.sink0.class=org.apache.phoenix.monitoring.GlobalPhoenixMetricsTestSink
--- End diff --
Can you leave a comment in the test that this configuration is here? Otherwise, might seem like voodoo
---
[GitHub] phoenix issue #315: PHOENIX-3655 Global Phoenix Client Metrics for PQS
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on the issue:
https://github.com/apache/phoenix/pull/315
> it looks like there are some commons-configuration issues that will need to be worked out for GlobalPhoenixMetricsTestSink.java
Just needs to be switched to the `org.apache.commons.configuration2` package. Also, GlobalPhoenixMetricsTestSink has a lot of unused imports. Would be good to make a pass over your changes for those.
---
[GitHub] phoenix issue #315: PHOENIX-3655 Global Phoenix Client Metrics for PQS
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on the issue:
https://github.com/apache/phoenix/pull/315
Pulling this into `master`, it looks like there are some commons-configuration issues that will need to be worked out for GlobalPhoenixMetricsTestSink.java
---
[GitHub] phoenix pull request #315: PHOENIX-3655 Global Phoenix Client Metrics for PQ...
Posted by xcangCRM <gi...@git.apache.org>.
Github user xcangCRM commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/315#discussion_r207052688
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/monitoring/GlobalMetricRegistriesAdapter.java ---
@@ -0,0 +1,167 @@
+/*
+ * 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.phoenix.monitoring;
+
+import java.util.Iterator;
+import java.util.Map;
+import java.util.Map.Entry;
+import org.apache.commons.lang.StringUtils;
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
+import org.apache.hadoop.hbase.metrics.Counter;
+import org.apache.hadoop.hbase.metrics.Gauge;
+import org.apache.hadoop.hbase.metrics.Histogram;
+import org.apache.hadoop.hbase.metrics.Meter;
+import org.apache.hadoop.hbase.metrics.Metric;
+import org.apache.hadoop.hbase.metrics.MetricRegistry;
+import org.apache.hadoop.hbase.metrics.MetricRegistryInfo;
+import org.apache.hadoop.hbase.metrics.Timer;
+import org.apache.hadoop.metrics2.MetricsCollector;
+import org.apache.hadoop.metrics2.MetricsInfo;
+import org.apache.hadoop.metrics2.MetricsRecordBuilder;
+import org.apache.hadoop.metrics2.MetricsSource;
+import org.apache.hadoop.metrics2.lib.DefaultMetricsSystem;
+import org.apache.hadoop.metrics2.lib.Interns;
+import org.apache.hadoop.metrics2.lib.MutableHistogram;
+import org.apache.hadoop.metrics2.source.JvmMetrics;
+
+/**
+ * Contents mostly copied from GlobalMetricRegistriesAdapter class from hbase-hadoop2-compat
+ * The adapter attaches HBase's MetricRegistry to Hadoop's DefaultMetricsSystem
+ * Doesn't handle dynamic attach/detach of registries
+ */
+public class GlobalMetricRegistriesAdapter {
+
+ private static final Log LOG = LogFactory.getLog(GlobalMetricRegistriesAdapter.class);
+ private static GlobalMetricRegistriesAdapter INSTANCE;
+
+ private GlobalMetricRegistriesAdapter() {
+ DefaultMetricsSystem.initialize("Phoenix");
+ JvmMetrics.initSingleton("Phoenix", "");
+ }
+
+ public static GlobalMetricRegistriesAdapter getInstance() {
+ if (INSTANCE == null) {
--- End diff --
nit: do you need double checking here?
if (INSTANCE == null) {
synchronized(this) {
if(INSTANCE == null) {
...
}
}
}
---
[GitHub] phoenix issue #315: PHOENIX-3655 Global Phoenix Client Metrics for PQS
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on the issue:
https://github.com/apache/phoenix/pull/315
@karanmehta93 but in PQS, we're controlling the Configuration. Isn't this as simple as changing `QueryServer#main(String[])` to use `HBaseFactoryProvider.getConfigurationFactory().getConfiguration();` instead of `HBaseConfiguration.create()`?
---
[GitHub] phoenix pull request #315: PHOENIX-3655 Global Phoenix Client Metrics for PQ...
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/315#discussion_r206259317
--- Diff: phoenix-core/src/it/java/org/apache/phoenix/monitoring/PhoenixMetricsIT.java ---
@@ -204,6 +219,39 @@ private static void resetGlobalMetrics() {
}
}
+ private boolean verifyMetricsFromSink() throws InterruptedException {
+ Map<String, Long> expectedMetrics = new HashMap<>();
+ for (GlobalMetric m : PhoenixRuntime.getGlobalPhoenixClientMetrics()) {
+ expectedMetrics.put(m.getMetricType().name(), m.getTotalSum());
+ }
+
+ for (int i = 0; i < MAX_RETRIES; i++) {
+ LOG.info("Verifying Global Metrics from Hadoop Sink, Retry: " + (i + 1));
+ if (verifyMetricsFromSinkOnce(expectedMetrics)) {
+ LOG.info("Values from Hadoop Metrics Sink match actual values");
+ return true;
+ }
+ Thread.sleep(1000);
+ }
+ return false;
+ }
+
+ private boolean verifyMetricsFromSinkOnce(Map<String, Long> expectedMetrics) {
+ synchronized (GlobalPhoenixMetricsTestSink.lock) {
+ for (AbstractMetric metric : GlobalPhoenixMetricsTestSink.metrics) {
+ if (expectedMetrics.containsKey(metric.name())) {
--- End diff --
> One approach can be to remove each entry from the map and check if the size is 0 at the end. How does that sound?
That would work too!
---
[GitHub] phoenix pull request #315: PHOENIX-3655 Global Phoenix Client Metrics for PQ...
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/315#discussion_r206638567
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/monitoring/GlobalMetricRegistriesAdapter.java ---
@@ -0,0 +1,167 @@
+/*
+ * 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.phoenix.monitoring;
+
+import java.util.Iterator;
+import java.util.Map;
+import java.util.Map.Entry;
+import org.apache.commons.lang.StringUtils;
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
+import org.apache.hadoop.hbase.metrics.Counter;
+import org.apache.hadoop.hbase.metrics.Gauge;
+import org.apache.hadoop.hbase.metrics.Histogram;
+import org.apache.hadoop.hbase.metrics.Meter;
+import org.apache.hadoop.hbase.metrics.Metric;
+import org.apache.hadoop.hbase.metrics.MetricRegistry;
+import org.apache.hadoop.hbase.metrics.MetricRegistryInfo;
+import org.apache.hadoop.hbase.metrics.Timer;
+import org.apache.hadoop.metrics2.MetricsCollector;
+import org.apache.hadoop.metrics2.MetricsInfo;
+import org.apache.hadoop.metrics2.MetricsRecordBuilder;
+import org.apache.hadoop.metrics2.MetricsSource;
+import org.apache.hadoop.metrics2.lib.DefaultMetricsSystem;
+import org.apache.hadoop.metrics2.lib.Interns;
+import org.apache.hadoop.metrics2.lib.MutableHistogram;
+import org.apache.hadoop.metrics2.source.JvmMetrics;
+
+/**
+ * Contents mostly copied from GlobalMetricRegistriesAdapter class from hbase-hadoop2-compat
+ * The adapter attaches HBase's MetricRegistry to Hadoop's DefaultMetricsSystem
+ * Doesn't handle dynamic attach/detach of registries
+ */
+public class GlobalMetricRegistriesAdapter {
+
+ private static final Log LOG = LogFactory.getLog(GlobalMetricRegistriesAdapter.class);
+ private static GlobalMetricRegistriesAdapter INSTANCE;
+
+ private GlobalMetricRegistriesAdapter() {
+ DefaultMetricsSystem.initialize("HBase");
+ JvmMetrics.initSingleton("HBase", "");
--- End diff --
Oh, right. I keep getting confused over that.
JVM metrics being exported from PQS makes total sense. +1 to that
I am now wondering: should we be exporting Phoenix metrics out of PQS under "Client" or make some specific "sub=PQS" or similar for it?
---
[GitHub] phoenix pull request #315: PHOENIX-3655 Global Phoenix Client Metrics for PQ...
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/315#discussion_r207003788
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/monitoring/GlobalMetricRegistriesAdapter.java ---
@@ -0,0 +1,167 @@
+/*
+ * 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.phoenix.monitoring;
+
+import java.util.Iterator;
+import java.util.Map;
+import java.util.Map.Entry;
+import org.apache.commons.lang.StringUtils;
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
+import org.apache.hadoop.hbase.metrics.Counter;
+import org.apache.hadoop.hbase.metrics.Gauge;
+import org.apache.hadoop.hbase.metrics.Histogram;
+import org.apache.hadoop.hbase.metrics.Meter;
+import org.apache.hadoop.hbase.metrics.Metric;
+import org.apache.hadoop.hbase.metrics.MetricRegistry;
+import org.apache.hadoop.hbase.metrics.MetricRegistryInfo;
+import org.apache.hadoop.hbase.metrics.Timer;
+import org.apache.hadoop.metrics2.MetricsCollector;
+import org.apache.hadoop.metrics2.MetricsInfo;
+import org.apache.hadoop.metrics2.MetricsRecordBuilder;
+import org.apache.hadoop.metrics2.MetricsSource;
+import org.apache.hadoop.metrics2.lib.DefaultMetricsSystem;
+import org.apache.hadoop.metrics2.lib.Interns;
+import org.apache.hadoop.metrics2.lib.MutableHistogram;
+import org.apache.hadoop.metrics2.source.JvmMetrics;
+
+/**
+ * Contents mostly copied from GlobalMetricRegistriesAdapter class from hbase-hadoop2-compat
+ * The adapter attaches HBase's MetricRegistry to Hadoop's DefaultMetricsSystem
+ * Doesn't handle dynamic attach/detach of registries
+ */
+public class GlobalMetricRegistriesAdapter {
+
+ private static final Log LOG = LogFactory.getLog(GlobalMetricRegistriesAdapter.class);
+ private static GlobalMetricRegistriesAdapter INSTANCE;
+
+ private GlobalMetricRegistriesAdapter() {
+ DefaultMetricsSystem.initialize("HBase");
+ JvmMetrics.initSingleton("HBase", "");
--- End diff --
> I am sorry, I didn't you on this one. Could you elaborate?
You're using openTSDB to collect all of this data and your use case is to "understand how PQS runs". To do this, you will need to know what hosts that PQS is running on to just look at PQS metrics. I was suggesting that having a unique name (or a tag) would make an operator's life much more simple -- it would be clear (and presumably easier to filter on) to find just PQS metrics.
For example, the difference of issuing a filter "host in pqs_server1, pqs_server2, pqs_server3" as opposed to just "tag=PQS", or similar.
Now that I'm thinking about this, leaving this as {{Phoenix,sub=CLIENT}} may be OK and you can instead just configure the push of metrics data to Hadoop Metrics2 to include a "tag" that indicates this is from PQS. That would make me happy.
---
[GitHub] phoenix pull request #315: PHOENIX-3655 Global Phoenix Client Metrics for PQ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/phoenix/pull/315
---
[GitHub] phoenix issue #315: PHOENIX-3655 Global Phoenix Client Metrics for PQS
Posted by karanmehta93 <gi...@git.apache.org>.
Github user karanmehta93 commented on the issue:
https://github.com/apache/phoenix/pull/315
Thanks @joshelser will update the PR soon. We have immediate requirements for HBase-1.x versions here. That is why I mentioned that at the start of PR [itself](https://issues.apache.org/jira/browse/PHOENIX-3655?page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel&focusedCommentId=16559438#comment-16559438) that it might be tricky to port these patches. You can try it out with 4.x-HBase-1.4 branch, which is where my current PR is based on. Let me know if you think anything else is required here.
---
[GitHub] phoenix issue #315: PHOENIX-3655 Global Phoenix Client Metrics for PQS
Posted by karanmehta93 <gi...@git.apache.org>.
Github user karanmehta93 commented on the issue:
https://github.com/apache/phoenix/pull/315
Great! That makes sense.
Thanks for the review @joshelser . I will be (squashing and) pushing this to 4.x-HBase-1.4 branch and will back port / forward port this patch as well.
---
[GitHub] phoenix pull request #315: PHOENIX-3655 Global Phoenix Client Metrics for PQ...
Posted by karanmehta93 <gi...@git.apache.org>.
Github user karanmehta93 commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/315#discussion_r207063036
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/monitoring/GlobalMetricRegistriesAdapter.java ---
@@ -0,0 +1,167 @@
+/*
+ * 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.phoenix.monitoring;
+
+import java.util.Iterator;
+import java.util.Map;
+import java.util.Map.Entry;
+import org.apache.commons.lang.StringUtils;
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
+import org.apache.hadoop.hbase.metrics.Counter;
+import org.apache.hadoop.hbase.metrics.Gauge;
+import org.apache.hadoop.hbase.metrics.Histogram;
+import org.apache.hadoop.hbase.metrics.Meter;
+import org.apache.hadoop.hbase.metrics.Metric;
+import org.apache.hadoop.hbase.metrics.MetricRegistry;
+import org.apache.hadoop.hbase.metrics.MetricRegistryInfo;
+import org.apache.hadoop.hbase.metrics.Timer;
+import org.apache.hadoop.metrics2.MetricsCollector;
+import org.apache.hadoop.metrics2.MetricsInfo;
+import org.apache.hadoop.metrics2.MetricsRecordBuilder;
+import org.apache.hadoop.metrics2.MetricsSource;
+import org.apache.hadoop.metrics2.lib.DefaultMetricsSystem;
+import org.apache.hadoop.metrics2.lib.Interns;
+import org.apache.hadoop.metrics2.lib.MutableHistogram;
+import org.apache.hadoop.metrics2.source.JvmMetrics;
+
+/**
+ * Contents mostly copied from GlobalMetricRegistriesAdapter class from hbase-hadoop2-compat
+ * The adapter attaches HBase's MetricRegistry to Hadoop's DefaultMetricsSystem
+ * Doesn't handle dynamic attach/detach of registries
+ */
+public class GlobalMetricRegistriesAdapter {
+
+ private static final Log LOG = LogFactory.getLog(GlobalMetricRegistriesAdapter.class);
+ private static GlobalMetricRegistriesAdapter INSTANCE;
+
+ private GlobalMetricRegistriesAdapter() {
+ DefaultMetricsSystem.initialize("Phoenix");
+ JvmMetrics.initSingleton("Phoenix", "");
+ }
+
+ public static GlobalMetricRegistriesAdapter getInstance() {
+ if (INSTANCE == null) {
--- End diff --
I think I am moving away from lazy initialization all together. No need of premature optimization here I believe.
---
[GitHub] phoenix pull request #315: PHOENIX-3655 Global Phoenix Client Metrics for PQ...
Posted by karanmehta93 <gi...@git.apache.org>.
Github user karanmehta93 commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/315#discussion_r206968534
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/monitoring/GlobalMetricRegistriesAdapter.java ---
@@ -0,0 +1,167 @@
+/*
+ * 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.phoenix.monitoring;
+
+import java.util.Iterator;
+import java.util.Map;
+import java.util.Map.Entry;
+import org.apache.commons.lang.StringUtils;
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
+import org.apache.hadoop.hbase.metrics.Counter;
+import org.apache.hadoop.hbase.metrics.Gauge;
+import org.apache.hadoop.hbase.metrics.Histogram;
+import org.apache.hadoop.hbase.metrics.Meter;
+import org.apache.hadoop.hbase.metrics.Metric;
+import org.apache.hadoop.hbase.metrics.MetricRegistry;
+import org.apache.hadoop.hbase.metrics.MetricRegistryInfo;
+import org.apache.hadoop.hbase.metrics.Timer;
+import org.apache.hadoop.metrics2.MetricsCollector;
+import org.apache.hadoop.metrics2.MetricsInfo;
+import org.apache.hadoop.metrics2.MetricsRecordBuilder;
+import org.apache.hadoop.metrics2.MetricsSource;
+import org.apache.hadoop.metrics2.lib.DefaultMetricsSystem;
+import org.apache.hadoop.metrics2.lib.Interns;
+import org.apache.hadoop.metrics2.lib.MutableHistogram;
+import org.apache.hadoop.metrics2.source.JvmMetrics;
+
+/**
+ * Contents mostly copied from GlobalMetricRegistriesAdapter class from hbase-hadoop2-compat
+ * The adapter attaches HBase's MetricRegistry to Hadoop's DefaultMetricsSystem
+ * Doesn't handle dynamic attach/detach of registries
+ */
+public class GlobalMetricRegistriesAdapter {
+
+ private static final Log LOG = LogFactory.getLog(GlobalMetricRegistriesAdapter.class);
+ private static GlobalMetricRegistriesAdapter INSTANCE;
+
+ private GlobalMetricRegistriesAdapter() {
+ DefaultMetricsSystem.initialize("HBase");
+ JvmMetrics.initSingleton("HBase", "");
--- End diff --
Lets keep it as client since they represent client in the end, even though it PQS that is instantiating it.
---
[GitHub] phoenix pull request #315: PHOENIX-3655 Global Phoenix Client Metrics for PQ...
Posted by karanmehta93 <gi...@git.apache.org>.
Github user karanmehta93 commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/315#discussion_r205900530
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/monitoring/GlobalClientMetrics.java ---
@@ -108,40 +114,88 @@
GLOBAL_HBASE_COUNT_ROWS_SCANNED(COUNT_ROWS_SCANNED),
GLOBAL_HBASE_COUNT_ROWS_FILTERED(COUNT_ROWS_FILTERED);
-
+ private static final Logger LOG = LoggerFactory.getLogger(GlobalClientMetrics.class);
private static final boolean isGlobalMetricsEnabled = QueryServicesOptions.withDefaults().isGlobalMetricsEnabled();
+ private MetricType metricType;
private GlobalMetric metric;
- public void update(long value) {
+ static {
+ initPhoenixGlobalClientMetrics();
if (isGlobalMetricsEnabled) {
- metric.change(value);
+ MetricRegistry metricRegistry = createMetricRegistry();
+ registerPhoenixMetricsToRegistry(metricRegistry);
+ GlobalMetricRegistriesAdapter.getInstance().registerMetricRegistry(metricRegistry);
+ }
+ }
+
+ private static void initPhoenixGlobalClientMetrics() {
+ for (GlobalClientMetrics globalMetric : GlobalClientMetrics.values()) {
+ globalMetric.metric = isGlobalMetricsEnabled ?
+ new GlobalMetricImpl(globalMetric.metricType) : new NoOpGlobalMetricImpl();
+ }
+ }
+
+ private static void registerPhoenixMetricsToRegistry(MetricRegistry metricRegistry) {
+ for (GlobalClientMetrics globalMetric : GlobalClientMetrics.values()) {
+ metricRegistry.register(globalMetric.metricType.columnName(),
+ new PhoenixGlobalMetricGauge(globalMetric.metric));
+ }
+ }
+
+ private static MetricRegistry createMetricRegistry() {
+ LOG.info("Creating Metric Registry for Phoenix Global Metrics");
+ MetricRegistryInfo registryInfo = new MetricRegistryInfo("PHOENIX", "Phoenix Global Metrics",
--- End diff --
Global here refers that it is an aggregation across all clients (or all Phoenix Connections).
> Maybe "Phoenix,sub=CLIENT" down below?
Seems reasonable as well.
---
[GitHub] phoenix issue #315: PHOENIX-3655 Global Phoenix Client Metrics for PQS
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on the issue:
https://github.com/apache/phoenix/pull/315
> Am I missing something here?
Nope, it's me :). I didn't realize that the Factory was creating a new `Configuration` every time. The setup of `GlobalClientMetrics` just... doesn't give us any way to do this nicely :(
I'm not sure what it takes to write a custom Factory (I think I did it once in an IT), but would be OK if you defer this.
After you commit this, please update the website with these necessary changes (e.g. at least tell people that PQS exports metrics).
---
[GitHub] phoenix pull request #315: PHOENIX-3655 Global Phoenix Client Metrics for PQ...
Posted by karanmehta93 <gi...@git.apache.org>.
Github user karanmehta93 commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/315#discussion_r207060270
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/monitoring/GlobalMetricRegistriesAdapter.java ---
@@ -0,0 +1,167 @@
+/*
+ * 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.phoenix.monitoring;
+
+import java.util.Iterator;
+import java.util.Map;
+import java.util.Map.Entry;
+import org.apache.commons.lang.StringUtils;
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
+import org.apache.hadoop.hbase.metrics.Counter;
+import org.apache.hadoop.hbase.metrics.Gauge;
+import org.apache.hadoop.hbase.metrics.Histogram;
+import org.apache.hadoop.hbase.metrics.Meter;
+import org.apache.hadoop.hbase.metrics.Metric;
+import org.apache.hadoop.hbase.metrics.MetricRegistry;
+import org.apache.hadoop.hbase.metrics.MetricRegistryInfo;
+import org.apache.hadoop.hbase.metrics.Timer;
+import org.apache.hadoop.metrics2.MetricsCollector;
+import org.apache.hadoop.metrics2.MetricsInfo;
+import org.apache.hadoop.metrics2.MetricsRecordBuilder;
+import org.apache.hadoop.metrics2.MetricsSource;
+import org.apache.hadoop.metrics2.lib.DefaultMetricsSystem;
+import org.apache.hadoop.metrics2.lib.Interns;
+import org.apache.hadoop.metrics2.lib.MutableHistogram;
+import org.apache.hadoop.metrics2.source.JvmMetrics;
+
+/**
+ * Contents mostly copied from GlobalMetricRegistriesAdapter class from hbase-hadoop2-compat
+ * The adapter attaches HBase's MetricRegistry to Hadoop's DefaultMetricsSystem
+ * Doesn't handle dynamic attach/detach of registries
+ */
+public class GlobalMetricRegistriesAdapter {
+
+ private static final Log LOG = LogFactory.getLog(GlobalMetricRegistriesAdapter.class);
+ private static GlobalMetricRegistriesAdapter INSTANCE;
+
+ private GlobalMetricRegistriesAdapter() {
+ DefaultMetricsSystem.initialize("Phoenix");
+ JvmMetrics.initSingleton("Phoenix", "");
+ }
+
+ public static GlobalMetricRegistriesAdapter getInstance() {
+ if (INSTANCE == null) {
--- End diff --
As of now, this method is called from static block in `GlobalClientMetrics`, which can happen only once, so we dont have any race conditions here.
I have two options here, add a lock here or move this initialization to static block of this class. What do you suggest?
---
[GitHub] phoenix pull request #315: PHOENIX-3655 Global Phoenix Client Metrics for PQ...
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/315#discussion_r206974865
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/monitoring/GlobalMetricRegistriesAdapter.java ---
@@ -0,0 +1,167 @@
+/*
+ * 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.phoenix.monitoring;
+
+import java.util.Iterator;
+import java.util.Map;
+import java.util.Map.Entry;
+import org.apache.commons.lang.StringUtils;
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
+import org.apache.hadoop.hbase.metrics.Counter;
+import org.apache.hadoop.hbase.metrics.Gauge;
+import org.apache.hadoop.hbase.metrics.Histogram;
+import org.apache.hadoop.hbase.metrics.Meter;
+import org.apache.hadoop.hbase.metrics.Metric;
+import org.apache.hadoop.hbase.metrics.MetricRegistry;
+import org.apache.hadoop.hbase.metrics.MetricRegistryInfo;
+import org.apache.hadoop.hbase.metrics.Timer;
+import org.apache.hadoop.metrics2.MetricsCollector;
+import org.apache.hadoop.metrics2.MetricsInfo;
+import org.apache.hadoop.metrics2.MetricsRecordBuilder;
+import org.apache.hadoop.metrics2.MetricsSource;
+import org.apache.hadoop.metrics2.lib.DefaultMetricsSystem;
+import org.apache.hadoop.metrics2.lib.Interns;
+import org.apache.hadoop.metrics2.lib.MutableHistogram;
+import org.apache.hadoop.metrics2.source.JvmMetrics;
+
+/**
+ * Contents mostly copied from GlobalMetricRegistriesAdapter class from hbase-hadoop2-compat
+ * The adapter attaches HBase's MetricRegistry to Hadoop's DefaultMetricsSystem
+ * Doesn't handle dynamic attach/detach of registries
+ */
+public class GlobalMetricRegistriesAdapter {
+
+ private static final Log LOG = LogFactory.getLog(GlobalMetricRegistriesAdapter.class);
+ private static GlobalMetricRegistriesAdapter INSTANCE;
+
+ private GlobalMetricRegistriesAdapter() {
+ DefaultMetricsSystem.initialize("HBase");
+ JvmMetrics.initSingleton("HBase", "");
--- End diff --
> why do we need to differentiate
As an operator, I'm going to expect much different characteristics coming from client applications than I am from PQS. Since PQS is a SPOF for (potentially) many users, I would expect a higher level of regularity from PQS than I would a client's app.
> We should get that info from the application JVM itself right
I'm sure there's something you can apply to find a PQS metric compared to other clients, but I am just thinking that it would be good to make this as easy as possible. I think watching PQS metrics would be a common use-case, not the exception.
---
[GitHub] phoenix pull request #315: PHOENIX-3655 Global Phoenix Client Metrics for PQ...
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/315#discussion_r206595985
--- Diff: phoenix-core/src/it/java/org/apache/phoenix/monitoring/PhoenixMetricsIT.java ---
@@ -204,6 +230,45 @@ private static void resetGlobalMetrics() {
}
}
+ // Phoenix Client Metrics are transported via Hadoop-metrics2 sink
+ // The test sink is defined at GlobalPhoenixMetricsTestSink
+ // Configuration for Hadoop-metrics2 comes from hadoop-metrics2.properties file located in test/resources
+ private boolean verifyMetricsFromSink() throws InterruptedException {
+ Map<String, Long> expectedMetrics = new HashMap<>();
+ for (GlobalMetric m : PhoenixRuntime.getGlobalPhoenixClientMetrics()) {
+ expectedMetrics.put(m.getMetricType().name(), m.getTotalSum());
+ }
+
+ for (int i = 0; i < MAX_RETRIES; i++) {
+ LOG.info("Verifying Global Metrics from Hadoop Sink, Retry: " + (i + 1));
+ if (verifyMetricsFromSinkOnce(expectedMetrics)) {
+ LOG.info("Values from Hadoop Metrics Sink match actual values");
+ return true;
+ }
+ Thread.sleep(1000);
--- End diff --
Change this to:
```
try {
Thread.sleep(1000);
} catch (InterruptedException e) {
Thread.currentThread().interrupt();
return false;
}
```
Then, remove the `throws InterruptedException`.
We should be catching, re-setting the interrupted state and just returning ASAP. Likely, this code never gets invoked in a unit-test context, but good practice.
---
[GitHub] phoenix pull request #315: PHOENIX-3655 Global Phoenix Client Metrics for PQ...
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/315#discussion_r206598160
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/monitoring/GlobalMetricRegistriesAdapter.java ---
@@ -0,0 +1,167 @@
+/*
+ * 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.phoenix.monitoring;
+
+import java.util.Iterator;
+import java.util.Map;
+import java.util.Map.Entry;
+import org.apache.commons.lang.StringUtils;
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
+import org.apache.hadoop.hbase.metrics.Counter;
+import org.apache.hadoop.hbase.metrics.Gauge;
+import org.apache.hadoop.hbase.metrics.Histogram;
+import org.apache.hadoop.hbase.metrics.Meter;
+import org.apache.hadoop.hbase.metrics.Metric;
+import org.apache.hadoop.hbase.metrics.MetricRegistry;
+import org.apache.hadoop.hbase.metrics.MetricRegistryInfo;
+import org.apache.hadoop.hbase.metrics.Timer;
+import org.apache.hadoop.metrics2.MetricsCollector;
+import org.apache.hadoop.metrics2.MetricsInfo;
+import org.apache.hadoop.metrics2.MetricsRecordBuilder;
+import org.apache.hadoop.metrics2.MetricsSource;
+import org.apache.hadoop.metrics2.lib.DefaultMetricsSystem;
+import org.apache.hadoop.metrics2.lib.Interns;
+import org.apache.hadoop.metrics2.lib.MutableHistogram;
+import org.apache.hadoop.metrics2.source.JvmMetrics;
+
+/**
+ * Contents mostly copied from GlobalMetricRegistriesAdapter class from hbase-hadoop2-compat
+ * The adapter attaches HBase's MetricRegistry to Hadoop's DefaultMetricsSystem
+ * Doesn't handle dynamic attach/detach of registries
+ */
+public class GlobalMetricRegistriesAdapter {
+
+ private static final Log LOG = LogFactory.getLog(GlobalMetricRegistriesAdapter.class);
+ private static GlobalMetricRegistriesAdapter INSTANCE;
+
+ private GlobalMetricRegistriesAdapter() {
+ DefaultMetricsSystem.initialize("HBase");
--- End diff --
s/HBase/Phoenix/
---
[GitHub] phoenix pull request #315: PHOENIX-3655 Global Phoenix Client Metrics for PQ...
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/315#discussion_r207051474
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/monitoring/GlobalMetricRegistriesAdapter.java ---
@@ -0,0 +1,167 @@
+/*
+ * 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.phoenix.monitoring;
+
+import java.util.Iterator;
+import java.util.Map;
+import java.util.Map.Entry;
+import org.apache.commons.lang.StringUtils;
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
+import org.apache.hadoop.hbase.metrics.Counter;
+import org.apache.hadoop.hbase.metrics.Gauge;
+import org.apache.hadoop.hbase.metrics.Histogram;
+import org.apache.hadoop.hbase.metrics.Meter;
+import org.apache.hadoop.hbase.metrics.Metric;
+import org.apache.hadoop.hbase.metrics.MetricRegistry;
+import org.apache.hadoop.hbase.metrics.MetricRegistryInfo;
+import org.apache.hadoop.hbase.metrics.Timer;
+import org.apache.hadoop.metrics2.MetricsCollector;
+import org.apache.hadoop.metrics2.MetricsInfo;
+import org.apache.hadoop.metrics2.MetricsRecordBuilder;
+import org.apache.hadoop.metrics2.MetricsSource;
+import org.apache.hadoop.metrics2.lib.DefaultMetricsSystem;
+import org.apache.hadoop.metrics2.lib.Interns;
+import org.apache.hadoop.metrics2.lib.MutableHistogram;
+import org.apache.hadoop.metrics2.source.JvmMetrics;
+
+/**
+ * Contents mostly copied from GlobalMetricRegistriesAdapter class from hbase-hadoop2-compat
+ * The adapter attaches HBase's MetricRegistry to Hadoop's DefaultMetricsSystem
+ * Doesn't handle dynamic attach/detach of registries
+ */
+public class GlobalMetricRegistriesAdapter {
+
+ private static final Log LOG = LogFactory.getLog(GlobalMetricRegistriesAdapter.class);
+ private static GlobalMetricRegistriesAdapter INSTANCE;
+
+ private GlobalMetricRegistriesAdapter() {
+ DefaultMetricsSystem.initialize("HBase");
+ JvmMetrics.initSingleton("HBase", "");
--- End diff --
I don't have a strong opinion on how you expose this to let PQS say "I am PQS" (e.g. whether CSV, list<string>, whatever).
But yeah, modifying `snapshotAllMetrics` to include a Hadoop Metrics2 MetricsTag should do the trick.
---
[GitHub] phoenix issue #315: PHOENIX-3655 Global Phoenix Client Metrics for PQS
Posted by karanmehta93 <gi...@git.apache.org>.
Github user karanmehta93 commented on the issue:
https://github.com/apache/phoenix/pull/315
> I'd just leave this as-is for now. I don't think it's "broken" how it is right now (PQS doesn't try to set anything in the Configuration for the PhoenixDriver to pick up).
It's just that some project using PQS wants to change configuration via code, It wont be possible since `HBaseConfiguration.create()` will only read XML on the class path. Either way, I am fine. Let me know your thoughts.
---
[GitHub] phoenix issue #315: PHOENIX-3655 Global Phoenix Client Metrics for PQS
Posted by karanmehta93 <gi...@git.apache.org>.
Github user karanmehta93 commented on the issue:
https://github.com/apache/phoenix/pull/315
The patch might be tricky to port to other branches. Will also look into that once the initial one gets in.
---
[GitHub] phoenix pull request #315: PHOENIX-3655 Global Phoenix Client Metrics for PQ...
Posted by karanmehta93 <gi...@git.apache.org>.
Github user karanmehta93 commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/315#discussion_r207060201
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/monitoring/GlobalMetricRegistriesAdapter.java ---
@@ -0,0 +1,167 @@
+/*
+ * 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.phoenix.monitoring;
+
+import java.util.Iterator;
+import java.util.Map;
+import java.util.Map.Entry;
+import org.apache.commons.lang.StringUtils;
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
+import org.apache.hadoop.hbase.metrics.Counter;
+import org.apache.hadoop.hbase.metrics.Gauge;
+import org.apache.hadoop.hbase.metrics.Histogram;
+import org.apache.hadoop.hbase.metrics.Meter;
+import org.apache.hadoop.hbase.metrics.Metric;
+import org.apache.hadoop.hbase.metrics.MetricRegistry;
+import org.apache.hadoop.hbase.metrics.MetricRegistryInfo;
+import org.apache.hadoop.hbase.metrics.Timer;
+import org.apache.hadoop.metrics2.MetricsCollector;
+import org.apache.hadoop.metrics2.MetricsInfo;
+import org.apache.hadoop.metrics2.MetricsRecordBuilder;
+import org.apache.hadoop.metrics2.MetricsSource;
+import org.apache.hadoop.metrics2.lib.DefaultMetricsSystem;
+import org.apache.hadoop.metrics2.lib.Interns;
+import org.apache.hadoop.metrics2.lib.MutableHistogram;
+import org.apache.hadoop.metrics2.source.JvmMetrics;
+
+/**
+ * Contents mostly copied from GlobalMetricRegistriesAdapter class from hbase-hadoop2-compat
+ * The adapter attaches HBase's MetricRegistry to Hadoop's DefaultMetricsSystem
+ * Doesn't handle dynamic attach/detach of registries
+ */
+public class GlobalMetricRegistriesAdapter {
+
+ private static final Log LOG = LogFactory.getLog(GlobalMetricRegistriesAdapter.class);
+ private static GlobalMetricRegistriesAdapter INSTANCE;
+
+ private GlobalMetricRegistriesAdapter() {
+ DefaultMetricsSystem.initialize("Phoenix");
+ JvmMetrics.initSingleton("Phoenix", "");
+ }
+
+ public static GlobalMetricRegistriesAdapter getInstance() {
+ if (INSTANCE == null) {
+ INSTANCE = new GlobalMetricRegistriesAdapter();
+ }
+ return INSTANCE;
+ }
+
+ public void registerMetricRegistry(MetricRegistry registry) {
+ if (registry == null) {
+ LOG.warn("Registry cannot be registered with Hadoop Metrics 2 since it is null.");
+ return;
+ }
+
+ HBaseMetrics2HadoopMetricsAdapter adapter = new HBaseMetrics2HadoopMetricsAdapter(registry);
+ registerToDefaultMetricsSystem(registry, adapter);
+ }
+
+ private void registerToDefaultMetricsSystem(MetricRegistry registry, HBaseMetrics2HadoopMetricsAdapter adapter) {
+ MetricRegistryInfo info = registry.getMetricRegistryInfo();
+ LOG.info("Registering " + info.getMetricsJmxContext() + " " + info.getMetricsDescription() + " into DefaultMetricsSystem");
+ DefaultMetricsSystem.instance().register(info.getMetricsJmxContext(), info.getMetricsDescription(), adapter);
+ }
+
+ /**
+ * Class to convert HBase Metric Objects to Hadoop Metrics2 Metric Objects
+ */
+ private static class HBaseMetrics2HadoopMetricsAdapter implements MetricsSource {
+ private static final Log LOG = LogFactory.getLog(HBaseMetrics2HadoopMetricsAdapter.class);
+ private final MetricRegistry registry;
+ private HBaseMetrics2HadoopMetricsAdapter(MetricRegistry registry) {
+ this.registry = registry;
+ }
+
+ public void snapshotAllMetrics(MetricRegistry metricRegistry, MetricsCollector collector) {
+ MetricRegistryInfo info = metricRegistry.getMetricRegistryInfo();
+ MetricsRecordBuilder builder = collector.addRecord(Interns.info(info.getMetricsName(), info.getMetricsDescription()));
+ builder.setContext(info.getMetricsContext());
+ this.snapshotAllMetrics(metricRegistry, builder);
+ }
+
+ public void snapshotAllMetrics(MetricRegistry metricRegistry, MetricsRecordBuilder builder) {
+ Map<String, Metric> metrics = metricRegistry.getMetrics();
+ Iterator i$ = metrics.entrySet().iterator();
--- End diff --
Yes, ideally should not have used it here. I copied the code from a class file. Will update it.
---
[GitHub] phoenix pull request #315: PHOENIX-3655 Global Phoenix Client Metrics for PQ...
Posted by karanmehta93 <gi...@git.apache.org>.
Github user karanmehta93 commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/315#discussion_r205899353
--- Diff: phoenix-core/src/test/resources/hadoop-metrics2.properties ---
@@ -32,10 +32,9 @@
# [prefix].[source|sink].[instance].[options]
# See javadoc of package-info.java for org.apache.hadoop.metrics2 for detail
-
-# Don't attempt to start jmx mbeans for all sources.
-# For right now, all metrics are exported to a phoenix table
-*.source.start_mbeans=false
+hbase.source.start_mbeans=true
+hbase.sink.sink0.class=org.apache.phoenix.monitoring.GlobalPhoenixMetricsTestSink
--- End diff --
Sure. It was hard for me as well since things were not working as expected in the first place. Took a little while to realize this file was the reason :)
---
[GitHub] phoenix pull request #315: PHOENIX-3655 Global Phoenix Client Metrics for PQ...
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/315#discussion_r205884048
--- Diff: phoenix-core/src/it/java/org/apache/phoenix/monitoring/PhoenixMetricsIT.java ---
@@ -204,6 +219,39 @@ private static void resetGlobalMetrics() {
}
}
+ private boolean verifyMetricsFromSink() throws InterruptedException {
+ Map<String, Long> expectedMetrics = new HashMap<>();
+ for (GlobalMetric m : PhoenixRuntime.getGlobalPhoenixClientMetrics()) {
+ expectedMetrics.put(m.getMetricType().name(), m.getTotalSum());
+ }
+
+ for (int i = 0; i < MAX_RETRIES; i++) {
+ LOG.info("Verifying Global Metrics from Hadoop Sink, Retry: " + (i + 1));
+ if (verifyMetricsFromSinkOnce(expectedMetrics)) {
+ LOG.info("Values from Hadoop Metrics Sink match actual values");
+ return true;
+ }
+ Thread.sleep(1000);
+ }
+ return false;
+ }
+
+ private boolean verifyMetricsFromSinkOnce(Map<String, Long> expectedMetrics) {
+ synchronized (GlobalPhoenixMetricsTestSink.lock) {
--- End diff --
If it were me, I would have used a Queue or something to save off the metrics, but there's nothing wrong with your approach here :)
---
[GitHub] phoenix pull request #315: PHOENIX-3655 Global Phoenix Client Metrics for PQ...
Posted by xcangCRM <gi...@git.apache.org>.
Github user xcangCRM commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/315#discussion_r207053000
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/monitoring/GlobalMetricRegistriesAdapter.java ---
@@ -0,0 +1,167 @@
+/*
+ * 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.phoenix.monitoring;
+
+import java.util.Iterator;
+import java.util.Map;
+import java.util.Map.Entry;
+import org.apache.commons.lang.StringUtils;
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
+import org.apache.hadoop.hbase.metrics.Counter;
+import org.apache.hadoop.hbase.metrics.Gauge;
+import org.apache.hadoop.hbase.metrics.Histogram;
+import org.apache.hadoop.hbase.metrics.Meter;
+import org.apache.hadoop.hbase.metrics.Metric;
+import org.apache.hadoop.hbase.metrics.MetricRegistry;
+import org.apache.hadoop.hbase.metrics.MetricRegistryInfo;
+import org.apache.hadoop.hbase.metrics.Timer;
+import org.apache.hadoop.metrics2.MetricsCollector;
+import org.apache.hadoop.metrics2.MetricsInfo;
+import org.apache.hadoop.metrics2.MetricsRecordBuilder;
+import org.apache.hadoop.metrics2.MetricsSource;
+import org.apache.hadoop.metrics2.lib.DefaultMetricsSystem;
+import org.apache.hadoop.metrics2.lib.Interns;
+import org.apache.hadoop.metrics2.lib.MutableHistogram;
+import org.apache.hadoop.metrics2.source.JvmMetrics;
+
+/**
+ * Contents mostly copied from GlobalMetricRegistriesAdapter class from hbase-hadoop2-compat
+ * The adapter attaches HBase's MetricRegistry to Hadoop's DefaultMetricsSystem
+ * Doesn't handle dynamic attach/detach of registries
+ */
+public class GlobalMetricRegistriesAdapter {
+
+ private static final Log LOG = LogFactory.getLog(GlobalMetricRegistriesAdapter.class);
+ private static GlobalMetricRegistriesAdapter INSTANCE;
+
+ private GlobalMetricRegistriesAdapter() {
+ DefaultMetricsSystem.initialize("Phoenix");
+ JvmMetrics.initSingleton("Phoenix", "");
+ }
+
+ public static GlobalMetricRegistriesAdapter getInstance() {
+ if (INSTANCE == null) {
+ INSTANCE = new GlobalMetricRegistriesAdapter();
+ }
+ return INSTANCE;
+ }
+
+ public void registerMetricRegistry(MetricRegistry registry) {
+ if (registry == null) {
+ LOG.warn("Registry cannot be registered with Hadoop Metrics 2 since it is null.");
+ return;
+ }
+
+ HBaseMetrics2HadoopMetricsAdapter adapter = new HBaseMetrics2HadoopMetricsAdapter(registry);
+ registerToDefaultMetricsSystem(registry, adapter);
+ }
+
+ private void registerToDefaultMetricsSystem(MetricRegistry registry, HBaseMetrics2HadoopMetricsAdapter adapter) {
+ MetricRegistryInfo info = registry.getMetricRegistryInfo();
+ LOG.info("Registering " + info.getMetricsJmxContext() + " " + info.getMetricsDescription() + " into DefaultMetricsSystem");
+ DefaultMetricsSystem.instance().register(info.getMetricsJmxContext(), info.getMetricsDescription(), adapter);
+ }
+
+ /**
+ * Class to convert HBase Metric Objects to Hadoop Metrics2 Metric Objects
+ */
+ private static class HBaseMetrics2HadoopMetricsAdapter implements MetricsSource {
+ private static final Log LOG = LogFactory.getLog(HBaseMetrics2HadoopMetricsAdapter.class);
+ private final MetricRegistry registry;
+ private HBaseMetrics2HadoopMetricsAdapter(MetricRegistry registry) {
+ this.registry = registry;
+ }
+
+ public void snapshotAllMetrics(MetricRegistry metricRegistry, MetricsCollector collector) {
+ MetricRegistryInfo info = metricRegistry.getMetricRegistryInfo();
+ MetricsRecordBuilder builder = collector.addRecord(Interns.info(info.getMetricsName(), info.getMetricsDescription()));
+ builder.setContext(info.getMetricsContext());
+ this.snapshotAllMetrics(metricRegistry, builder);
+ }
+
+ public void snapshotAllMetrics(MetricRegistry metricRegistry, MetricsRecordBuilder builder) {
+ Map<String, Metric> metrics = metricRegistry.getMetrics();
+ Iterator i$ = metrics.entrySet().iterator();
--- End diff --
why do you need this? i$
---
[GitHub] phoenix pull request #315: PHOENIX-3655 Global Phoenix Client Metrics for PQ...
Posted by karanmehta93 <gi...@git.apache.org>.
Github user karanmehta93 commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/315#discussion_r207054877
--- Diff: phoenix-core/src/test/resources/hadoop-metrics2.properties ---
@@ -32,10 +32,9 @@
# [prefix].[source|sink].[instance].[options]
# See javadoc of package-info.java for org.apache.hadoop.metrics2 for detail
-
-# Don't attempt to start jmx mbeans for all sources.
-# For right now, all metrics are exported to a phoenix table
-*.source.start_mbeans=false
+phoenix.source.start_mbeans=true
+phoenix.sink.sink0.class=org.apache.phoenix.monitoring.GlobalPhoenixMetricsTestSink
--- End diff --
It is just an identifier and can be anything.
---
[GitHub] phoenix pull request #315: PHOENIX-3655 Global Phoenix Client Metrics for PQ...
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/315#discussion_r206598488
--- Diff: phoenix-core/src/it/java/org/apache/phoenix/monitoring/PhoenixMetricsIT.java ---
@@ -204,6 +230,45 @@ private static void resetGlobalMetrics() {
}
}
+ // Phoenix Client Metrics are transported via Hadoop-metrics2 sink
+ // The test sink is defined at GlobalPhoenixMetricsTestSink
+ // Configuration for Hadoop-metrics2 comes from hadoop-metrics2.properties file located in test/resources
--- End diff --
Nice!
---
[GitHub] phoenix pull request #315: PHOENIX-3655 Global Phoenix Client Metrics for PQ...
Posted by karanmehta93 <gi...@git.apache.org>.
Github user karanmehta93 commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/315#discussion_r206616222
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/monitoring/GlobalMetricRegistriesAdapter.java ---
@@ -0,0 +1,167 @@
+/*
+ * 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.phoenix.monitoring;
+
+import java.util.Iterator;
+import java.util.Map;
+import java.util.Map.Entry;
+import org.apache.commons.lang.StringUtils;
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
+import org.apache.hadoop.hbase.metrics.Counter;
+import org.apache.hadoop.hbase.metrics.Gauge;
+import org.apache.hadoop.hbase.metrics.Histogram;
+import org.apache.hadoop.hbase.metrics.Meter;
+import org.apache.hadoop.hbase.metrics.Metric;
+import org.apache.hadoop.hbase.metrics.MetricRegistry;
+import org.apache.hadoop.hbase.metrics.MetricRegistryInfo;
+import org.apache.hadoop.hbase.metrics.Timer;
+import org.apache.hadoop.metrics2.MetricsCollector;
+import org.apache.hadoop.metrics2.MetricsInfo;
+import org.apache.hadoop.metrics2.MetricsRecordBuilder;
+import org.apache.hadoop.metrics2.MetricsSource;
+import org.apache.hadoop.metrics2.lib.DefaultMetricsSystem;
+import org.apache.hadoop.metrics2.lib.Interns;
+import org.apache.hadoop.metrics2.lib.MutableHistogram;
+import org.apache.hadoop.metrics2.source.JvmMetrics;
+
+/**
+ * Contents mostly copied from GlobalMetricRegistriesAdapter class from hbase-hadoop2-compat
+ * The adapter attaches HBase's MetricRegistry to Hadoop's DefaultMetricsSystem
+ * Doesn't handle dynamic attach/detach of registries
+ */
+public class GlobalMetricRegistriesAdapter {
+
+ private static final Log LOG = LogFactory.getLog(GlobalMetricRegistriesAdapter.class);
+ private static GlobalMetricRegistriesAdapter INSTANCE;
+
+ private GlobalMetricRegistriesAdapter() {
+ DefaultMetricsSystem.initialize("HBase");
+ JvmMetrics.initSingleton("HBase", "");
--- End diff --
Actually I added this since this is going to be a part of PQS, where having these metrics would help. Should I cover this with a config parameter then?
---
[GitHub] phoenix pull request #315: PHOENIX-3655 Global Phoenix Client Metrics for PQ...
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/315#discussion_r206598832
--- Diff: phoenix-core/src/it/java/org/apache/phoenix/monitoring/GlobalPhoenixMetricsTestSink.java ---
@@ -0,0 +1,57 @@
+/*
+ * 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.phoenix.monitoring;
+
+import org.apache.commons.configuration.SubsetConfiguration;
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
+import org.apache.hadoop.metrics2.AbstractMetric;
+import org.apache.hadoop.metrics2.MetricsRecord;
+import org.apache.hadoop.metrics2.MetricsSink;
+import org.apache.phoenix.util.PhoenixRuntime;
+
+import java.util.Map;
+
+import static junit.framework.TestCase.assertTrue;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.fail;
+
+public class GlobalPhoenixMetricsTestSink implements MetricsSink {
+
+ public static final String PHOENIX_METRICS_RECORD_NAME = "PHOENIX";
+ static Object lock = new Object();
--- End diff --
Can you leave a comment here about the importance of this lock? e.g. that the caller should hold it to read `metrics`.
---
[GitHub] phoenix pull request #315: PHOENIX-3655 Global Phoenix Client Metrics for PQ...
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/315#discussion_r205884335
--- Diff: phoenix-core/src/it/java/org/apache/phoenix/monitoring/PhoenixMetricsIT.java ---
@@ -204,6 +219,39 @@ private static void resetGlobalMetrics() {
}
}
+ private boolean verifyMetricsFromSink() throws InterruptedException {
+ Map<String, Long> expectedMetrics = new HashMap<>();
+ for (GlobalMetric m : PhoenixRuntime.getGlobalPhoenixClientMetrics()) {
+ expectedMetrics.put(m.getMetricType().name(), m.getTotalSum());
+ }
+
+ for (int i = 0; i < MAX_RETRIES; i++) {
+ LOG.info("Verifying Global Metrics from Hadoop Sink, Retry: " + (i + 1));
+ if (verifyMetricsFromSinkOnce(expectedMetrics)) {
+ LOG.info("Values from Hadoop Metrics Sink match actual values");
+ return true;
+ }
+ Thread.sleep(1000);
+ }
+ return false;
+ }
+
+ private boolean verifyMetricsFromSinkOnce(Map<String, Long> expectedMetrics) {
+ synchronized (GlobalPhoenixMetricsTestSink.lock) {
+ for (AbstractMetric metric : GlobalPhoenixMetricsTestSink.metrics) {
+ if (expectedMetrics.containsKey(metric.name())) {
--- End diff --
This check doesn't catch the case where you have `expectedMetrics` but `GlobalPhoenixMetricsTestSink.metrics` is empty. I'd switch this around to iterate over your expectations, ensuring that they all exist.
---
[GitHub] phoenix issue #315: PHOENIX-3655 Global Phoenix Client Metrics for PQS
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on the issue:
https://github.com/apache/phoenix/pull/315
> Is it fine for me to change
I'd just leave this as-is for now. I don't think it's "broken" how it is right now (PQS doesn't try to set anything in the Configuration for the PhoenixDriver to pick up).
---
[GitHub] phoenix pull request #315: PHOENIX-3655 Global Phoenix Client Metrics for PQ...
Posted by xcangCRM <gi...@git.apache.org>.
Github user xcangCRM commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/315#discussion_r207053124
--- Diff: phoenix-core/src/test/resources/hadoop-metrics2.properties ---
@@ -32,10 +32,9 @@
# [prefix].[source|sink].[instance].[options]
# See javadoc of package-info.java for org.apache.hadoop.metrics2 for detail
-
-# Don't attempt to start jmx mbeans for all sources.
-# For right now, all metrics are exported to a phoenix table
-*.source.start_mbeans=false
+phoenix.source.start_mbeans=true
+phoenix.sink.sink0.class=org.apache.phoenix.monitoring.GlobalPhoenixMetricsTestSink
--- End diff --
I think it usually starts with sink 1 . (?) at least ni HBase.
---
[GitHub] phoenix issue #315: PHOENIX-3655 Global Phoenix Client Metrics for PQS
Posted by karanmehta93 <gi...@git.apache.org>.
Github user karanmehta93 commented on the issue:
https://github.com/apache/phoenix/pull/315
> The setup of GlobalClientMetrics just... doesn't give us any way to do this nicely
Yes, agreed on that one.
> I'm not sure what it takes to write a custom Factory (I think I did it once in an IT), but would be OK if you defer this
Checkout `PhoenixMetricsDisabledIT` in this patch. I am doing the same.
> After you commit this, please update the website with these necessary changes (e.g. at least tell people that PQS exports metrics).
Sure will file a new Jira for documentation change as well.
One last question, Is it fine for me to change the `QueryServer#main()` to use `HBaseFactoryProvider.getConfigurationFactory().getConfiguration()` instead of the regular one. We can control the config from other projects, if required. What do you think?
---
[GitHub] phoenix pull request #315: PHOENIX-3655 Global Phoenix Client Metrics for PQ...
Posted by karanmehta93 <gi...@git.apache.org>.
Github user karanmehta93 commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/315#discussion_r207054938
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/monitoring/GlobalMetricRegistriesAdapter.java ---
@@ -0,0 +1,167 @@
+/*
+ * 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.phoenix.monitoring;
+
+import java.util.Iterator;
+import java.util.Map;
+import java.util.Map.Entry;
+import org.apache.commons.lang.StringUtils;
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
+import org.apache.hadoop.hbase.metrics.Counter;
+import org.apache.hadoop.hbase.metrics.Gauge;
+import org.apache.hadoop.hbase.metrics.Histogram;
+import org.apache.hadoop.hbase.metrics.Meter;
+import org.apache.hadoop.hbase.metrics.Metric;
+import org.apache.hadoop.hbase.metrics.MetricRegistry;
+import org.apache.hadoop.hbase.metrics.MetricRegistryInfo;
+import org.apache.hadoop.hbase.metrics.Timer;
+import org.apache.hadoop.metrics2.MetricsCollector;
+import org.apache.hadoop.metrics2.MetricsInfo;
+import org.apache.hadoop.metrics2.MetricsRecordBuilder;
+import org.apache.hadoop.metrics2.MetricsSource;
+import org.apache.hadoop.metrics2.lib.DefaultMetricsSystem;
+import org.apache.hadoop.metrics2.lib.Interns;
+import org.apache.hadoop.metrics2.lib.MutableHistogram;
+import org.apache.hadoop.metrics2.source.JvmMetrics;
+
+/**
+ * Contents mostly copied from GlobalMetricRegistriesAdapter class from hbase-hadoop2-compat
+ * The adapter attaches HBase's MetricRegistry to Hadoop's DefaultMetricsSystem
+ * Doesn't handle dynamic attach/detach of registries
+ */
+public class GlobalMetricRegistriesAdapter {
+
+ private static final Log LOG = LogFactory.getLog(GlobalMetricRegistriesAdapter.class);
+ private static GlobalMetricRegistriesAdapter INSTANCE;
+
+ private GlobalMetricRegistriesAdapter() {
+ DefaultMetricsSystem.initialize("HBase");
+ JvmMetrics.initSingleton("HBase", "");
--- End diff --
Cool. Thanks!
---