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: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org