Skip to content

Conversation

@smallzhongfeng
Copy link
Contributor

What changes were proposed in this pull request?

Add tag as label in the metrics.
image

So that we can separate the metrics in Grafana.

Why are the changes needed?

Fix: #778

Does this PR introduce any user-facing change?

No.

How was this patch tested?

fix uts.

@smallzhongfeng smallzhongfeng changed the title #778 feat: Separate ShuffleServer metrics through tags [#778] feat: Separate ShuffleServer metrics through tags Apr 11, 2023
@codecov-commenter
Copy link

codecov-commenter commented Apr 11, 2023

Codecov Report

Merging #812 (4224baa) into master (cae7cd9) will increase coverage by 1.19%.
The diff coverage is 90.74%.

@@             Coverage Diff              @@
##             master     #812      +/-   ##
============================================
+ Coverage     57.63%   58.83%   +1.19%     
- Complexity     2058     2068      +10     
============================================
  Files           306      292      -14     
  Lines         14871    13020    -1851     
  Branches       1221     1238      +17     
============================================
- Hits           8571     7660     -911     
+ Misses         5808     4922     -886     
+ Partials        492      438      -54     
Impacted Files Coverage Δ
.../java/org/apache/uniffle/server/ShuffleServer.java 66.95% <53.84%> (-2.42%) ⬇️
.../apache/uniffle/common/metrics/MetricsManager.java 69.69% <75.00%> (+1.27%) ⬆️
...org/apache/uniffle/common/metrics/GRPCMetrics.java 51.31% <81.48%> (+2.70%) ⬆️
...pache/uniffle/common/metrics/EmptyGRPCMetrics.java 100.00% <100.00%> (ø)
...fle/coordinator/metric/CoordinatorGrpcMetrics.java 100.00% <100.00%> (ø)
...uniffle/coordinator/metric/CoordinatorMetrics.java 91.89% <100.00%> (+0.46%) ⬆️
...pache/uniffle/server/ShuffleServerGrpcMetrics.java 100.00% <100.00%> (ø)
...rg/apache/uniffle/server/ShuffleServerMetrics.java 95.74% <100.00%> (+0.14%) ⬆️

... and 33 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@advancedxy
Copy link
Contributor

@smallzhongfeng Thanks for your work.. When I'm working on #793, I made similar changes. However it seems too much changes and it was primary to label cluster name and I thought it wasn't worth it.

Do you have some more input for this one? I'm OK to accept this PR with some comments.

@smallzhongfeng
Copy link
Contributor Author

smallzhongfeng commented Apr 11, 2023

Thanks @advancedxy , but currently, I'm sorry that I haven't found a better way to make the code more elegant.. :-(

@jerqi jerqi requested a review from advancedxy April 11, 2023 11:15
@smallzhongfeng
Copy link
Contributor Author

If you have time, please help me take a look again. Thanks @advancedxy :-)

Copy link
Contributor

@advancedxy advancedxy left a comment

Choose a reason for hiding this comment

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

LGTM, except one minor comment.

@smallzhongfeng
Copy link
Contributor Author

Also cc @jerqi ,thanks!

@smallzhongfeng smallzhongfeng merged commit f361ac0 into apache:master Apr 17, 2023
@smallzhongfeng
Copy link
Contributor Author

Thanks! @advancedxy @jerqi Merged to master.

@smallzhongfeng smallzhongfeng deleted the Uniffle-778 branch May 9, 2023 08:04
@smlHao
Copy link

smlHao commented Jun 25, 2023

What changes were proposed in this pull request?

Add tag as label in the metrics. image

So that we can separate the metrics in Grafana.

Why are the changes needed?

Fix: #778

Does this PR introduce any user-facing change?

No.

How was this patch tested?

fix uts.

hello,I am using spark on k8s + uniffle, want to monitor uniffle by promethus in grafana, can i add your WeChat or DingDing?

@jerqi
Copy link
Contributor

jerqi commented Jun 25, 2023

9ea34cef3d263ebb1bb28b6d0978c2bd

@smlHao
Copy link

smlHao commented Jun 25, 2023

Thank you for fast reply !!!

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.

[FEATURE] Separate ShuffleServer metrics through tags

5 participants