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

feat: Add metrics for cache hit/miss for object store cache #1405

Merged
merged 8 commits into from
Apr 18, 2023

Conversation

nearsyh
Copy link
Contributor

@nearsyh nearsyh commented Apr 17, 2023

I hereby agree to the terms of the GreptimeDB CLA

What's changed and what's your intention?

Add two cache metrics to record cache hit/miss. #1276

  • Metric names:
    • lru_cache.hit
    • lru_cache.miss
  • Metric labels:
    • lru_cache.name: its value is set as "path"
    • lru_cache.range: its value is set as the range in the read arguments.

Please explain IN DETAIL what the changes are in this PR and why they are needed:

  • Summarize your change (mandatory)
    • Add metrics to record cache hit/miss

Note: I'm not sure about these:

  • Which labels should we use. I assume the cardinality of "range" is limited, and it's helpful to include it as a label. Let me know if my understanding is wrong.
  • Naming convention about the metrics.

Checklist

  • I have written the necessary rustdoc comments.
  • I have added the necessary unit tests and integration tests.

Refer to a related PR or issue link (optional)

#1276

@nearsyh nearsyh changed the title Add metrics for cache hit/miss for object store cache feat: Add metrics for cache hit/miss for object store cache Apr 17, 2023
src/object-store/src/cache_policy.rs Outdated Show resolved Hide resolved
src/object-store/src/cache_policy.rs Outdated Show resolved Hide resolved
src/object-store/src/cache_policy.rs Outdated Show resolved Hide resolved
@killme2008
Copy link
Contributor

@nearsyh Thanks a lot for your contribution, there are some minor suggestions.

src/object-store/src/metrics.rs Outdated Show resolved Hide resolved
src/object-store/src/metrics.rs Outdated Show resolved Hide resolved
src/object-store/src/metrics.rs Outdated Show resolved Hide resolved
src/object-store/src/cache_policy.rs Show resolved Hide resolved
src/object-store/src/cache_policy.rs Show resolved Hide resolved
src/object-store/src/cache_policy.rs Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Apr 18, 2023

Codecov Report

Merging #1405 (7de8229) into develop (408de51) will decrease coverage by 0.58%.
The diff coverage is 74.13%.

@@             Coverage Diff             @@
##           develop    #1405      +/-   ##
===========================================
- Coverage    86.00%   85.42%   -0.58%     
===========================================
  Files          533      534       +1     
  Lines        79474    79521      +47     
===========================================
- Hits         68350    67934     -416     
- Misses       11124    11587     +463     

Copy link
Contributor

@killme2008 killme2008 left a comment

Choose a reason for hiding this comment

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

LGTM

@nearsyh
Copy link
Contributor Author

nearsyh commented Apr 18, 2023

@killme2008 Just to confirm, is there anything else I need to do for this PR?

@evenyag
Copy link
Contributor

evenyag commented Apr 18, 2023

@nearsyh Thx, I'm going to merge this as all checks have passed.

@evenyag evenyag merged commit c6f024a into GreptimeTeam:develop Apr 18, 2023
@killme2008
Copy link
Contributor

@nearsyh Congrats on your first PR! Thank you.

@nearsyh nearsyh deleted the cache-metric branch April 18, 2023 06:32
paomian pushed a commit to paomian/greptimedb that referenced this pull request Oct 19, 2023
…Team#1405)

* Add the cache hit/miss counter

* Verify the cache metrics are included

* Resolve comments

* Rename the error kind label name to be consistent with other metrics

* Rename the object store metric names

* Avoid using glob imports

* Format the code

* chore: Update src/object-store/src/metrics.rs mod doc

---------

Co-authored-by: Yingwen <realevenyag@gmail.com>
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.

3 participants