yanghua commented on a change in pull request #1572:
URL: https://github.com/apache/incubator-hudi/pull/1572#discussion_r421979461
##########
File path:
hudi-client/src/main/java/org/apache/hudi/config/HoodieMetricsConfig.java
##########
@@ -48,6 +48,8 @@
public static final String GRAPHITE_SERVER_PORT = GRAPHITE_PREFIX + ".port";
public static final int DEFAULT_GRAPHITE_SERVER_PORT = 4756;
Review comment:
remove this empty line?
##########
File path:
hudi-client/src/main/java/org/apache/hudi/config/HoodieMetricsConfig.java
##########
@@ -92,28 +103,63 @@ public Builder withReporterType(String reporterType) {
return this;
}
- public Builder toGraphiteHost(String host) {
+ public Builder withGraphiteHost(String host) {
Review comment:
I have two opinions about these changes:
- Does it break the compatibility?
- Packaging different config options of different reporters is not a good
choice, it would be better to refactor.
##########
File path:
hudi-client/src/main/java/org/apache/hudi/metrics/datadog/DatadogHttpClient.java
##########
@@ -0,0 +1,128 @@
+/*
+ * 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.hudi.metrics.datadog;
+
+import org.apache.hudi.common.util.StringUtils;
+import org.apache.hudi.common.util.ValidationUtils;
+
+import org.apache.http.HttpHeaders;
+import org.apache.http.HttpStatus;
+import org.apache.http.client.config.RequestConfig;
+import org.apache.http.client.methods.CloseableHttpResponse;
+import org.apache.http.client.methods.HttpGet;
+import org.apache.http.client.methods.HttpPost;
+import org.apache.http.client.methods.HttpUriRequest;
+import org.apache.http.entity.ContentType;
+import org.apache.http.entity.StringEntity;
+import org.apache.http.impl.client.CloseableHttpClient;
+import org.apache.http.impl.client.HttpClientBuilder;
+import org.apache.log4j.LogManager;
+import org.apache.log4j.Logger;
+
+import java.io.Closeable;
+import java.io.IOException;
+
+/**
+ * Datadog API HTTP client.
+ * <p>
+ * Responsible for API endpoint routing, validating API key, and sending
requests with metrics payload.
+ */
+public class DatadogHttpClient implements Closeable {
+
+ private static final Logger LOG =
LogManager.getLogger(DatadogHttpClient.class);
+
+ private static final String SERIES_URL_FORMAT =
"https://app.datadoghq.%s/api/v1/series";;
+ private static final String VALIDATE_URL_FORMAT =
"https://app.datadoghq.%s/api/v1/validate";;
+ private static final String HEADER_KEY_API_KEY = "DD-API-KEY";
+ private static final int TIMEOUT_MILLIS = 3000;
Review comment:
Can we make this field configurable?
##########
File path:
hudi-client/src/main/java/org/apache/hudi/config/HoodieWriteConfig.java
##########
@@ -507,6 +512,37 @@ public String getJmxPort() {
return props.getProperty(HoodieMetricsConfig.JMX_PORT);
}
+ public ApiSite getDatadogApiSite() {
Review comment:
Ditto, IMO, we should also refactor these change in the future.
##########
File path:
hudi-client/src/main/java/org/apache/hudi/metrics/datadog/DatadogMetricsReporter.java
##########
@@ -0,0 +1,90 @@
+/*
+ * 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.hudi.metrics.datadog;
+
+import org.apache.hudi.common.util.Option;
+import org.apache.hudi.common.util.StringUtils;
+import org.apache.hudi.common.util.ValidationUtils;
+import org.apache.hudi.config.HoodieWriteConfig;
+import org.apache.hudi.metrics.MetricsReporter;
+import org.apache.hudi.metrics.datadog.DatadogHttpClient.ApiSite;
+
+import com.codahale.metrics.MetricFilter;
+import com.codahale.metrics.MetricRegistry;
+
+import java.io.Closeable;
+import java.util.List;
+import java.util.concurrent.TimeUnit;
+
+/**
+ * Hudi Datadog metrics reporter.
+ * <p>
+ * Responsible for reading Hoodie metrics configurations and hooking up with
{@link org.apache.hudi.metrics.Metrics}.
+ * <p>
+ * Internally delegate reporting tasks to {@link DatadogReporter}.
+ */
+public class DatadogMetricsReporter extends MetricsReporter {
+
+ private final DatadogReporter reporter;
+
+ public DatadogMetricsReporter(HoodieWriteConfig config, MetricRegistry
registry) {
+ ApiSite apiSite = config.getDatadogApiSite();
+ String apiKey = config.getDatadogApiKey();
+ ValidationUtils.checkState(!StringUtils.isNullOrEmpty(apiKey),
+ "Datadog cannot be initialized: API key is null or empty.");
+ boolean skipValidation = config.getDatadogApiKeySkipValidation();
+ String prefix = config.getDatadogMetricPrefix();
+ ValidationUtils.checkState(!StringUtils.isNullOrEmpty(prefix),
+ "Datadog cannot be initialized: Metric prefix is null or empty.");
+ Option<String> host = Option.ofNullable(config.getDatadogMetricHost());
+ List<String> tagList = config.getDatadogMetricTags();
+ Option<List<String>> tags = tagList.isEmpty() ? Option.empty() :
Option.of(tagList);
+
+ reporter = new DatadogReporter(
+ registry,
+ new DatadogHttpClient(apiSite, apiKey, skipValidation),
+ prefix,
+ host,
+ tags,
+ MetricFilter.ALL,
+ TimeUnit.SECONDS,
+ TimeUnit.SECONDS
+ );
+ }
+
+ @Override
+ public void start() {
+ reporter.start(30, TimeUnit.SECONDS);
Review comment:
Can this hard code be configurable?
##########
File path: pom.xml
##########
@@ -245,6 +245,7 @@
<version>${maven-surefire-plugin.version}</version>
<configuration>
<skip>${skipUTs}</skip>
+ <argLine>-Xms256m -Xmx2g</argLine>
Review comment:
We must add this line in this PR?
##########
File path:
hudi-client/src/test/java/org/apache/hudi/metrics/datadog/TestDatadogHttpClient.java
##########
@@ -0,0 +1,152 @@
+/*
+ * 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.hudi.metrics.datadog;
+
+import org.apache.hudi.metrics.datadog.DatadogHttpClient.ApiSite;
+
+import org.apache.http.StatusLine;
+import org.apache.http.client.methods.CloseableHttpResponse;
+import org.apache.http.impl.client.CloseableHttpClient;
+import org.apache.log4j.AppenderSkeleton;
+import org.apache.log4j.Level;
+import org.apache.log4j.Logger;
+import org.apache.log4j.spi.LoggingEvent;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
+import org.mockito.ArgumentCaptor;
+import org.mockito.Captor;
+import org.mockito.Mock;
+import org.mockito.junit.jupiter.MockitoExtension;
+
+import java.io.IOException;
+import java.util.Arrays;
+import java.util.List;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.junit.jupiter.api.Assertions.fail;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+@ExtendWith(MockitoExtension.class)
+public class TestDatadogHttpClient {
+
+ @Mock
+ AppenderSkeleton appender;
+
+ @Captor
+ ArgumentCaptor<LoggingEvent> logCaptor;
+
+ @Mock
+ CloseableHttpClient httpClient;
+
+ @Mock
+ CloseableHttpResponse httpResponse;
+
+ @Mock
+ StatusLine statusLine;
+
+ private void mockResponse(int statusCode) {
+ when(statusLine.getStatusCode()).thenReturn(statusCode);
+ when(httpResponse.getStatusLine()).thenReturn(statusLine);
+ try {
+ when(httpClient.execute(any())).thenReturn(httpResponse);
+ } catch (IOException e) {
+ fail(e.getMessage(), e);
+ }
+ }
+
+ @Test
+ public void validateApiKey_shouldThrowException_whenRequestFailed() throws
IOException {
Review comment:
It may be my lack of knowledge. Are these patterns like `xxx_xxx_xxx`
related to the parsing mechanism of the latest test framework? Otherwise, why
not use hump nomenclature?
----------------------------------------------------------------
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:
[email protected]
