zentol commented on a change in pull request #18668:
URL: https://github.com/apache/flink/pull/18668#discussion_r802481192
##########
File path:
flink-metrics/flink-metrics-prometheus/src/main/java/org/apache/flink/metrics/prometheus/PrometheusPushGatewayReporter.java
##########
@@ -85,4 +99,17 @@ public void close() {
}
super.close();
}
+
+ private static URL tryCreateUrl(String hostUrl) {
+ try {
+ return new URL(hostUrl);
Review comment:
This should be done in the factory
##########
File path:
flink-metrics/flink-metrics-prometheus/src/main/java/org/apache/flink/metrics/prometheus/PrometheusPushGatewayReporterFactory.java
##########
@@ -63,9 +64,18 @@ public PrometheusPushGatewayReporter
createMetricReporter(Properties properties)
parseGroupingKey(
metricConfig.getString(GROUPING_KEY.key(),
GROUPING_KEY.defaultValue()));
- if (host == null || host.isEmpty() || port < 1) {
- throw new IllegalArgumentException(
- "Invalid host/port configuration. Host: " + host + " Port:
" + port);
+ String hostUrlConfig = metricConfig.getString(HOST_URL.key(),
HOST_URL.defaultValue());
+
+ final String hostUrl;
+ if (hostUrlConfig != null && !hostUrlConfig.isEmpty()) {
Review comment:
Use StringUtils#isNullOrWhitespaceOnly
##########
File path:
flink-metrics/flink-metrics-prometheus/src/main/java/org/apache/flink/metrics/prometheus/PrometheusPushGatewayReporter.java
##########
@@ -57,6 +60,17 @@
this.deleteOnShutdown = deleteOnShutdown;
}
+ PrometheusPushGatewayReporter(
+ String hostUrl,
+ String jobName,
Review comment:
Why do we need a second constructor and can't just change the existing
one?
##########
File path:
flink-metrics/flink-metrics-prometheus/src/test/java/org/apache/flink/metrics/prometheus/PrometheusPushGatewayReporterTest.java
##########
@@ -49,4 +55,52 @@ public void testParseIncompleteGroupingKey() {
groupingKey =
PrometheusPushGatewayReporterFactory.parseGroupingKey("k1");
Assert.assertTrue(groupingKey.isEmpty());
}
+
+ @Test
+ public void testConnectToPushGatewayUsingHostAndPort() {
+ PrometheusPushGatewayReporterFactory factory = new
PrometheusPushGatewayReporterFactory();
+ MetricConfig metricConfig = new MetricConfig();
+ metricConfig.setProperty(HOST.key(), "localhost");
+ metricConfig.setProperty(PORT.key(), "18080");
+ PrometheusPushGatewayReporter reporter =
factory.createMetricReporter(metricConfig);
+ String gatewayBaseURL =
+ (String) Whitebox.getInternalState(reporter.getPushGateway(),
"gatewayBaseURL");
+ Assert.assertEquals(gatewayBaseURL, "http://localhost:18080/metrics/";);
+ }
+
+ @Test
+ public void testConnectToPushGatewayUsingHostUrl() {
+ PrometheusPushGatewayReporterFactory factory = new
PrometheusPushGatewayReporterFactory();
+ MetricConfig metricConfig = new MetricConfig();
+ metricConfig.setProperty(HOST_URL.key(), "https://localhost:18080";);
+ PrometheusPushGatewayReporter reporter =
factory.createMetricReporter(metricConfig);
+ String gatewayBaseURL =
+ (String) Whitebox.getInternalState(reporter.getPushGateway(),
"gatewayBaseURL");
+ Assert.assertEquals(gatewayBaseURL,
"https://localhost:18080/metrics/";);
+ }
+
+ @Test
+ public void testConnectToPushGatewayPreferHostUrl() {
+ PrometheusPushGatewayReporterFactory factory = new
PrometheusPushGatewayReporterFactory();
+ MetricConfig metricConfig = new MetricConfig();
+ metricConfig.setProperty(HOST_URL.key(), "https://localhost:18080";);
+ metricConfig.setProperty(HOST.key(), "localhost1");
+ metricConfig.setProperty(PORT.key(), "18081");
+ PrometheusPushGatewayReporter reporter =
factory.createMetricReporter(metricConfig);
+ String gatewayBaseURL =
+ (String) Whitebox.getInternalState(reporter.getPushGateway(),
"gatewayBaseURL");
+ Assert.assertEquals(gatewayBaseURL,
"https://localhost:18080/metrics/";);
+ }
+
+ @Test
+ public void
testConnectToPushGatewayThrowsExceptionWithoutHostInformation() {
+ PrometheusPushGatewayReporterFactory factory = new
PrometheusPushGatewayReporterFactory();
+ MetricConfig metricConfig = new MetricConfig();
+ Assert.assertThrows(
+ IllegalArgumentException.class, () ->
factory.createMetricReporter(metricConfig));
+
+ metricConfig.setProperty(HOST.key(), "localhost");
+ Assert.assertThrows(
+ IllegalArgumentException.class, () ->
factory.createMetricReporter(metricConfig));
Review comment:
missing a check for it failing if only the port is set.
##########
File path:
flink-metrics/flink-metrics-prometheus/src/test/java/org/apache/flink/metrics/prometheus/PrometheusPushGatewayReporterTest.java
##########
@@ -49,4 +55,52 @@ public void testParseIncompleteGroupingKey() {
groupingKey =
PrometheusPushGatewayReporterFactory.parseGroupingKey("k1");
Assert.assertTrue(groupingKey.isEmpty());
}
+
+ @Test
+ public void testConnectToPushGatewayUsingHostAndPort() {
+ PrometheusPushGatewayReporterFactory factory = new
PrometheusPushGatewayReporterFactory();
+ MetricConfig metricConfig = new MetricConfig();
+ metricConfig.setProperty(HOST.key(), "localhost");
+ metricConfig.setProperty(PORT.key(), "18080");
+ PrometheusPushGatewayReporter reporter =
factory.createMetricReporter(metricConfig);
+ String gatewayBaseURL =
+ (String) Whitebox.getInternalState(reporter.getPushGateway(),
"gatewayBaseURL");
+ Assert.assertEquals(gatewayBaseURL, "http://localhost:18080/metrics/";);
+ }
+
+ @Test
+ public void testConnectToPushGatewayUsingHostUrl() {
+ PrometheusPushGatewayReporterFactory factory = new
PrometheusPushGatewayReporterFactory();
+ MetricConfig metricConfig = new MetricConfig();
+ metricConfig.setProperty(HOST_URL.key(), "https://localhost:18080";);
+ PrometheusPushGatewayReporter reporter =
factory.createMetricReporter(metricConfig);
+ String gatewayBaseURL =
+ (String) Whitebox.getInternalState(reporter.getPushGateway(),
"gatewayBaseURL");
+ Assert.assertEquals(gatewayBaseURL,
"https://localhost:18080/metrics/";);
+ }
+
+ @Test
+ public void testConnectToPushGatewayPreferHostUrl() {
+ PrometheusPushGatewayReporterFactory factory = new
PrometheusPushGatewayReporterFactory();
+ MetricConfig metricConfig = new MetricConfig();
+ metricConfig.setProperty(HOST_URL.key(), "https://localhost:18080";);
+ metricConfig.setProperty(HOST.key(), "localhost1");
+ metricConfig.setProperty(PORT.key(), "18081");
+ PrometheusPushGatewayReporter reporter =
factory.createMetricReporter(metricConfig);
+ String gatewayBaseURL =
+ (String) Whitebox.getInternalState(reporter.getPushGateway(),
"gatewayBaseURL");
Review comment:
Please find a ways to test this without using mockito, e.g., by storing
the URL in the reporter and making it accessible, or moving the host/port
handling into a static `@VisibleForTesting` method similar to
`parseGroupingKey`.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
