You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@zookeeper.apache.org by GitBox <gi...@apache.org> on 2021/01/09 01:25:30 UTC
[GitHub] [zookeeper] Shoothzj opened a new pull request #1574: ZOOKEEPER-4054: Make prometheus listen host can configure
Shoothzj opened a new pull request #1574:
URL: https://github.com/apache/zookeeper/pull/1574
Make prometheus listen host can configure listen IP, helpful to security. default is 0.0.0.0
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] Shoothzj commented on pull request #1574: ZOOKEEPER-4054: Make prometheus listen host can configure
Posted by GitBox <gi...@apache.org>.
Shoothzj commented on pull request #1574:
URL: https://github.com/apache/zookeeper/pull/1574#issuecomment-782848270
> * Please add the doc about the new property: **metricsProvider.httpHost** in the **zookeeperAdmin.md**
> * Please write an unit case to cover this change
PTLA
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] maoling commented on a change in pull request #1574: ZOOKEEPER-4054: Make prometheus listen host can configure
Posted by GitBox <gi...@apache.org>.
maoling commented on a change in pull request #1574:
URL: https://github.com/apache/zookeeper/pull/1574#discussion_r584108559
##########
File path: zookeeper-docs/src/main/resources/markdown/zookeeperAdmin.md
##########
@@ -2068,6 +2068,9 @@ options are used to configure the [AdminServer](#sc_adminserver).
Set to "org.apache.zookeeper.metrics.prometheus.PrometheusMetricsProvider" to
enable Prometheus.io exporter.
+* *metricsProvider.httpHost* :
+ Prometheus.io exporter will start a Jetty server and listen this addr, default is "0.0.0.0"
Review comment:
listen to this address
##########
File path: zookeeper-metrics-providers/zookeeper-prometheus-metrics/src/test/java/org/apache/zookeeper/metrics/prometheus/PrometheusMetricsProviderConfigTest.java
##########
@@ -0,0 +1,42 @@
+package org.apache.zookeeper.metrics.prometheus;
+
+import io.prometheus.client.CollectorRegistry;
+import org.apache.zookeeper.metrics.MetricsProviderLifeCycleException;
+import org.junit.Assert;
+import org.junit.Test;
+
+import java.util.Properties;
+
+/**
+ * @author hezhangjian
+ */
+public class PrometheusMetricsProviderConfigTest {
+
Review comment:
this unit test is great. we also need to write some cases for the valid address and port?
##########
File path: zookeeper-metrics-providers/zookeeper-prometheus-metrics/src/test/java/org/apache/zookeeper/metrics/prometheus/PrometheusMetricsProviderTest.java
##########
@@ -56,6 +56,7 @@ public void setup() throws Exception {
CollectorRegistry.defaultRegistry.clear();
provider = new PrometheusMetricsProvider();
Properties configuration = new Properties();
+ configuration.setProperty("httpHost", "127.0.0.1"); // ephemeral port
configuration.setProperty("httpPort", "0"); // ephemeral port
Review comment:
ephemeral hostname? :)
##########
File path: zookeeper-metrics-providers/zookeeper-prometheus-metrics/src/test/java/org/apache/zookeeper/metrics/prometheus/PrometheusMetricsProviderTest.java
##########
@@ -166,9 +165,7 @@ public void testGauge() throws Exception {
// override gauge, without unregister
provider.getRootContext().registerGauge("gg", gauge0);
- provider.dump((k, v) -> {
- count[0]++;
- }
+ provider.dump((k, v) -> count[0]++
);
Review comment:
Please don't do the unrelated refactor in this PR
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] Shoothzj commented on a change in pull request #1574: ZOOKEEPER-4054: Make prometheus listen host can configure
Posted by GitBox <gi...@apache.org>.
Shoothzj commented on a change in pull request #1574:
URL: https://github.com/apache/zookeeper/pull/1574#discussion_r584209670
##########
File path: zookeeper-metrics-providers/zookeeper-prometheus-metrics/src/test/java/org/apache/zookeeper/metrics/prometheus/PrometheusMetricsProviderTest.java
##########
@@ -166,9 +165,7 @@ public void testGauge() throws Exception {
// override gauge, without unregister
provider.getRootContext().registerGauge("gg", gauge0);
- provider.dump((k, v) -> {
- count[0]++;
- }
+ provider.dump((k, v) -> count[0]++
);
Review comment:
reverted
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] maoling closed pull request #1574: ZOOKEEPER-4054: Make prometheus listen host can configure
Posted by GitBox <gi...@apache.org>.
maoling closed pull request #1574:
URL: https://github.com/apache/zookeeper/pull/1574
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] asfgit closed pull request #1574: ZOOKEEPER-4054: Make prometheus listen host can configure
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #1574:
URL: https://github.com/apache/zookeeper/pull/1574
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] Shoothzj edited a comment on pull request #1574: ZOOKEEPER-4054: Make prometheus listen host can configure
Posted by GitBox <gi...@apache.org>.
Shoothzj edited a comment on pull request #1574:
URL: https://github.com/apache/zookeeper/pull/1574#issuecomment-809014582
> @Shoothzj I retry the CI, it became green. But when I run the UT(`testInvalidAddr`) in my local mac:
>
> ```
> org.junit.ComparisonFailure:
> Expected :java.net.SocketException: Unresolved address
> Actual :java.io.IOException: Failed to bind to 256.256.256.256:7000
> ```
>
> What's wrong with me? or the test is flaky?
I test it on my local mac too. `jdk8u251` and `jdk14.0.1`. I think may be the class **IPAddressUtil**'s method `textToNumericFormatV4` think `256.256.256.256` as an valid ip address.
Could you tell me your jdk version and which jdk you use?
If it's unstable, may be it's better to change a testcase to listen to invalid domain name like this , what do you think ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] Shoothzj commented on pull request #1574: ZOOKEEPER-4054: Make prometheus listen host can configure
Posted by GitBox <gi...@apache.org>.
Shoothzj commented on pull request #1574:
URL: https://github.com/apache/zookeeper/pull/1574#issuecomment-780247006
@maoling could you help looking at this? thanks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] Shoothzj commented on a change in pull request #1574: ZOOKEEPER-4054: Make prometheus listen host can configure
Posted by GitBox <gi...@apache.org>.
Shoothzj commented on a change in pull request #1574:
URL: https://github.com/apache/zookeeper/pull/1574#discussion_r628998284
##########
File path: zookeeper-docs/src/main/resources/markdown/zookeeperAdmin.md
##########
@@ -2068,6 +2068,9 @@ options are used to configure the [AdminServer](#sc_adminserver).
Set to "org.apache.zookeeper.metrics.prometheus.PrometheusMetricsProvider" to
enable Prometheus.io exporter.
+* *metricsProvider.httpHost* :
+ Prometheus.io exporter will start a Jetty server and listen this address, default is "0.0.0.0"
+
Review comment:
You mean before the "Promtheus" I missed two spaces? Fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] maoling commented on a change in pull request #1574: ZOOKEEPER-4054: Make prometheus listen host can configure
Posted by GitBox <gi...@apache.org>.
maoling commented on a change in pull request #1574:
URL: https://github.com/apache/zookeeper/pull/1574#discussion_r579789496
##########
File path: zookeeper-metrics-providers/zookeeper-prometheus-metrics/src/main/java/org/apache/zookeeper/metrics/prometheus/PrometheusMetricsProvider.java
##########
@@ -83,7 +86,7 @@ public void start() throws MetricsProviderLifeCycleException {
if (exportJvmInfo) {
Review comment:
also logging the **hostname** here ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] Shoothzj commented on a change in pull request #1574: ZOOKEEPER-4054: Make prometheus listen host can configure
Posted by GitBox <gi...@apache.org>.
Shoothzj commented on a change in pull request #1574:
URL: https://github.com/apache/zookeeper/pull/1574#discussion_r593853439
##########
File path: zookeeper-metrics-providers/zookeeper-prometheus-metrics/src/test/java/org/apache/zookeeper/metrics/prometheus/PrometheusMetricsProviderTest.java
##########
@@ -56,6 +56,7 @@ public void setup() throws Exception {
CollectorRegistry.defaultRegistry.clear();
provider = new PrometheusMetricsProvider();
Properties configuration = new Properties();
+ configuration.setProperty("httpHost", "127.0.0.1"); // local host for test
configuration.setProperty("httpPort", "0"); // ephemeral port
Review comment:
I think the port 0 is enough to covert this PR.
By the way, try to import class `PortAssignment` which is a test class in `zookeeper-server` module, can't imported easily.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] Shoothzj commented on pull request #1574: ZOOKEEPER-4054: Make prometheus listen host can configure
Posted by GitBox <gi...@apache.org>.
Shoothzj commented on pull request #1574:
URL: https://github.com/apache/zookeeper/pull/1574#issuecomment-809014582
> @Shoothzj I retry the CI, it became green. But when I run the UT(`testInvalidAddr`) in my local mac:
>
> ```
> org.junit.ComparisonFailure:
> Expected :java.net.SocketException: Unresolved address
> Actual :java.io.IOException: Failed to bind to 256.256.256.256:7000
> ```
>
> What's wrong with me? or the test is flaky?
I test it on my local mac too. `jdk8u251` and `jdk14.0.1`. I think may be the class **IPAddressUtil**'s method `textToNumericFormatV4` think `256.256.256.256` as an valid ip address.
Could you tell me your jdk version and which jdk you use?
May be it's better to add a testcase to listen to invalid domain name, what do you think ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] Shoothzj commented on a change in pull request #1574: ZOOKEEPER-4054: Make prometheus listen host can configure
Posted by GitBox <gi...@apache.org>.
Shoothzj commented on a change in pull request #1574:
URL: https://github.com/apache/zookeeper/pull/1574#discussion_r584209659
##########
File path: zookeeper-metrics-providers/zookeeper-prometheus-metrics/src/test/java/org/apache/zookeeper/metrics/prometheus/PrometheusMetricsProviderTest.java
##########
@@ -56,6 +56,7 @@ public void setup() throws Exception {
CollectorRegistry.defaultRegistry.clear();
provider = new PrometheusMetricsProvider();
Properties configuration = new Properties();
+ configuration.setProperty("httpHost", "127.0.0.1"); // ephemeral port
configuration.setProperty("httpPort", "0"); // ephemeral port
Review comment:
It's a mistake, it should be "local host for test"
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] Shoothzj commented on a change in pull request #1574: ZOOKEEPER-4054: Make prometheus listen host can configure
Posted by GitBox <gi...@apache.org>.
Shoothzj commented on a change in pull request #1574:
URL: https://github.com/apache/zookeeper/pull/1574#discussion_r584209697
##########
File path: zookeeper-docs/src/main/resources/markdown/zookeeperAdmin.md
##########
@@ -2068,6 +2068,9 @@ options are used to configure the [AdminServer](#sc_adminserver).
Set to "org.apache.zookeeper.metrics.prometheus.PrometheusMetricsProvider" to
enable Prometheus.io exporter.
+* *metricsProvider.httpHost* :
+ Prometheus.io exporter will start a Jetty server and listen this addr, default is "0.0.0.0"
Review comment:
fixed
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] maoling commented on pull request #1574: ZOOKEEPER-4054: Make prometheus listen host can configure
Posted by GitBox <gi...@apache.org>.
maoling commented on pull request #1574:
URL: https://github.com/apache/zookeeper/pull/1574#issuecomment-808911418
@Shoothzj I retry the CI, it became green. But when I run the UT(`testInvalidAddr`) in my local mac:
```
org.junit.ComparisonFailure:
Expected :java.net.SocketException: Unresolved address
Actual :java.io.IOException: Failed to bind to 256.256.256.256:7000
```
What's wrong with me? or the test is flaky?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] maoling commented on pull request #1574: ZOOKEEPER-4054: Make prometheus listen host can configure
Posted by GitBox <gi...@apache.org>.
maoling commented on pull request #1574:
URL: https://github.com/apache/zookeeper/pull/1574#issuecomment-792729652
@eolivelli PTAL :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] maoling commented on pull request #1574: ZOOKEEPER-4054: Make prometheus listen host can configure
Posted by GitBox <gi...@apache.org>.
maoling commented on pull request #1574:
URL: https://github.com/apache/zookeeper/pull/1574#issuecomment-807915029
@Shoothzj Sorry for my late, I'll recheck this patch at this weekend, if no other concerns, I'll merge it at this weekend(03-27 ~ 03-28). Cc @eolivelli
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] Shoothzj commented on pull request #1574: ZOOKEEPER-4054: Make prometheus listen host can configure
Posted by GitBox <gi...@apache.org>.
Shoothzj commented on pull request #1574:
URL: https://github.com/apache/zookeeper/pull/1574#issuecomment-833195205
@maoling Could you please take a look this weekend?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] Shoothzj commented on pull request #1574: ZOOKEEPER-4054: Make prometheus listen host can configure
Posted by GitBox <gi...@apache.org>.
Shoothzj commented on pull request #1574:
URL: https://github.com/apache/zookeeper/pull/1574#issuecomment-757403786
@maoling How do I deal with this problem? should I retry?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] Shoothzj commented on a change in pull request #1574: ZOOKEEPER-4054: Make prometheus listen host can configure
Posted by GitBox <gi...@apache.org>.
Shoothzj commented on a change in pull request #1574:
URL: https://github.com/apache/zookeeper/pull/1574#discussion_r584238807
##########
File path: zookeeper-metrics-providers/zookeeper-prometheus-metrics/src/test/java/org/apache/zookeeper/metrics/prometheus/PrometheusMetricsProviderConfigTest.java
##########
@@ -0,0 +1,42 @@
+package org.apache.zookeeper.metrics.prometheus;
+
+import io.prometheus.client.CollectorRegistry;
+import org.apache.zookeeper.metrics.MetricsProviderLifeCycleException;
+import org.junit.Assert;
+import org.junit.Test;
+
+import java.util.Properties;
+
+/**
+ * @author hezhangjian
+ */
+public class PrometheusMetricsProviderConfigTest {
+
Review comment:
Added a Valid host and port test. The abality test should be tested in PrometheusMetricsProviderTest.
I am not sure if test a specify port is properly, so i use port 0 for test
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] maoling commented on a change in pull request #1574: ZOOKEEPER-4054: Make prometheus listen host can configure
Posted by GitBox <gi...@apache.org>.
maoling commented on a change in pull request #1574:
URL: https://github.com/apache/zookeeper/pull/1574#discussion_r628871719
##########
File path: zookeeper-docs/src/main/resources/markdown/zookeeperAdmin.md
##########
@@ -2068,6 +2068,9 @@ options are used to configure the [AdminServer](#sc_adminserver).
Set to "org.apache.zookeeper.metrics.prometheus.PrometheusMetricsProvider" to
enable Prometheus.io exporter.
+* *metricsProvider.httpHost* :
+ Prometheus.io exporter will start a Jetty server and listen this address, default is "0.0.0.0"
+
Review comment:
here has whitespace, please don't have it next time:)
##########
File path: zookeeper-metrics-providers/zookeeper-prometheus-metrics/src/test/java/org/apache/zookeeper/metrics/prometheus/PrometheusMetricsProviderConfigTest.java
##########
@@ -0,0 +1,72 @@
+/*
+ * 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.zookeeper.metrics.prometheus;
+
+import io.prometheus.client.CollectorRegistry;
+
+import java.util.Properties;
+
+import org.apache.zookeeper.metrics.MetricsProviderLifeCycleException;
+import org.junit.Assert;
+import org.junit.Test;
+
+
+public class PrometheusMetricsProviderConfigTest {
+
+ @Test
+ public void testInvalidPort() {
+ MetricsProviderLifeCycleException exception = Assert
+ .assertThrows(MetricsProviderLifeCycleException.class, () -> {
+ CollectorRegistry.defaultRegistry.clear();
+ PrometheusMetricsProvider provider = new PrometheusMetricsProvider();
+ Properties configuration = new Properties();
+ configuration.setProperty("httpPort", "65536");
+ configuration.setProperty("exportJvmInfo", "false");
+ provider.configure(configuration);
+ provider.start();
+ });
+ Assert.assertEquals("java.lang.IllegalArgumentException: port out of range:65536", exception.getMessage());
+ }
+
+ @Test
+ public void testInvalidAddr() {
+ MetricsProviderLifeCycleException exception = Assert
+ .assertThrows(MetricsProviderLifeCycleException.class, () -> {
+ CollectorRegistry.defaultRegistry.clear();
+ PrometheusMetricsProvider provider = new PrometheusMetricsProvider();
+ Properties configuration = new Properties();
+ configuration.setProperty("httpHost", "256.256.256.256");
Review comment:
- Yep. ` configuration.setProperty("httpHost", "master");` is better.
##########
File path: zookeeper-metrics-providers/zookeeper-prometheus-metrics/src/test/java/org/apache/zookeeper/metrics/prometheus/PrometheusMetricsProviderConfigTest.java
##########
@@ -0,0 +1,72 @@
+/*
+ * 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.zookeeper.metrics.prometheus;
+
+import io.prometheus.client.CollectorRegistry;
+
+import java.util.Properties;
+
+import org.apache.zookeeper.metrics.MetricsProviderLifeCycleException;
+import org.junit.Assert;
+import org.junit.Test;
+
+
+public class PrometheusMetricsProviderConfigTest {
+
+ @Test
+ public void testInvalidPort() {
+ MetricsProviderLifeCycleException exception = Assert
+ .assertThrows(MetricsProviderLifeCycleException.class, () -> {
+ CollectorRegistry.defaultRegistry.clear();
+ PrometheusMetricsProvider provider = new PrometheusMetricsProvider();
+ Properties configuration = new Properties();
+ configuration.setProperty("httpPort", "65536");
+ configuration.setProperty("exportJvmInfo", "false");
+ provider.configure(configuration);
+ provider.start();
+ });
+ Assert.assertEquals("java.lang.IllegalArgumentException: port out of range:65536", exception.getMessage());
+ }
+
+ @Test
+ public void testInvalidAddr() {
+ MetricsProviderLifeCycleException exception = Assert
+ .assertThrows(MetricsProviderLifeCycleException.class, () -> {
+ CollectorRegistry.defaultRegistry.clear();
+ PrometheusMetricsProvider provider = new PrometheusMetricsProvider();
+ Properties configuration = new Properties();
+ configuration.setProperty("httpHost", "256.256.256.256");
+ provider.configure(configuration);
+ provider.start();
+ });
+ Assert.assertEquals("java.net.SocketException: Unresolved address", exception.getMessage());
Review comment:
- You have asserted there was a `MetricsProviderLifeCycleException` happens here, so don't make so specify assertion here to avoid the environment diversity？So removing this line?
##########
File path: zookeeper-docs/src/main/resources/markdown/zookeeperAdmin.md
##########
@@ -2068,6 +2068,9 @@ options are used to configure the [AdminServer](#sc_adminserver).
Set to "org.apache.zookeeper.metrics.prometheus.PrometheusMetricsProvider" to
enable Prometheus.io exporter.
+* *metricsProvider.httpHost* :
+ Prometheus.io exporter will start a Jetty server and listen this address, default is "0.0.0.0"
Review comment:
Add this: **New in 3.8.0:** ?
##########
File path: zookeeper-metrics-providers/zookeeper-prometheus-metrics/src/main/java/org/apache/zookeeper/metrics/prometheus/PrometheusMetricsProvider.java
##########
@@ -72,18 +74,20 @@
@Override
public void configure(Properties configuration) throws MetricsProviderLifeCycleException {
LOG.info("Initializing metrics, configuration: {}", configuration);
+ this.host = configuration.getProperty("httpHost", "0.0.0.0");
this.port = Integer.parseInt(configuration.getProperty("httpPort", "7000"));
this.exportJvmInfo = Boolean.parseBoolean(configuration.getProperty("exportJvmInfo", "true"));
}
@Override
public void start() throws MetricsProviderLifeCycleException {
try {
- LOG.info("Starting /metrics HTTP endpoint at port {} exportJvmInfo: {}", port, exportJvmInfo);
+ LOG.info("Starting /metrics HTTP endpoint at port {} listening {} exportJvmInfo: {}",
+ port, host, exportJvmInfo);
Review comment:
The following is better?
```
LOG.info("Starting /metrics HTTP endpoint at host: {}, port: {}, exportJvmInfo: {}",
host, port, exportJvmInfo);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] maoling closed pull request #1574: ZOOKEEPER-4054: Make prometheus listen host can configure
Posted by GitBox <gi...@apache.org>.
maoling closed pull request #1574:
URL: https://github.com/apache/zookeeper/pull/1574
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] Shoothzj commented on pull request #1574: ZOOKEEPER-4054: Make prometheus listen host can configure
Posted by GitBox <gi...@apache.org>.
Shoothzj commented on pull request #1574:
URL: https://github.com/apache/zookeeper/pull/1574#issuecomment-787045169
@maoling PTAL, And all checks have passed
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] Shoothzj commented on a change in pull request #1574: ZOOKEEPER-4054: Make prometheus listen host can configure
Posted by GitBox <gi...@apache.org>.
Shoothzj commented on a change in pull request #1574:
URL: https://github.com/apache/zookeeper/pull/1574#discussion_r584209358
##########
File path: zookeeper-metrics-providers/zookeeper-prometheus-metrics/src/test/java/org/apache/zookeeper/metrics/prometheus/PrometheusMetricsProviderConfigTest.java
##########
@@ -0,0 +1,42 @@
+package org.apache.zookeeper.metrics.prometheus;
+
+import io.prometheus.client.CollectorRegistry;
+import org.apache.zookeeper.metrics.MetricsProviderLifeCycleException;
+import org.junit.Assert;
+import org.junit.Test;
+
+import java.util.Properties;
+
+/**
+ * @author hezhangjian
+ */
+public class PrometheusMetricsProviderConfigTest {
+
Review comment:
I think the PrometheusMetricsProviderTest is already a valid test case.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] Shoothzj commented on a change in pull request #1574: ZOOKEEPER-4054: Make prometheus listen host can configure
Posted by GitBox <gi...@apache.org>.
Shoothzj commented on a change in pull request #1574:
URL: https://github.com/apache/zookeeper/pull/1574#discussion_r628997963
##########
File path: zookeeper-metrics-providers/zookeeper-prometheus-metrics/src/test/java/org/apache/zookeeper/metrics/prometheus/PrometheusMetricsProviderConfigTest.java
##########
@@ -0,0 +1,72 @@
+/*
+ * 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.zookeeper.metrics.prometheus;
+
+import io.prometheus.client.CollectorRegistry;
+
+import java.util.Properties;
+
+import org.apache.zookeeper.metrics.MetricsProviderLifeCycleException;
+import org.junit.Assert;
+import org.junit.Test;
+
+
+public class PrometheusMetricsProviderConfigTest {
+
+ @Test
+ public void testInvalidPort() {
+ MetricsProviderLifeCycleException exception = Assert
+ .assertThrows(MetricsProviderLifeCycleException.class, () -> {
+ CollectorRegistry.defaultRegistry.clear();
+ PrometheusMetricsProvider provider = new PrometheusMetricsProvider();
+ Properties configuration = new Properties();
+ configuration.setProperty("httpPort", "65536");
+ configuration.setProperty("exportJvmInfo", "false");
+ provider.configure(configuration);
+ provider.start();
+ });
+ Assert.assertEquals("java.lang.IllegalArgumentException: port out of range:65536", exception.getMessage());
+ }
+
+ @Test
+ public void testInvalidAddr() {
+ MetricsProviderLifeCycleException exception = Assert
+ .assertThrows(MetricsProviderLifeCycleException.class, () -> {
+ CollectorRegistry.defaultRegistry.clear();
+ PrometheusMetricsProvider provider = new PrometheusMetricsProvider();
+ Properties configuration = new Properties();
+ configuration.setProperty("httpHost", "256.256.256.256");
+ provider.configure(configuration);
+ provider.start();
+ });
+ Assert.assertEquals("java.net.SocketException: Unresolved address", exception.getMessage());
Review comment:
Removed specify assertion
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] maoling closed pull request #1574: ZOOKEEPER-4054: Make prometheus listen host can configure
Posted by GitBox <gi...@apache.org>.
maoling closed pull request #1574:
URL: https://github.com/apache/zookeeper/pull/1574
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] Shoothzj closed pull request #1574: ZOOKEEPER-4054: Make prometheus listen host can configure
Posted by GitBox <gi...@apache.org>.
Shoothzj closed pull request #1574:
URL: https://github.com/apache/zookeeper/pull/1574
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] asfgit closed pull request #1574: ZOOKEEPER-4054: Make prometheus listen host can configure
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #1574:
URL: https://github.com/apache/zookeeper/pull/1574
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] maoling commented on pull request #1574: ZOOKEEPER-4054: Make prometheus listen host can configure
Posted by GitBox <gi...@apache.org>.
maoling commented on pull request #1574:
URL: https://github.com/apache/zookeeper/pull/1574#issuecomment-841766320
@Shoothzj don't worry about anything. I close and reopen the PR, just for kick off the CI.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] Shoothzj commented on pull request #1574: ZOOKEEPER-4054: Make prometheus listen host can configure
Posted by GitBox <gi...@apache.org>.
Shoothzj commented on pull request #1574:
URL: https://github.com/apache/zookeeper/pull/1574#issuecomment-841955752
> @Shoothzj The CI failure was caused by this PR, please take a look
>
> ```
> diff --git a/zookeeper-server/src/test/resources/data/invalidsnap/version-2/snapshot.83f b/zookeeper-server/src/test/resources/data/invalidsnap/version-2/snapshot.83f
> deleted file mode 100644
> index 26dc5f665..000000000
> Binary files a/zookeeper-server/src/test/resources/data/invalidsnap/version-2/snapshot.83f and /dev/null differ
>
> [ERROR] Tests run: 4, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 1.072 s <<< FAILURE! - in org.apache.zookeeper.test.InvalidSnapshotTest
> [ERROR] testSnapshotFormatterWithInvalidSnap Time elapsed: 0.034 s <<< FAILURE!
> org.opentest4j.AssertionFailedError: expected: <true> but was: <false>
> at org.apache.zookeeper.test.InvalidSnapshotTest.testSnapshotFormatterWithInvalidSnap(InvalidSnapshotTest.java:81)
> ```
@maoling
Fixed the tests.
I am sorry for my careless. The mistake must be introduced by running、debuging tests. And github fold the binary file..
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] maoling commented on pull request #1574: ZOOKEEPER-4054: Make prometheus listen host can configure
Posted by GitBox <gi...@apache.org>.
maoling commented on pull request #1574:
URL: https://github.com/apache/zookeeper/pull/1574#issuecomment-781299994
@Shoothzj
Sorry for our late, I'll take a look at this weekend. Thanks for your contribution.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] maoling commented on pull request #1574: ZOOKEEPER-4054: Make prometheus listen host can configure
Posted by GitBox <gi...@apache.org>.
maoling commented on pull request #1574:
URL: https://github.com/apache/zookeeper/pull/1574#issuecomment-841829567
@Shoothzj The CI failure was caused by this PR, please take a look
```
diff --git a/zookeeper-server/src/test/resources/data/invalidsnap/version-2/snapshot.83f b/zookeeper-server/src/test/resources/data/invalidsnap/version-2/snapshot.83f
deleted file mode 100644
index 26dc5f665..000000000
Binary files a/zookeeper-server/src/test/resources/data/invalidsnap/version-2/snapshot.83f and /dev/null differ
[ERROR] Tests run: 4, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 1.072 s <<< FAILURE! - in org.apache.zookeeper.test.InvalidSnapshotTest
[ERROR] testSnapshotFormatterWithInvalidSnap Time elapsed: 0.034 s <<< FAILURE!
org.opentest4j.AssertionFailedError: expected: <true> but was: <false>
at org.apache.zookeeper.test.InvalidSnapshotTest.testSnapshotFormatterWithInvalidSnap(InvalidSnapshotTest.java:81)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] Shoothzj commented on pull request #1574: ZOOKEEPER-4054: Make prometheus listen host can configure
Posted by GitBox <gi...@apache.org>.
Shoothzj commented on pull request #1574:
URL: https://github.com/apache/zookeeper/pull/1574#issuecomment-841747076
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] maoling closed pull request #1574: ZOOKEEPER-4054: Make prometheus listen host can configure
Posted by GitBox <gi...@apache.org>.
maoling closed pull request #1574:
URL: https://github.com/apache/zookeeper/pull/1574
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] Shoothzj commented on a change in pull request #1574: ZOOKEEPER-4054: Make prometheus listen host can configure
Posted by GitBox <gi...@apache.org>.
Shoothzj commented on a change in pull request #1574:
URL: https://github.com/apache/zookeeper/pull/1574#discussion_r584209358
##########
File path: zookeeper-metrics-providers/zookeeper-prometheus-metrics/src/test/java/org/apache/zookeeper/metrics/prometheus/PrometheusMetricsProviderConfigTest.java
##########
@@ -0,0 +1,42 @@
+package org.apache.zookeeper.metrics.prometheus;
+
+import io.prometheus.client.CollectorRegistry;
+import org.apache.zookeeper.metrics.MetricsProviderLifeCycleException;
+import org.junit.Assert;
+import org.junit.Test;
+
+import java.util.Properties;
+
+/**
+ * @author hezhangjian
+ */
+public class PrometheusMetricsProviderConfigTest {
+
Review comment:
I think the PrometheusMetricsProviderTest is already a valid test case.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] maoling commented on pull request #1574: ZOOKEEPER-4054: Make prometheus listen host can configure
Posted by GitBox <gi...@apache.org>.
maoling commented on pull request #1574:
URL: https://github.com/apache/zookeeper/pull/1574#issuecomment-765224019
@Shoothzj
- The failure was caused by `C client`. For the CI failure, you can close this PR then reopen it or
`git commit --amend` & `git push origin your-branch -f ` to trigger the CI build
- @eolivelli PTAL. If he is not free, I'll take a look at this weekend.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] Shoothzj commented on a change in pull request #1574: ZOOKEEPER-4054: Make prometheus listen host can configure
Posted by GitBox <gi...@apache.org>.
Shoothzj commented on a change in pull request #1574:
URL: https://github.com/apache/zookeeper/pull/1574#discussion_r628997433
##########
File path: zookeeper-metrics-providers/zookeeper-prometheus-metrics/src/main/java/org/apache/zookeeper/metrics/prometheus/PrometheusMetricsProvider.java
##########
@@ -72,18 +74,20 @@
@Override
public void configure(Properties configuration) throws MetricsProviderLifeCycleException {
LOG.info("Initializing metrics, configuration: {}", configuration);
+ this.host = configuration.getProperty("httpHost", "0.0.0.0");
this.port = Integer.parseInt(configuration.getProperty("httpPort", "7000"));
this.exportJvmInfo = Boolean.parseBoolean(configuration.getProperty("exportJvmInfo", "true"));
}
@Override
public void start() throws MetricsProviderLifeCycleException {
try {
- LOG.info("Starting /metrics HTTP endpoint at port {} exportJvmInfo: {}", port, exportJvmInfo);
+ LOG.info("Starting /metrics HTTP endpoint at port {} listening {} exportJvmInfo: {}",
+ port, host, exportJvmInfo);
Review comment:
Yes , it's better
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] Shoothzj commented on a change in pull request #1574: ZOOKEEPER-4054: Make prometheus listen host can configure
Posted by GitBox <gi...@apache.org>.
Shoothzj commented on a change in pull request #1574:
URL: https://github.com/apache/zookeeper/pull/1574#discussion_r628998104
##########
File path: zookeeper-docs/src/main/resources/markdown/zookeeperAdmin.md
##########
@@ -2068,6 +2068,9 @@ options are used to configure the [AdminServer](#sc_adminserver).
Set to "org.apache.zookeeper.metrics.prometheus.PrometheusMetricsProvider" to
enable Prometheus.io exporter.
+* *metricsProvider.httpHost* :
+ Prometheus.io exporter will start a Jetty server and listen this address, default is "0.0.0.0"
Review comment:
Refer to other "New in xxxx", I Added.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] maoling commented on pull request #1574: ZOOKEEPER-4054: Make prometheus listen host can configure
Posted by GitBox <gi...@apache.org>.
maoling commented on pull request #1574:
URL: https://github.com/apache/zookeeper/pull/1574#issuecomment-842405581
@Shoothzj Thanks for your contribution.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] maoling commented on pull request #1574: ZOOKEEPER-4054: Make prometheus listen host can configure
Posted by GitBox <gi...@apache.org>.
maoling commented on pull request #1574:
URL: https://github.com/apache/zookeeper/pull/1574#issuecomment-841766320
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] Shoothzj closed pull request #1574: ZOOKEEPER-4054: Make prometheus listen host can configure
Posted by GitBox <gi...@apache.org>.
Shoothzj closed pull request #1574:
URL: https://github.com/apache/zookeeper/pull/1574
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] Shoothzj commented on pull request #1574: ZOOKEEPER-4054: Make prometheus listen host can configure
Posted by GitBox <gi...@apache.org>.
Shoothzj commented on pull request #1574:
URL: https://github.com/apache/zookeeper/pull/1574#issuecomment-757403786
@maoling How do I deal with this problem? should I retry?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] Shoothzj commented on pull request #1574: ZOOKEEPER-4054: Make prometheus listen host can configure
Posted by GitBox <gi...@apache.org>.
Shoothzj commented on pull request #1574:
URL: https://github.com/apache/zookeeper/pull/1574#issuecomment-841747076
> @Shoothzj
>
> * Ennnnnnnn, I have remembered I have replied your latest comment long long ago. Where is my comment? (Strange thing!!!)
> * After you resolve my latest request changes, I'll merge this PR asap. At the latest, next weekend(05-15)
I saw you closing and opening this PR, what's wrong with this ? I noticed a test failure, but it's not related with this code。 The remaining discussion not resolve is because I'm not sure if I fixed the right space.
Thanks for your patient.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] maoling commented on a change in pull request #1574: ZOOKEEPER-4054: Make prometheus listen host can configure
Posted by GitBox <gi...@apache.org>.
maoling commented on a change in pull request #1574:
URL: https://github.com/apache/zookeeper/pull/1574#discussion_r589392017
##########
File path: zookeeper-metrics-providers/zookeeper-prometheus-metrics/src/test/java/org/apache/zookeeper/metrics/prometheus/PrometheusMetricsProviderTest.java
##########
@@ -56,6 +56,7 @@ public void setup() throws Exception {
CollectorRegistry.defaultRegistry.clear();
provider = new PrometheusMetricsProvider();
Properties configuration = new Properties();
+ configuration.setProperty("httpHost", "127.0.0.1"); // local host for test
configuration.setProperty("httpPort", "0"); // ephemeral port
Review comment:
PortAssignment.unique()?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org