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.

Sign up for GitHub

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

Jump to bottom

Bookie server lost some metrics of OrderedExecutor #4373

Closed
TakaHiR07 opened this issue May 17, 2024 · 0 comments · Fixed by #4374
Closed

Bookie server lost some metrics of OrderedExecutor #4373

TakaHiR07 opened this issue May 17, 2024 · 0 comments · Fixed by #4374
Labels
type/bug

Comments

@TakaHiR07
Copy link
Contributor

TakaHiR07 commented May 17, 2024 •
edited
Loading

BUG REPORT

Describe the bug

We found that bookie lost some metrics of OrderedExecutor. The problem is caused after #3546

The root cause is : If we disable traceTaskExecution, it is ok. But if we enable traceTaskExecution, SingleThreadExecutor would do addExecutorDecorators(). Then it can't go through the judge if (thread instanceof SingleThreadExecutor). Therefore, singleThreadExecutor can not do registerMetrics.

企业微信截图_2afea188-7d24-4be9-be45-0fba5a517eb9

企业微信截图_8439ec29-1307-4d62-ad26-7753c038aeb9

After diving into the code. I found that there are several areas need to be improved in OrderedExecutor and SingleThreadExecutor.

  1. fix the lost metrics
  2. no need to keep the code relevant to if (thread instanceof ThreadPoolExecutor). This part of code never trigger
  3. SingleThreadExecutor#registerMetrics register duplicate metric. This is redundant
  4. SingleThreadExecutor#registerMetrics use different format to register metric, which is different from the previous version. I think it's better the keep the previous metric format

Duplicate metric and different metric format:
企业微信截图_c48701cd-9da2-496d-abac-c30d288b5edb

企业微信截图_402b81a0-58aa-40e9-9729-d95095e04b08
@TakaHiR07 TakaHiR07 mentioned this issue May 17, 2024
Merged
shoothzj pushed a commit that referenced this issue May 25, 2024
@TakaHiR07
fix OrderedExecutor lost some metric (#4374)
e5300a0 
Fix [#4373](#4373)

### Motivation

As shown in the issue.

This is the first pr to fix the lost metric problem. And the other pr improve the metric in SingleThreadExecutor.

Co-authored-by: fanjianye <fanjianye@bigo.sg>
shoothzj pushed a commit that referenced this issue May 25, 2024
@TakaHiR07 @shoothzj
fix OrderedExecutor lost some metric (#4374)
8829ed7 
Fix [#4373](#4373)

### Motivation

As shown in the issue.

This is the first pr to fix the lost metric problem. And the other pr improve the metric in SingleThreadExecutor.

Co-authored-by: fanjianye <fanjianye@bigo.sg>
(cherry picked from commit e5300a0)
shoothzj pushed a commit that referenced this issue May 25, 2024
@TakaHiR07 @shoothzj
fix OrderedExecutor lost some metric (#4374)
2fa28a8 
Fix [#4373](#4373)

### Motivation

As shown in the issue.

This is the first pr to fix the lost metric problem. And the other pr improve the metric in SingleThreadExecutor.

Co-authored-by: fanjianye <fanjianye@bigo.sg>
(cherry picked from commit e5300a0)
@TakaHiR07 TakaHiR07 mentioned this issue May 27, 2024
Open
Ghatage pushed a commit to sijie/bookkeeper that referenced this issue Jul 12, 2024
@TakaHiR07
fix OrderedExecutor lost some metric (apache#4374)
fda3862 
Fix [apache#4373](apache#4373)

### Motivation

As shown in the issue.

This is the first pr to fix the lost metric problem. And the other pr improve the metric in SingleThreadExecutor.

Co-authored-by: fanjianye <fanjianye@bigo.sg>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
type/bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix lost prometheus metric in OrderedExecutor TakaHiR07/bookkeeper
1 participant
@TakaHiR07