You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@hudi.apache.org by GitBox <gi...@apache.org> on 2020/04/29 19:55:24 UTC

[GitHub] [incubator-hudi] xushiyan opened a new pull request #1572: [HUDI-836] Implement datadog metric reporter

xushiyan opened a new pull request #1572:
URL: https://github.com/apache/incubator-hudi/pull/1572


   * Add a new metric reporter type for Datadog
   * Implement reporting support for Datadog http api
   
   ## Committer checklist
   
    - [ ] Has a corresponding JIRA in PR title & commit
    
    - [ ] Commit message is descriptive of the change
    
    - [ ] CI is green
   
    - [ ] Necessary doc changes done or have another open PR
          
    - [ ] For large changes, please consider breaking it into sub-tasks under an umbrella JIRA.


----------------------------------------------------------------
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] [incubator-hudi] yanghua commented on a change in pull request #1572: [HUDI-836] Implement datadog metrics reporter

Posted by GitBox <gi...@apache.org>.
yanghua commented on a change in pull request #1572:
URL: https://github.com/apache/incubator-hudi/pull/1572#discussion_r426435865



##########
File path: hudi-client/src/main/java/org/apache/hudi/metrics/datadog/DatadogReporter.java
##########
@@ -0,0 +1,168 @@
+/*
+ * 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.ValidationUtils;
+
+import com.codahale.metrics.Clock;
+import com.codahale.metrics.Counter;
+import com.codahale.metrics.Gauge;
+import com.codahale.metrics.Histogram;
+import com.codahale.metrics.Meter;
+import com.codahale.metrics.MetricFilter;
+import com.codahale.metrics.MetricRegistry;
+import com.codahale.metrics.ScheduledReporter;
+import com.codahale.metrics.Timer;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.fasterxml.jackson.databind.node.ArrayNode;
+import com.fasterxml.jackson.databind.node.ObjectNode;
+import com.fasterxml.jackson.databind.node.TextNode;
+import org.apache.log4j.LogManager;
+import org.apache.log4j.Logger;
+
+import java.io.IOException;
+import java.util.List;
+import java.util.Objects;
+import java.util.SortedMap;
+import java.util.concurrent.TimeUnit;
+import java.util.stream.Collectors;
+
+/**
+ * A reporter which publishes metric values to Datadog API.
+ * <p>
+ * Responsible for collecting and composing metrics payload.
+ * <p>
+ * Internally use {@link DatadogHttpClient} to interact with Datadog APIs.
+ */
+public class DatadogReporter extends ScheduledReporter {
+
+  private static final Logger LOG = LogManager.getLogger(DatadogReporter.class);
+
+  private final DatadogHttpClient client;
+  private final String prefix;
+  private final Option<String> host;
+  private final Option<List<String>> tags;
+  private final Clock clock;
+
+  protected DatadogReporter(
+      MetricRegistry registry,
+      DatadogHttpClient client,
+      String prefix,
+      Option<String> host,
+      Option<List<String>> tags,
+      MetricFilter filter,
+      TimeUnit rateUnit,
+      TimeUnit durationUnit) {
+    super(registry, "hudi-datadog-reporter", filter, rateUnit, durationUnit);
+    this.client = client;
+    this.prefix = prefix;
+    this.host = host;
+    this.tags = tags;
+    this.clock = Clock.defaultClock();
+  }
+
+  @Override
+  public void report(
+      SortedMap<String, Gauge> gauges,
+      SortedMap<String, Counter> counters,
+      SortedMap<String, Histogram> histograms,
+      SortedMap<String, Meter> meters,
+      SortedMap<String, Timer> timers) {
+    final long now = clock.getTime() / 1000;
+    final PayloadBuilder builder = new PayloadBuilder();
+
+    builder.withType("gauge");
+    gauges.forEach((metricName, metric) -> {
+      builder.addGauge(prefix(metricName), now, (long) metric.getValue());
+    });
+
+    host.ifPresent(builder::withHost);
+    tags.ifPresent(builder::withTags);
+
+    client.send(builder.build());
+  }
+
+  protected String prefix(String... components) {
+    return MetricRegistry.name(prefix, components);
+  }
+
+  @Override
+  public void stop() {
+    try {
+      super.stop();
+    } finally {
+      try {
+        client.close();
+      } catch (IOException e) {
+        LOG.warn("Error disconnecting from Datadog.", e);
+      }
+    }
+  }
+
+  /**
+   * Build payload that contains metrics data.
+   * <p>
+   * Refer to Datadog API reference https://docs.datadoghq.com/api/?lang=bash#post-timeseries-points
+   */
+  static class PayloadBuilder {
+
+    private static final ObjectMapper MAPPER = new ObjectMapper();
+
+    private final ObjectNode payload;
+    private final ArrayNode series;
+    private String type;
+
+    PayloadBuilder() {
+      payload = MAPPER.createObjectNode();
+      series = payload.putArray("series");
+    }
+
+    PayloadBuilder withType(String type) {
+      this.type = type;
+      return this;
+    }
+
+    PayloadBuilder addGauge(String metric, long timestamp, long gaugeValue) {
+      ValidationUtils.checkState(Objects.equals(type, "gauge"));

Review comment:
       It would be better to extract these hard codes to be constants




----------------------------------------------------------------
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] [incubator-hudi] xushiyan commented on pull request #1572: [HUDI-836] Implement datadog metrics reporter

Posted by GitBox <gi...@apache.org>.
xushiyan commented on pull request #1572:
URL: https://github.com/apache/incubator-hudi/pull/1572#issuecomment-624831618


   @yanghua @vinothchandar The travis tests is flaky; i can pass the tests locally. Please go ahead review the PR; I'll do some checkings on the CI failure later. Thank you.


----------------------------------------------------------------
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] [incubator-hudi] xushiyan commented on pull request #1572: [HUDI-836] Implement datadog metrics reporter

Posted by GitBox <gi...@apache.org>.
xushiyan commented on pull request #1572:
URL: https://github.com/apache/incubator-hudi/pull/1572#issuecomment-629702325


   @yanghua CI passed. Also updated #1603 for the newly added config. 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] [incubator-hudi] codecov-io edited a comment on pull request #1572: [HUDI-836] Implement datadog metric reporter

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #1572:
URL: https://github.com/apache/incubator-hudi/pull/1572#issuecomment-621450664


   # [Codecov](https://codecov.io/gh/apache/incubator-hudi/pull/1572?src=pr&el=h1) Report
   > Merging [#1572](https://codecov.io/gh/apache/incubator-hudi/pull/1572?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-hudi/commit/9059bce977cee98e2d65769622c46a1941c563dd&el=desc) will **decrease** coverage by `0.58%`.
   > The diff coverage is `2.81%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-hudi/pull/1572/graphs/tree.svg?width=650&height=150&src=pr&token=VTTXabwbs2)](https://codecov.io/gh/apache/incubator-hudi/pull/1572?src=pr&el=tree)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master    #1572      +/-   ##
   ============================================
   - Coverage     71.81%   71.23%   -0.59%     
     Complexity      294      294              
   ============================================
     Files           385      388       +3     
     Lines         16540    16679     +139     
     Branches       1661     1669       +8     
   ============================================
   + Hits          11878    11881       +3     
   - Misses         3932     4068     +136     
     Partials        730      730              
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-hudi/pull/1572?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | [...java/org/apache/hudi/config/HoodieWriteConfig.java](https://codecov.io/gh/apache/incubator-hudi/pull/1572/diff?src=pr&el=tree#diff-aHVkaS1jbGllbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvY29uZmlnL0hvb2RpZVdyaXRlQ29uZmlnLmphdmE=) | `82.00% <0.00%> (-2.85%)` | `0.00 <0.00> (ø)` | |
   | [...rg/apache/hudi/metrics/MetricsReporterFactory.java](https://codecov.io/gh/apache/incubator-hudi/pull/1572/diff?src=pr&el=tree#diff-aHVkaS1jbGllbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvbWV0cmljcy9NZXRyaWNzUmVwb3J0ZXJGYWN0b3J5LmphdmE=) | `40.00% <0.00%> (-6.16%)` | `0.00 <0.00> (ø)` | |
   | [...apache/hudi/metrics/datadog/DatadogHttpClient.java](https://codecov.io/gh/apache/incubator-hudi/pull/1572/diff?src=pr&el=tree#diff-aHVkaS1jbGllbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvbWV0cmljcy9kYXRhZG9nL0RhdGFkb2dIdHRwQ2xpZW50LmphdmE=) | `0.00% <0.00%> (ø)` | `0.00 <0.00> (?)` | |
   | [...e/hudi/metrics/datadog/DatadogMetricsReporter.java](https://codecov.io/gh/apache/incubator-hudi/pull/1572/diff?src=pr&el=tree#diff-aHVkaS1jbGllbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvbWV0cmljcy9kYXRhZG9nL0RhdGFkb2dNZXRyaWNzUmVwb3J0ZXIuamF2YQ==) | `0.00% <0.00%> (ø)` | `0.00 <0.00> (?)` | |
   | [...g/apache/hudi/metrics/datadog/DatadogReporter.java](https://codecov.io/gh/apache/incubator-hudi/pull/1572/diff?src=pr&el=tree#diff-aHVkaS1jbGllbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvbWV0cmljcy9kYXRhZG9nL0RhdGFkb2dSZXBvcnRlci5qYXZh) | `0.00% <0.00%> (ø)` | `0.00 <0.00> (?)` | |
   | [...va/org/apache/hudi/config/HoodieMetricsConfig.java](https://codecov.io/gh/apache/incubator-hudi/pull/1572/diff?src=pr&el=tree#diff-aHVkaS1jbGllbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvY29uZmlnL0hvb2RpZU1ldHJpY3NDb25maWcuamF2YQ==) | `40.38% <11.76%> (-12.40%)` | `0.00 <0.00> (ø)` | |
   | [...src/main/java/org/apache/hudi/metrics/Metrics.java](https://codecov.io/gh/apache/incubator-hudi/pull/1572/diff?src=pr&el=tree#diff-aHVkaS1jbGllbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvbWV0cmljcy9NZXRyaWNzLmphdmE=) | `56.75% <100.00%> (ø)` | `0.00 <0.00> (ø)` | |
   | [...a/org/apache/hudi/metrics/MetricsReporterType.java](https://codecov.io/gh/apache/incubator-hudi/pull/1572/diff?src=pr&el=tree#diff-aHVkaS1jbGllbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvbWV0cmljcy9NZXRyaWNzUmVwb3J0ZXJUeXBlLmphdmE=) | `100.00% <100.00%> (ø)` | `0.00 <0.00> (ø)` | |
   | [...che/hudi/common/util/BufferedRandomAccessFile.java](https://codecov.io/gh/apache/incubator-hudi/pull/1572/diff?src=pr&el=tree#diff-aHVkaS1jb21tb24vc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvY29tbW9uL3V0aWwvQnVmZmVyZWRSYW5kb21BY2Nlc3NGaWxlLmphdmE=) | `54.38% <0.00%> (-0.88%)` | `0.00% <0.00%> (ø%)` | |
   | ... and [3 more](https://codecov.io/gh/apache/incubator-hudi/pull/1572/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-hudi/pull/1572?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-hudi/pull/1572?src=pr&el=footer). Last update [9059bce...04e47c4](https://codecov.io/gh/apache/incubator-hudi/pull/1572?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
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] [incubator-hudi] vinothchandar commented on pull request #1572: [HUDI-836] Implement datadog metrics reporter

Posted by GitBox <gi...@apache.org>.
vinothchandar commented on pull request #1572:
URL: https://github.com/apache/incubator-hudi/pull/1572#issuecomment-632774712


   @xushiyan may be we need to doc this or write a small post? 
   


----------------------------------------------------------------
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] [incubator-hudi] vinothchandar merged pull request #1572: [HUDI-836] Implement datadog metrics reporter

Posted by GitBox <gi...@apache.org>.
vinothchandar merged pull request #1572:
URL: https://github.com/apache/incubator-hudi/pull/1572


   


----------------------------------------------------------------
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] [incubator-hudi] vinothchandar commented on pull request #1572: [HUDI-836] Implement datadog metrics reporter

Posted by GitBox <gi...@apache.org>.
vinothchandar commented on pull request #1572:
URL: https://github.com/apache/incubator-hudi/pull/1572#issuecomment-625049202


   @xushiyan I think the metrics tests are hard coding the ports and running into issues? I ran into one such issue locally.. PR brewing.. 


----------------------------------------------------------------
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] [incubator-hudi] codecov-io commented on pull request #1572: [HUDI-836] Implement datadog metric reporter

Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #1572:
URL: https://github.com/apache/incubator-hudi/pull/1572#issuecomment-621450664


   # [Codecov](https://codecov.io/gh/apache/incubator-hudi/pull/1572?src=pr&el=h1) Report
   > Merging [#1572](https://codecov.io/gh/apache/incubator-hudi/pull/1572?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-hudi/commit/9059bce977cee98e2d65769622c46a1941c563dd&el=desc) will **decrease** coverage by `0.56%`.
   > The diff coverage is `2.73%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-hudi/pull/1572/graphs/tree.svg?width=650&height=150&src=pr&token=VTTXabwbs2)](https://codecov.io/gh/apache/incubator-hudi/pull/1572?src=pr&el=tree)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master    #1572      +/-   ##
   ============================================
   - Coverage     71.81%   71.24%   -0.57%     
     Complexity      294      294              
   ============================================
     Files           385      388       +3     
     Lines         16540    16683     +143     
     Branches       1661     1668       +7     
   ============================================
   + Hits          11878    11886       +8     
   - Misses         3932     4066     +134     
   - Partials        730      731       +1     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-hudi/pull/1572?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | [...java/org/apache/hudi/config/HoodieWriteConfig.java](https://codecov.io/gh/apache/incubator-hudi/pull/1572/diff?src=pr&el=tree#diff-aHVkaS1jbGllbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvY29uZmlnL0hvb2RpZVdyaXRlQ29uZmlnLmphdmE=) | `81.32% <0.00%> (-3.53%)` | `0.00 <0.00> (ø)` | |
   | [...rg/apache/hudi/metrics/MetricsReporterFactory.java](https://codecov.io/gh/apache/incubator-hudi/pull/1572/diff?src=pr&el=tree#diff-aHVkaS1jbGllbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvbWV0cmljcy9NZXRyaWNzUmVwb3J0ZXJGYWN0b3J5LmphdmE=) | `40.00% <0.00%> (-6.16%)` | `0.00 <0.00> (ø)` | |
   | [...apache/hudi/metrics/datadog/DatadogHttpClient.java](https://codecov.io/gh/apache/incubator-hudi/pull/1572/diff?src=pr&el=tree#diff-aHVkaS1jbGllbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvbWV0cmljcy9kYXRhZG9nL0RhdGFkb2dIdHRwQ2xpZW50LmphdmE=) | `0.00% <0.00%> (ø)` | `0.00 <0.00> (?)` | |
   | [...e/hudi/metrics/datadog/DatadogMetricsReporter.java](https://codecov.io/gh/apache/incubator-hudi/pull/1572/diff?src=pr&el=tree#diff-aHVkaS1jbGllbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvbWV0cmljcy9kYXRhZG9nL0RhdGFkb2dNZXRyaWNzUmVwb3J0ZXIuamF2YQ==) | `0.00% <0.00%> (ø)` | `0.00 <0.00> (?)` | |
   | [...g/apache/hudi/metrics/datadog/DatadogReporter.java](https://codecov.io/gh/apache/incubator-hudi/pull/1572/diff?src=pr&el=tree#diff-aHVkaS1jbGllbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvbWV0cmljcy9kYXRhZG9nL0RhdGFkb2dSZXBvcnRlci5qYXZh) | `0.00% <0.00%> (ø)` | `0.00 <0.00> (?)` | |
   | [...va/org/apache/hudi/config/HoodieMetricsConfig.java](https://codecov.io/gh/apache/incubator-hudi/pull/1572/diff?src=pr&el=tree#diff-aHVkaS1jbGllbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvY29uZmlnL0hvb2RpZU1ldHJpY3NDb25maWcuamF2YQ==) | `40.38% <11.76%> (-12.40%)` | `0.00 <0.00> (ø)` | |
   | [...src/main/java/org/apache/hudi/metrics/Metrics.java](https://codecov.io/gh/apache/incubator-hudi/pull/1572/diff?src=pr&el=tree#diff-aHVkaS1jbGllbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvbWV0cmljcy9NZXRyaWNzLmphdmE=) | `67.56% <100.00%> (+10.81%)` | `0.00 <0.00> (ø)` | |
   | [...a/org/apache/hudi/metrics/MetricsReporterType.java](https://codecov.io/gh/apache/incubator-hudi/pull/1572/diff?src=pr&el=tree#diff-aHVkaS1jbGllbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvbWV0cmljcy9NZXRyaWNzUmVwb3J0ZXJUeXBlLmphdmE=) | `100.00% <100.00%> (ø)` | `0.00 <0.00> (ø)` | |
   | ... and [3 more](https://codecov.io/gh/apache/incubator-hudi/pull/1572/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-hudi/pull/1572?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-hudi/pull/1572?src=pr&el=footer). Last update [9059bce...8cb8444](https://codecov.io/gh/apache/incubator-hudi/pull/1572?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
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] [incubator-hudi] vinothchandar commented on pull request #1572: [HUDI-836] Implement datadog metrics reporter

Posted by GitBox <gi...@apache.org>.
vinothchandar commented on pull request #1572:
URL: https://github.com/apache/incubator-hudi/pull/1572#issuecomment-624504748


   @xushiyan absolutely... please go ahead! 


----------------------------------------------------------------
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] [incubator-hudi] yanghua commented on pull request #1572: [HUDI-836] Implement datadog metrics reporter

Posted by GitBox <gi...@apache.org>.
yanghua commented on pull request #1572:
URL: https://github.com/apache/incubator-hudi/pull/1572#issuecomment-629960658


   Will do a final check.


----------------------------------------------------------------
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] [incubator-hudi] xushiyan commented on a change in pull request #1572: [HUDI-836] Implement datadog metrics reporter

Posted by GitBox <gi...@apache.org>.
xushiyan commented on a change in pull request #1572:
URL: https://github.com/apache/incubator-hudi/pull/1572#discussion_r422526655



##########
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:
       These builder methods are not used by any logic. I think they are legacy code and only mentioned in config docs. I'm changing the names in #1603 too. 
   
   You're right that we should segregate the configs for different reporter types. It shouldn't give too much hassle to do refactoring.




----------------------------------------------------------------
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] [incubator-hudi] xushiyan commented on a change in pull request #1572: [HUDI-836] Implement datadog metrics reporter

Posted by GitBox <gi...@apache.org>.
xushiyan commented on a change in pull request #1572:
URL: https://github.com/apache/incubator-hudi/pull/1572#discussion_r422528107



##########
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 is just one unit test naming convention I tried to adopt...it basically represents [theMethodUnderTest_expectedBehavior_condition] for being descriptive of the logic. See [this](https://stackoverflow.com/questions/155436/unit-test-naming-best-practices) for reference.
   
   The `_` is to break the long sentence for readability when using hump nomenclature. I don't mind changing it to hump nomenclature though.




----------------------------------------------------------------
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] [incubator-hudi] xushiyan commented on pull request #1572: [HUDI-836] Implement datadog metrics reporter

Posted by GitBox <gi...@apache.org>.
xushiyan commented on pull request #1572:
URL: https://github.com/apache/incubator-hudi/pull/1572#issuecomment-632789214


   > @xushiyan may be we need to doc this or write a small post?
   
   Yes @vinothchandar will do the post soon. Config docs update is in #1603 . As for http-client, I have been using this implementation in production, looking good so far. Will definitely test it with more scenarios.


----------------------------------------------------------------
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] [incubator-hudi] xushiyan commented on a change in pull request #1572: [HUDI-836] Implement datadog metrics reporter

Posted by GitBox <gi...@apache.org>.
xushiyan commented on a change in pull request #1572:
URL: https://github.com/apache/incubator-hudi/pull/1572#discussion_r422526952



##########
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:
       I tried to gauge on a good number of configs for user to control over.. but i guess it won't harm if we go with all configurable with sane defaults. So yes i can make it configurable.




----------------------------------------------------------------
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] [incubator-hudi] xushiyan commented on pull request #1572: [HUDI-836] Implement datadog metrics reporter

Posted by GitBox <gi...@apache.org>.
xushiyan commented on pull request #1572:
URL: https://github.com/apache/incubator-hudi/pull/1572#issuecomment-625360473


   @vinothchandar @yanghua CI is passing now... I don't seem to have tests failing; it seemed just sometimes travis got freaked out... 


----------------------------------------------------------------
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] [incubator-hudi] yanghua commented on pull request #1572: [HUDI-836] Implement datadog metrics reporter

Posted by GitBox <gi...@apache.org>.
yanghua commented on pull request #1572:
URL: https://github.com/apache/incubator-hudi/pull/1572#issuecomment-625670562


   > @yanghua Once this is approved, I'll then update this docs update PR #1603 with Chinese version. Thanks.
   
   OK, will review soon.


----------------------------------------------------------------
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] [incubator-hudi] xushiyan commented on pull request #1572: [HUDI-836] Implement datadog metrics reporter

Posted by GitBox <gi...@apache.org>.
xushiyan commented on pull request #1572:
URL: https://github.com/apache/incubator-hudi/pull/1572#issuecomment-625661496


   @yanghua Once this is approved, I'll then update this docs update PR https://github.com/apache/incubator-hudi/pull/1603 with Chinese version. 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] [incubator-hudi] xushiyan commented on a change in pull request #1572: [HUDI-836] Implement datadog metrics reporter

Posted by GitBox <gi...@apache.org>.
xushiyan commented on a change in pull request #1572:
URL: https://github.com/apache/incubator-hudi/pull/1572#discussion_r422769043



##########
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:
       Yup changed to hump namings.




----------------------------------------------------------------
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] [incubator-hudi] xushiyan commented on pull request #1572: [HUDI-836] Implement datadog metrics reporter

Posted by GitBox <gi...@apache.org>.
xushiyan commented on pull request #1572:
URL: https://github.com/apache/incubator-hudi/pull/1572#issuecomment-624415349


   @yanghua Right. I'll make another PR to update the configs docs, too.


----------------------------------------------------------------
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] [incubator-hudi] yanghua commented on a change in pull request #1572: [HUDI-836] Implement datadog metrics reporter

