Skip to content

[HUDI-210] Implement prometheus metrics reporter#1095

Closed
XuQianJin-Stars wants to merge 1 commit intoapache:masterfrom
XuQianJin-Stars:HUDI-210
Closed

[HUDI-210] Implement prometheus metrics reporter#1095
XuQianJin-Stars wants to merge 1 commit intoapache:masterfrom
XuQianJin-Stars:HUDI-210

Conversation

@XuQianJin-Stars
Copy link
Contributor

@XuQianJin-Stars XuQianJin-Stars commented Dec 11, 2019

What is the purpose of the pull request

Since Prometheus is a very popular monitoring system and time series database, it would be better to provide a metrics reporter to report metrics to prometheus.

Brief change log

The changes are as follows:

  • pom.xml
  • hudi-client/pom.xml
  • hudi-client/src/main/java/org/apache/hudi/config/HoodieMetricsConfig.java
  • hudi-client/src/main/java/org/apache/hudi/config/HoodieWriteConfig.java
  • metrics/MetricsPrometheusPushGatewayReporter.java
  • metrics/MetricsReporterFactory.java
  • metrics/MetricsReporterType.java

Verify this pull request

This change added tests and can be verified as follows:

  • By TestHoodiePrometheusPushGatewayMetrics.java to verify the change.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, default port is 9091.

@lamberken
Copy link
Member

lamberken commented Dec 11, 2019

hi, Thanks for opening the PR @XuQianJin-Stars, this test case is useless. Because PushGateway use io.prometheus.client.Gauge instead of com.codahale.metrics.Gauge.
It need to regist io.prometheus.client.Gauge firstly, after that we can send these metrics.

For example, MetricRegistry registry = new MetricRegistry(); belllow is redundant.

PushGateway pushGateway = new PushGateway("localhost:9091");


MetricRegistry registry = new MetricRegistry();
registry.register("push_gateway1", (Gauge<Long>) () -> 123L);


io.prometheus.client.Gauge submitActiveTasksGauge = io.prometheus.client.Gauge.build()
        .name("active_tasks")
        .help("active_tasks").register();
submitActiveTasksGauge.set(5L);

pushGateway.push(CollectorRegistry.defaultRegistry, "PushGateway");

Copy link
Member

@lamberken lamberken Dec 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hi, this dependency can remove.

<dependency>
  <groupId>io.prometheus</groupId>
  <artifactId>simpleclient</artifactId>
</dependency>

@XuQianJin-Stars
Copy link
Contributor Author

hi @lamber-ken Thank you very much, Let me change it.

@XuQianJin-Stars
Copy link
Contributor Author

hi, Thanks for opening the PR @XuQianJin-Stars, this test case is useless. Because PushGateway use io.prometheus.client.Gauge instead of com.codahale.metrics.Gauge.
It need to regist io.prometheus.client.Gauge firstly, after that we can send these metrics.

For example, MetricRegistry registry = new MetricRegistry(); belllow is redundant.

PushGateway pushGateway = new PushGateway("localhost:9091");


MetricRegistry registry = new MetricRegistry();
registry.register("push_gateway1", (Gauge<Long>) () -> 123L);


io.prometheus.client.Gauge submitActiveTasksGauge = io.prometheus.client.Gauge.build()
        .name("active_tasks")
        .help("active_tasks").register();
submitActiveTasksGauge.set(5L);

pushGateway.push(CollectorRegistry.defaultRegistry, "PushGateway");

well, you are right. I will remove this test.

@leesf
Copy link
Contributor

leesf commented Dec 17, 2019

@lamber-ken @XuQianJin-Stars. Is it ready?

@lamberken
Copy link
Member

@lamber-ken @XuQianJin-Stars. Is it ready?

Not yet.

@XuQianJin-Stars
Copy link
Contributor Author

@lamber-ken @XuQianJin-Stars. Is it ready?

Not yet.

@yanghua
Copy link
Contributor

yanghua commented Jan 16, 2020

@XuQianJin-Stars What's the status of this PR?

@XuQianJin-Stars
Copy link
Contributor Author

hi @leesf @lamber-ken Hive any time to continue review this PR?

@piyushrl
Copy link

What is the status of this PR? Is it ready to merge?

@leesf
Copy link
Contributor

leesf commented May 18, 2020

What is the status of this PR? Is it ready to merge?

Hi @piyushrl Thanks for the interests, now @xushiyan is taking over the PR.

@xushiyan
Copy link
Member

What is the status of this PR? Is it ready to merge?

Hi @piyushrl Thanks for the interests, now @xushiyan is taking over the PR.

Due to conflicts, planning to resume work on this once https://issues.apache.org/jira/browse/HUDI-836 is done.

@liujinhui1994
Copy link
Contributor

@xushiyan hello,how is the progress

@xushiyan
Copy link
Member

xushiyan commented Jun 9, 2020

@xushiyan hello,how is the progress

Unfortunately I have to de-prioritize this as the test improvements are more needed at the moment. I may only be able to come back to this after a while. Please feel free to pick up the ticket in the mean time. Thanks.

@leesf
Copy link
Contributor

leesf commented Jul 31, 2020

closing this one in favor of #1726

@leesf leesf closed this Jul 31, 2020
nsivabalan pushed a commit to nsivabalan/hudi that referenced this pull request Jan 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants