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

[improve] add metrics: total_entry_log_space_bytes #4507

Merged

Conversation

ethqunzhong
Copy link
Contributor

Motivation

add metric TOTAL_ENTRY_LOG_SPACE_BYTES As a supplement to metric ACTIVE_ENTRY_LOG_SPACE_BYTES

This allows us to easily analyze the proportion of valid data in the cluster entry log.

Changes

  1. add metric TOTAL_ENTRY_LOG_SPACE_BYTES in GarbageCollectorStats.
  2. Standardize totalEntryLogSize and activeEntryLogSize naming.

Copy link
Contributor

@liangyepianzhou liangyepianzhou left a comment

Choose a reason for hiding this comment

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

LGTM, It is also recommended to add a test

@StevenLuMT StevenLuMT closed this Feb 14, 2025
@StevenLuMT StevenLuMT reopened this Feb 14, 2025
@StevenLuMT
Copy link
Member

reopen's reason: rerun failure checks

Copy link
Member

@StevenLuMT StevenLuMT left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@hezhangjian hezhangjian left a comment

Choose a reason for hiding this comment

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

I am a little hesitat, I prefer to remove TOTAL_ perfix, it's common appears as suffix to indicate it's type. Put it in front doesn't bring additional information.

Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

+1

@eolivelli
Copy link
Contributor

For the name: we sure follow current conventions in the project codebase

@hezhangjian
Copy link
Member

@eolivelli I didn't see "TOTAL_" prefix in other metric name. did I miss something?

@StevenLuMT
Copy link
Member

+1, It might be better to change total_entry_log_space_bytes to entry_log_space_bytes.

@ethqunzhong
Copy link
Contributor Author

+1, It might be better to change total_entry_log_space_bytes to entry_log_space_bytes.

@StevenLuMT thx, make sense. updated.

@dlg99
Copy link
Contributor

dlg99 commented Feb 17, 2025

Please fix the checkstyle error. Otherwise LGTM.

[INFO] There is 1 error reported by Checkstyle 9.3 with buildtools/src/main/resources/bookkeeper/checkstyle.xml ruleset.
Error:  src/main/java/org/apache/bookkeeper/bookie/stats/GarbageCollectorStats.java:[38,1] (imports) ImportOrder: Import org.apache.bookkeeper.bookie.BookKeeperServerStats.ENTRY_LOG_SPACE_BYTES appears after other imports that it should precede

Signed-off-by: Zhangjian He <[email protected]>
@StevenLuMT
Copy link
Member

image

@ethqunzhong please fix it

@hezhangjian hezhangjian merged commit 606db74 into apache:master Feb 18, 2025
23 checks passed
@hezhangjian
Copy link
Member

Thanks for your contribution. :)

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.

6 participants