Posted by GitBox <gi...@apache.org>.
yanghua commented on a change in pull request #1572:
URL: https://github.com/apache/incubator-hudi/pull/1572#discussion_r421982141



##########
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 in one class is not a good choice, it would be better to refactor. 




----------------------------------------------------------------
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] [incubator-hudi] xushiyan commented on pull request #1572: [HUDI-836] Implement datadog metrics reporter

Posted by GitBox <gi...@apache.org>.
xushiyan commented on pull request #1572:
URL: https://github.com/apache/incubator-hudi/pull/1572#issuecomment-628304476


   Note: blocked by #1623 


----------------------------------------------------------------
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] [incubator-hudi] yanghua commented on a change in pull request #1572: [HUDI-836] Implement datadog metrics reporter

Posted by GitBox <gi...@apache.org>.
yanghua commented on a change in pull request #1572:
URL: https://github.com/apache/incubator-hudi/pull/1572#discussion_r422750808



##########
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:
       I guess it may be a practice. I do not against this style, it looks good. But for now, it would be better to keep a unified style right?
   
   Of cause, you can start a DISCUSS  on dev ML.




----------------------------------------------------------------
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] [incubator-hudi] xushiyan commented on pull request #1572: [HUDI-836] Implement datadog metrics reporter

Posted by GitBox <gi...@apache.org>.
xushiyan commented on pull request #1572:
URL: https://github.com/apache/incubator-hudi/pull/1572#issuecomment-624372155


   @vinothchandar I'm thinking of writing another short blog to introduce Datadog support with the new configs. Sounds good?


----------------------------------------------------------------
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] [incubator-hudi] yanghua commented on pull request #1572: [HUDI-836] Implement datadog metrics reporter

Posted by GitBox <gi...@apache.org>.
yanghua commented on pull request #1572:
URL: https://github.com/apache/incubator-hudi/pull/1572#issuecomment-631840124


   @vinothchandar If you are busy with other things. Can we merge it firstly? Then, iterate later.


----------------------------------------------------------------
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] [incubator-hudi] yanghua commented on pull request #1572: [HUDI-836] Implement datadog metrics reporter

Posted by GitBox <gi...@apache.org>.
yanghua commented on pull request #1572:
URL: https://github.com/apache/incubator-hudi/pull/1572#issuecomment-624415097


   > @vinothchandar I'm thinking of writing another short blog to introduce Datadog support with the new configs. Sounds good?
   
   @xushiyan Absolutely good idea, IMO, not only blog but also documentation of config option.


----------------------------------------------------------------
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] [incubator-hudi] xushiyan commented on a change in pull request #1572: [HUDI-836] Implement datadog metrics reporter

Posted by GitBox <gi...@apache.org>.
xushiyan commented on a change in pull request #1572:
URL: https://github.com/apache/incubator-hudi/pull/1572#discussion_r426634201



##########
File path: hudi-client/src/main/java/org/apache/hudi/metrics/datadog/DatadogReporter.java
##########
@@ -0,0 +1,168 @@
+/*
+ * 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.ValidationUtils;
+
+import com.codahale.metrics.Clock;
+import com.codahale.metrics.Counter;
+import com.codahale.metrics.Gauge;
+import com.codahale.metrics.Histogram;
+import com.codahale.metrics.Meter;
+import com.codahale.metrics.MetricFilter;
+import com.codahale.metrics.MetricRegistry;
+import com.codahale.metrics.ScheduledReporter;
+import com.codahale.metrics.Timer;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.fasterxml.jackson.databind.node.ArrayNode;
+import com.fasterxml.jackson.databind.node.ObjectNode;
+import com.fasterxml.jackson.databind.node.TextNode;
+import org.apache.log4j.LogManager;
+import org.apache.log4j.Logger;
+
+import java.io.IOException;
+import java.util.List;
+import java.util.Objects;
+import java.util.SortedMap;
+import java.util.concurrent.TimeUnit;
+import java.util.stream.Collectors;
+
+/**
+ * A reporter which publishes metric values to Datadog API.
+ * <p>
+ * Responsible for collecting and composing metrics payload.
+ * <p>
+ * Internally use {@link DatadogHttpClient} to interact with Datadog APIs.
+ */
+public class DatadogReporter extends ScheduledReporter {
+
+  private static final Logger LOG = LogManager.getLogger(DatadogReporter.class);
+
+  private final DatadogHttpClient client;
+  private final String prefix;
+  private final Option<String> host;
+  private final Option<List<String>> tags;
+  private final Clock clock;
+
+  protected DatadogReporter(
+      MetricRegistry registry,
+      DatadogHttpClient client,
+      String prefix,
+      Option<String> host,
+      Option<List<String>> tags,
+      MetricFilter filter,
+      TimeUnit rateUnit,
+      TimeUnit durationUnit) {
+    super(registry, "hudi-datadog-reporter", filter, rateUnit, durationUnit);
+    this.client = client;
+    this.prefix = prefix;
+    this.host = host;
+    this.tags = tags;
+    this.clock = Clock.defaultClock();
+  }
+
+  @Override
+  public void report(
+      SortedMap<String, Gauge> gauges,
+      SortedMap<String, Counter> counters,
+      SortedMap<String, Histogram> histograms,
+      SortedMap<String, Meter> meters,
+      SortedMap<String, Timer> timers) {
+    final long now = clock.getTime() / 1000;
+    final PayloadBuilder builder = new PayloadBuilder();
+
+    builder.withType("gauge");
+    gauges.forEach((metricName, metric) -> {
+      builder.addGauge(prefix(metricName), now, (long) metric.getValue());
+    });
+
+    host.ifPresent(builder::withHost);
+    tags.ifPresent(builder::withTags);
+
+    client.send(builder.build());
+  }
+
+  protected String prefix(String... components) {
+    return MetricRegistry.name(prefix, components);
+  }
+
+  @Override
+  public void stop() {
+    try {
+      super.stop();
+    } finally {
+      try {
+        client.close();
+      } catch (IOException e) {
+        LOG.warn("Error disconnecting from Datadog.", e);
+      }
+    }
+  }
+
+  /**
+   * Build payload that contains metrics data.
+   * <p>
+   * Refer to Datadog API reference https://docs.datadoghq.com/api/?lang=bash#post-timeseries-points
+   */
+  static class PayloadBuilder {
+
+    private static final ObjectMapper MAPPER = new ObjectMapper();
+
+    private final ObjectNode payload;
+    private final ArrayNode series;
+    private String type;
+
+    PayloadBuilder() {
+      payload = MAPPER.createObjectNode();
+      series = payload.putArray("series");
+    }
+
+    PayloadBuilder withType(String type) {
+      this.type = type;
+      return this;
+    }
+
+    PayloadBuilder addGauge(String metric, long timestamp, long gaugeValue) {
+      ValidationUtils.checkState(Objects.equals(type, "gauge"));

Review comment:
       @yanghua 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] [incubator-hudi] xushiyan commented on pull request #1572: [HUDI-836] Implement datadog metrics reporter

Posted by GitBox <gi...@apache.org>.
xushiyan commented on pull request #1572:
URL: https://github.com/apache/incubator-hudi/pull/1572#issuecomment-624370686


   @yanghua This is ready for review. Also manually tested the implementation and was able to collect metrics in Datadog.
   
   ![Screen Shot 2020-05-05 at 5 03 14 PM](https://user-images.githubusercontent.com/2701446/81127259-8a323f00-8ef2-11ea-9e92-b5350e2d64aa.png)
   


----------------------------------------------------------------
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] [incubator-hudi] yanghua commented on a change in pull request #1572: [HUDI-836] Implement datadog metrics reporter

Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org

[GitHub] [incubator-hudi] xushiyan commented on pull request #1572: [HUDI-836] Implement datadog metrics reporter

Posted by GitBox <gi...@apache.org>.
xushiyan commented on pull request #1572:
URL: https://github.com/apache/incubator-hudi/pull/1572#issuecomment-624435108


   Just observed weird CI failure...first time seeing OOM there. Trying with some settings in
   https://github.com/apache/incubator-hudi/pull/1595


----------------------------------------------------------------
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] [incubator-hudi] codecov-io edited a comment on pull request #1572: [HUDI-836] Implement datadog metrics reporter

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #1572:
URL: https://github.com/apache/incubator-hudi/pull/1572#issuecomment-621450664


   # [Codecov](https://codecov.io/gh/apache/incubator-hudi/pull/1572?src=pr&el=h1) Report
   > Merging [#1572](https://codecov.io/gh/apache/incubator-hudi/pull/1572?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-hudi/commit/25a0080b2f6ddce0e528b2a72aea33a565f0e565&el=desc) will **decrease** coverage by `0.10%`.
   > The diff coverage is `8.98%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-hudi/pull/1572/graphs/tree.svg?width=650&height=150&src=pr&token=VTTXabwbs2)](https://codecov.io/gh/apache/incubator-hudi/pull/1572?src=pr&el=tree)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master    #1572      +/-   ##
   ============================================
   - Coverage     16.71%   16.61%   -0.11%     
   - Complexity      795      797       +2     
   ============================================
     Files           340      344       +4     
     Lines         15030    15195     +165     
     Branches       1499     1509      +10     
   ============================================
   + Hits           2512     2524      +12     
   - Misses        12188    12336     +148     
   - Partials        330      335       +5     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-hudi/pull/1572?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | [...java/org/apache/hudi/config/HoodieWriteConfig.java](https://codecov.io/gh/apache/incubator-hudi/pull/1572/diff?src=pr&el=tree#diff-aHVkaS1jbGllbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvY29uZmlnL0hvb2RpZVdyaXRlQ29uZmlnLmphdmE=) | `40.98% <0.00%> (-2.31%)` | `47.00 <0.00> (ø)` | |
   | [...src/main/java/org/apache/hudi/metrics/Metrics.java](https://codecov.io/gh/apache/incubator-hudi/pull/1572/diff?src=pr&el=tree#diff-aHVkaS1jbGllbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvbWV0cmljcy9NZXRyaWNzLmphdmE=) | `0.00% <0.00%> (ø)` | `0.00 <0.00> (ø)` | |
   | [...rg/apache/hudi/metrics/MetricsReporterFactory.java](https://codecov.io/gh/apache/incubator-hudi/pull/1572/diff?src=pr&el=tree#diff-aHVkaS1jbGllbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvbWV0cmljcy9NZXRyaWNzUmVwb3J0ZXJGYWN0b3J5LmphdmE=) | `0.00% <0.00%> (ø)` | `0.00 <0.00> (ø)` | |
   | [...apache/hudi/metrics/datadog/DatadogHttpClient.java](https://codecov.io/gh/apache/incubator-hudi/pull/1572/diff?src=pr&el=tree#diff-aHVkaS1jbGllbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvbWV0cmljcy9kYXRhZG9nL0RhdGFkb2dIdHRwQ2xpZW50LmphdmE=) | `0.00% <0.00%> (ø)` | `0.00 <0.00> (?)` | |
   | [...e/hudi/metrics/datadog/DatadogMetricsReporter.java](https://codecov.io/gh/apache/incubator-hudi/pull/1572/diff?src=pr&el=tree#diff-aHVkaS1jbGllbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvbWV0cmljcy9kYXRhZG9nL0RhdGFkb2dNZXRyaWNzUmVwb3J0ZXIuamF2YQ==) | `0.00% <0.00%> (ø)` | `0.00 <0.00> (?)` | |
   | [...g/apache/hudi/metrics/datadog/DatadogReporter.java](https://codecov.io/gh/apache/incubator-hudi/pull/1572/diff?src=pr&el=tree#diff-aHVkaS1jbGllbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvbWV0cmljcy9kYXRhZG9nL0RhdGFkb2dSZXBvcnRlci5qYXZh) | `0.00% <0.00%> (ø)` | `0.00 <0.00> (?)` | |
   | [...apache/hudi/config/HoodieMetricsDatadogConfig.java](https://codecov.io/gh/apache/incubator-hudi/pull/1572/diff?src=pr&el=tree#diff-aHVkaS1jbGllbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvY29uZmlnL0hvb2RpZU1ldHJpY3NEYXRhZG9nQ29uZmlnLmphdmE=) | `36.36% <36.36%> (ø)` | `2.00 <2.00> (?)` | |
   | [...va/org/apache/hudi/config/HoodieMetricsConfig.java](https://codecov.io/gh/apache/incubator-hudi/pull/1572/diff?src=pr&el=tree#diff-aHVkaS1jbGllbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvY29uZmlnL0hvb2RpZU1ldHJpY3NDb25maWcuamF2YQ==) | `38.46% <66.66%> (+2.35%)` | `3.00 <0.00> (ø)` | |
   | [...a/org/apache/hudi/metrics/MetricsReporterType.java](https://codecov.io/gh/apache/incubator-hudi/pull/1572/diff?src=pr&el=tree#diff-aHVkaS1jbGllbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvbWV0cmljcy9NZXRyaWNzUmVwb3J0ZXJUeXBlLmphdmE=) | `100.00% <100.00%> (ø)` | `1.00 <0.00> (ø)` | |
   | [...che/hudi/common/table/timeline/TimelineLayout.java](https://codecov.io/gh/apache/incubator-hudi/pull/1572/diff?src=pr&el=tree#diff-aHVkaS1jb21tb24vc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvY29tbW9uL3RhYmxlL3RpbWVsaW5lL1RpbWVsaW5lTGF5b3V0LmphdmE=) | `78.57% <0.00%> (-14.29%)` | `3.00% <0.00%> (ø%)` | |
   | ... and [4 more](https://codecov.io/gh/apache/incubator-hudi/pull/1572/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-hudi/pull/1572?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-hudi/pull/1572?src=pr&el=footer). Last update [25a0080...2b4589f](https://codecov.io/gh/apache/incubator-hudi/pull/1572?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
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] [incubator-hudi] xushiyan commented on pull request #1572: [HUDI-836] Implement datadog metrics reporter

Posted by GitBox <gi...@apache.org>.
xushiyan commented on pull request #1572:
URL: https://github.com/apache/incubator-hudi/pull/1572#issuecomment-626460368


   @yanghua In the last commit, I added a new config class `HoodieMetricsDatadogConfig` just for datadog related configs and reverted previous change to `HoodieMetricsConfig`. I think it'd be better to refactor the configs of 2 other reporter types in a separate PR to minimize risks. If the new config class looks good, I'll do a separate PR for the other two, probably with some test cases, too.


----------------------------------------------------------------
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] [incubator-hudi] codecov-io edited a comment on pull request #1572: [HUDI-836] Implement datadog metric reporter

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #1572:
URL: https://github.com/apache/incubator-hudi/pull/1572#issuecomment-621450664


   # [Codecov](https://codecov.io/gh/apache/incubator-hudi/pull/1572?src=pr&el=h1) Report
   > Merging [#1572](https://codecov.io/gh/apache/incubator-hudi/pull/1572?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-hudi/commit/9059bce977cee98e2d65769622c46a1941c563dd&el=desc) will **decrease** coverage by `0.58%`.
   > The diff coverage is `2.81%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-hudi/pull/1572/graphs/tree.svg?width=650&height=150&src=pr&token=VTTXabwbs2)](https://codecov.io/gh/apache/incubator-hudi/pull/1572?src=pr&el=tree)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master    #1572      +/-   ##
   ============================================
   - Coverage     71.81%   71.23%   -0.59%     
     Complexity      294      294              
   ============================================
     Files           385      388       +3     
     Lines         16540    16679     +139     
     Branches       1661     1669       +8     
   ============================================
   + Hits          11878    11881       +3     
   - Misses         3932     4068     +136     
     Partials        730      730              
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-hudi/pull/1572?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | [...java/org/apache/hudi/config/HoodieWriteConfig.java](https://codecov.io/gh/apache/incubator-hudi/pull/1572/diff?src=pr&el=tree#diff-aHVkaS1jbGllbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvY29uZmlnL0hvb2RpZVdyaXRlQ29uZmlnLmphdmE=) | `82.00% <0.00%> (-2.85%)` | `0.00 <0.00> (ø)` | |
   | [...rg/apache/hudi/metrics/MetricsReporterFactory.java](https://codecov.io/gh/apache/incubator-hudi/pull/1572/diff?src=pr&el=tree#diff-aHVkaS1jbGllbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvbWV0cmljcy9NZXRyaWNzUmVwb3J0ZXJGYWN0b3J5LmphdmE=) | `40.00% <0.00%> (-6.16%)` | `0.00 <0.00> (ø)` | |
   | [...apache/hudi/metrics/datadog/DatadogHttpClient.java](https://codecov.io/gh/apache/incubator-hudi/pull/1572/diff?src=pr&el=tree#diff-aHVkaS1jbGllbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvbWV0cmljcy9kYXRhZG9nL0RhdGFkb2dIdHRwQ2xpZW50LmphdmE=) | `0.00% <0.00%> (ø)` | `0.00 <0.00> (?)` | |
   | [...e/hudi/metrics/datadog/DatadogMetricsReporter.java](https://codecov.io/gh/apache/incubator-hudi/pull/1572/diff?src=pr&el=tree#diff-aHVkaS1jbGllbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvbWV0cmljcy9kYXRhZG9nL0RhdGFkb2dNZXRyaWNzUmVwb3J0ZXIuamF2YQ==) | `0.00% <0.00%> (ø)` | `0.00 <0.00> (?)` | |
   | [...g/apache/hudi/metrics/datadog/DatadogReporter.java](https://codecov.io/gh/apache/incubator-hudi/pull/1572/diff?src=pr&el=tree#diff-aHVkaS1jbGllbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvbWV0cmljcy9kYXRhZG9nL0RhdGFkb2dSZXBvcnRlci5qYXZh) | `0.00% <0.00%> (ø)` | `0.00 <0.00> (?)` | |
   | [...va/org/apache/hudi/config/HoodieMetricsConfig.java](https://codecov.io/gh/apache/incubator-hudi/pull/1572/diff?src=pr&el=tree#diff-aHVkaS1jbGllbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvY29uZmlnL0hvb2RpZU1ldHJpY3NDb25maWcuamF2YQ==) | `40.38% <11.76%> (-12.40%)` | `0.00 <0.00> (ø)` | |
   | [...src/main/java/org/apache/hudi/metrics/Metrics.java](https://codecov.io/gh/apache/incubator-hudi/pull/1572/diff?src=pr&el=tree#diff-aHVkaS1jbGllbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvbWV0cmljcy9NZXRyaWNzLmphdmE=) | `56.75% <100.00%> (ø)` | `0.00 <0.00> (ø)` | |
   | [...a/org/apache/hudi/metrics/MetricsReporterType.java](https://codecov.io/gh/apache/incubator-hudi/pull/1572/diff?src=pr&el=tree#diff-aHVkaS1jbGllbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvbWV0cmljcy9NZXRyaWNzUmVwb3J0ZXJUeXBlLmphdmE=) | `100.00% <100.00%> (ø)` | `0.00 <0.00> (ø)` | |
   | [...che/hudi/common/util/BufferedRandomAccessFile.java](https://codecov.io/gh/apache/incubator-hudi/pull/1572/diff?src=pr&el=tree#diff-aHVkaS1jb21tb24vc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvY29tbW9uL3V0aWwvQnVmZmVyZWRSYW5kb21BY2Nlc3NGaWxlLmphdmE=) | `54.38% <0.00%> (-0.88%)` | `0.00% <0.00%> (ø%)` | |
   | ... and [3 more](https://codecov.io/gh/apache/incubator-hudi/pull/1572/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-hudi/pull/1572?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-hudi/pull/1572?src=pr&el=footer). Last update [9059bce...04e47c4](https://codecov.io/gh/apache/incubator-hudi/pull/1572?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
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] [incubator-hudi] vinothchandar commented on pull request #1572: [HUDI-836] Implement datadog metrics reporter

Posted by GitBox <gi...@apache.org>.
vinothchandar commented on pull request #1572:
URL: https://github.com/apache/incubator-hudi/pull/1572#issuecomment-632774153


   Sorry folks.. Got delayed ..  This looks fine at a high level.. I was mulling if using http-client directly will cause any issues for us.. but seems ok.. 
   
   If something comes up, we can fix forward.. 


----------------------------------------------------------------
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] [incubator-hudi] xushiyan commented on a change in pull request #1572: [HUDI-836] Implement datadog metrics reporter

Posted by GitBox <gi...@apache.org>.
xushiyan commented on a change in pull request #1572:
URL: https://github.com/apache/incubator-hudi/pull/1572#discussion_r422527119



##########
File path: pom.xml
##########
@@ -245,6 +245,7 @@
         <version>${maven-surefire-plugin.version}</version>
         <configuration>
           <skip>${skipUTs}</skip>
+          <argLine>-Xms256m -Xmx2g</argLine>

Review comment:
       it was there when i got OOM in travis...maybe not needed now. let me try removing it




----------------------------------------------------------------
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