Skip to content

[#1356] improvement: add metric of total expired pre-allocated buffers - #1412

Merged
zuston merged 6 commits into
apache:masterfrom
lifeSo:feature_1356_expired_pre_allocated_buffer
Jan 5, 2024
Merged

[#1356] improvement: add metric of total expired pre-allocated buffers#1412
zuston merged 6 commits into
apache:masterfrom
lifeSo:feature_1356_expired_pre_allocated_buffer

Conversation

@lifeSo

@lifeSo lifeSo commented Dec 30, 2023

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

Add metrics of expired pre-allocated buffer size

Why are the changes needed?

Fix: #1356

Does this PR introduce any user-facing change?

No.

How was this patch tested?

test on PRD

@codecov-commenter

codecov-commenter commented Dec 30, 2023 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (eb88143) 53.25% compared to head (fb638d5) 54.15%.
Report is 24 commits behind head on master.

Additional details and impacted files 
@@             Coverage Diff              @@
##             master    #1412      +/-   ##
============================================
+ Coverage     53.25%   54.15%   +0.90%     
- Complexity     2715     2721       +6     
============================================
  Files           418      399      -19     
  Lines         23918    21629    -2289     
  Branches       2043     2045       +2     
============================================
- Hits          12738    11714    -1024     
+ Misses        10395     9199    -1196     
+ Partials        785      716      -69

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@lifeSo

lifeSo commented Dec 31, 2023

Copy link
Copy Markdown
Contributor Author

PTAL @jerqi @zuston

@lifeSo lifeSo changed the title [#1356] FEATURE: add metrics of expired pre-allocated buffer size #1356 [#1356] feature: add metrics of expired pre-allocated buffer size #1356 Dec 31, 2023
@lifeSo lifeSo changed the title [#1356] feature: add metrics of expired pre-allocated buffer size #1356 [#1356] improvement: add metrics of expired pre-allocated buffer size #1356 Dec 31, 2023
roryqi
roryqi previously approved these changes Jan 2, 2024
View reviewed changes

@roryqi roryqi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM.

zuston
zuston reviewed Jan 2, 2024
View reviewed changes

@zuston zuston left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think the expired require buffer id number also could be collected as a metric, this could be involved in this PR. WDYT?

Comment thread server/src/main/java/org/apache/uniffle/server/ShuffleServerMetrics.java Outdated

public static Gauge.Child gaugeIsHealthy;
public static Gauge.Child gaugeAllocatedBufferSize;
public static Gauge.Child gaugePreAllocatedBufferSize;

@zuston zuston Jan 2, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

why changing this metric name?

@zuston zuston Jan 4, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ping @lifeSo

@lifeSo
lifeSo force-pushed the feature_1356_expired_pre_allocated_buffer branch from cb346f2 to 7f9f918 Compare January 2, 2024 20:37
@lifeSo
lifeSo requested review from roryqi and zuston January 3, 2024 02:27
@lifeSo

lifeSo commented Jan 3, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

I think the expired require buffer id number also could be collected as a metric, this could be involved in this PR. WDYT?

@zuston
I code the function, but I think and think, think that "expired require buffer id number " is meanless, because buffer id may be duplicated for different apps, and can not determine buffer id belong to which apps.

@zuston

zuston commented Jan 3, 2024

Copy link
Copy Markdown
Member

Emm. you misunderstand my thought. I hope the number of expired buffer rather than buffer id list. This number increase could reflect something wrong.

@lifeSo

lifeSo commented Jan 3, 2024

Copy link
Copy Markdown
Contributor Author

Emm. you misunderstand my thought. I hope the number of expired buffer rather than buffer id list. This number increase could reflect something wrong.

@zuston Get it and i coded it, PTAL.
@jerqi May you help review the code, according the requirement, I think it is ok.

zuston
zuston reviewed Jan 4, 2024
View reviewed changes
Comment thread server/src/main/java/org/apache/uniffle/server/ShuffleServerMetrics.java Outdated

private static final String IS_HEALTHY = "is_healthy";
private static final String ALLOCATED_BUFFER_SIZE = "allocated_buffer_size";
private static final String EXPIRED_PRE_ALLOCATED_BUFFER_SIZE =

@zuston zuston Jan 4, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

these metrics should be counter rather than gauge. Right ?

And the name should be obey the rule. xxxx_total

zhengchenyu
zhengchenyu reviewed Jan 4, 2024
View reviewed changes
Comment thread server/src/main/java/org/apache/uniffle/server/ShuffleTaskManager.java
// move release memory code down to here as the requiredBuffer could be consumed during
// removing processing.
shuffleBufferManager.releaseMemory(info.getRequireSize(), false, true);
ShuffleServerMetrics.counterExpiredPreAllocatedBufferSizeTotal.inc(info.getRequireSize());

@zhengchenyu zhengchenyu Jan 4, 2024

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

CounterExpiredPreAllocatedBufferSizeTotal is necessary? I feel A bit repetitive with counterExpiredPreAllocatedBufferIdTotal.
If we're just trying to capture how often timeouts happen, counterExpiredPreAllocatedBufferIdTotal maybe enough. But I'm not sure if you serve any other purpose.

@zuston zuston Jan 4, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

the total expired buffer size is to diagnose cases where allocated metric is negative. I will compare these value to check what happen of allocated is leaked.

zuston
zuston reviewed Jan 4, 2024
View reviewed changes

@zuston zuston left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Overall lgtm. Please solve the rest comment.

Comment thread server/src/main/java/org/apache/uniffle/server/ShuffleServerMetrics.java Outdated

public static Gauge.Child gaugeIsHealthy;
public static Gauge.Child gaugeAllocatedBufferSize;
public static Gauge.Child gaugePreAllocatedBufferSize;

@zuston zuston Jan 4, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ping @lifeSo

@lifeSo
lifeSo requested a review from zuston January 4, 2024 13:01
zuston
zuston approved these changes Jan 5, 2024
View reviewed changes

@zuston zuston left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM.

@zuston zuston changed the title [#1356] improvement: add metrics of expired pre-allocated buffer size #1356 [#1356] improvement: add metric of total expired pre-allocated buffers #1356 Jan 5, 2024
@zuston zuston changed the title [#1356] improvement: add metric of total expired pre-allocated buffers #1356 [#1356] improvement: add metric of total expired pre-allocated buffers Jan 5, 2024
@zuston
zuston merged commit 7d59126 into apache:master Jan 5, 2024
@lifeSo
lifeSo deleted the feature_1356_expired_pre_allocated_buffer branch January 5, 2024 03:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zhengchenyu zhengchenyu zhengchenyu left review comments
@zuston zuston zuston approved these changes
@roryqi roryqi Awaiting requested review from roryqi

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[FEATURE] add metrics of expired pre-allocated buffer size

5 participants

@lifeSo @codecov-commenter @zuston @roryqi @zhengchenyu