Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[HUDI-7337] Implement MetricsReporter that reports metrics to M3 #10565

Merged
merged 7 commits into from
Mar 5, 2024

Conversation

kbuci
Copy link
Contributor

@kbuci kbuci commented Jan 25, 2024

Change Logs

  • Added new MetricsReporter implementation org.apache.hudi.metrics.m3.M3MetricsReporter which emits metrics to M3
  • Added new dependencies in HUDI : com.uber.m3:tally-m3, com.uber.m3:tally-core

Impact

  • Users using https://m3db.io can enable reporting metrics to M3 by setting hoodie.metrics.reporter.type as M3 and their corresponding host address/port in hoodie.metrics.m3.host / hoodie.metrics.m3.port

Risk level (write none, low medium or high below)

low

Documentation Update

Documentation should be updated with the new config options for M3 metrics reporting:

  • hoodie.metrics.m3.host
  • hoodie.metrics.m3.port
  • hoodie.metrics.m3.tags
  • hoodie.metrics.m3.service
  • hoodie.metrics.m3.env

Contributor's checklist

  • Read through contributor's guide
  • Change Logs and Impact were stated clearly
  • Adequate tests were added if applicable
  • CI passed

@kbuci kbuci requested a review from danny0405 January 26, 2024 19:48
@@ -120,6 +120,16 @@
<groupId>io.prometheus</groupId>
<artifactId>simpleclient_pushgateway</artifactId>
</dependency>
<dependency>
<groupId>com.uber.m3</groupId>
<artifactId>tally-m3</artifactId>
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any risk for the uber specific jar to conflict with others?

Copy link
Contributor Author

@kbuci kbuci Jan 29, 2024

Choose a reason for hiding this comment

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

I took a look at dependencies used by com.uber.m3:tally-m3 and com.uber.m3:tally-core . It seems that the former has dependency org.apache.thrift:libthrift:jar:0.9.3 and com.google.code.findbugs:jsr305:jar:3.0.2 .
I'll take a look at how to make sure com.uber.m3:tally-m3 uses its own version of thrift and com.google.code.findbugs so that HUDI APIs (and other dependencies) won't use in tally's versions and tally will use the versions it needs, which I think I might need to use shading to solve.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For now I ended up shading com.uber.m3 in the packaging/hudi-*-bundle POM files. To be fully transparent, I'm not too familiar with MVN/Java dependency management, so I did this approach based on what I saw on the web. Since my main priority is to just make sure dependencies "brought in" by com.uber.m3 don't "override" jar versions by hoodie_oss or users. If there's a better convention/technique for guarding against future dependency conflicts in Apache Java projects then I can try doing that instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, shading is fine.

@kbuci kbuci requested a review from danny0405 January 31, 2024 00:57
@hudi-bot
Copy link

CI report:

Bot commands @hudi-bot supports the following commands:
  • @hudi-bot run azure re-run the last Azure build

@prashantwason prashantwason added the release-0.14.2 Patches/Issue fixes targetted for 0.14.2 release label Feb 6, 2024
@github-actions github-actions bot added the size:L PR with lines of changes in (300, 1000] label Feb 26, 2024
@nsivabalan nsivabalan self-assigned this Feb 28, 2024
@@ -120,6 +120,16 @@
<groupId>io.prometheus</groupId>
<artifactId>simpleclient_pushgateway</artifactId>
</dependency>
<dependency>
<groupId>com.uber.m3</groupId>
<artifactId>tally-m3</artifactId>
Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, shading is fine.

@nsivabalan nsivabalan merged commit 6c7ee8e into apache:master Mar 5, 2024
32 checks passed
yihua pushed a commit that referenced this pull request May 14, 2024
)


---------

Co-authored-by: Krishen Bhan <“bkrishen@uber.com”>
yihua pushed a commit that referenced this pull request May 14, 2024
)


---------

Co-authored-by: Krishen Bhan <“bkrishen@uber.com”>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-0.14.2 Patches/Issue fixes targetted for 0.14.2 release size:L PR with lines of changes in (300, 1000]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants