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

Add Broker Cache Entry Eviction Rate Metric #16584

Closed
michaeljmarshall opened this issue Jul 13, 2022 · 3 comments · Fixed by #17248
Closed

Add Broker Cache Entry Eviction Rate Metric #16584

michaeljmarshall opened this issue Jul 13, 2022 · 3 comments · Fixed by #17248
Assignees
@michaeljmarshall
Labels
area/broker area/metrics

Comments

@michaeljmarshall
Copy link
Member

Is your enhancement request related to a problem? Please describe.
The current broker cache eviction metric, pulsar_ml_cache_evictions, measures when the cache's hasSpaceInCache triggers an eviction. There are no other cache eviction metrics right now, making it hard to gain insight into cache evictions.

Describe the solution you'd like
I would like to see metrics providing insight into the rate at which entries are getting evicted from the cache. I'm not sure if entry invalidations should also be included in this metric, but we should consider a metric for that as well.

These metrics would only be computed at the broker level.

Describe alternatives you've considered
I don't see another metric that gives insight into broker cache evictions.

Additional context
Add any other context or screenshots about the feature request here.
@michaeljmarshall
Copy link
Member Author

@lhotari - I think additional metrics like the one described above could help make an issue like #16054 easier to observe.

@sijie sijie mentioned this issue Jul 14, 2022
Closed
@tjiuming
Copy link
Contributor

tjiuming commented Aug 5, 2022

@michaeljmarshall I'd like to fix the issue

@michaeljmarshall michaeljmarshall mentioned this issue Aug 24, 2022
Merged
1 task
@michaeljmarshall michaeljmarshall self-assigned this Aug 24, 2022
@michaeljmarshall
Copy link
Member Author

@tjiuming - sorry, I missed your comment. I implemented the metrics today. Feel free to review the PR if you have time.

michaeljmarshall added a commit that referenced this issue Aug 24, 2022
@michaeljmarshall
[enh][broker] Add metrics for entry cache insertion, eviction (#17248)
e3b2540 
Fixes #16584

### Motivation

With the `RangeCache`, it is hard to reason about its behavior other than cache hits/misses or the cache's size hitting the limit and triggering a size based eviction. This PR adds 3 new metrics to help provide additional insight into the cache's behavior. It adds `pulsar_ml_cache_inserted_entries_total`, `pulsar_ml_cache_evicted_entries_total`, and `pulsar_ml_cache_entries`.

### Modifications

* Add new metrics for cache insertion, eviction, and current number of entries.
* Add new methods to the `ManagedLedgerFactoryMXBean` interface.
* Update several method return values in the `RangeCache`.
* Update tests.

### Verifying this change

This change is covered by modified tests that already existed.

### Does this pull request potentially affect one of the following parts:

There is a breaking change to the `RangeCache` class for the `clear` and the `evictLEntriesBeforeTimestamp` methods. The previous result was a `long`, and now it is a `Pair<Integer, Long>`. The new result matches the same style as `evictLeastAccessedEntries`. Given that this class is only meant for use within the broker, I think it is reasonable to break these methods. I will send a note to the mailing list.

### Documentation
  
- [x] `doc`
Nicklee007 pushed a commit to Nicklee007/pulsar that referenced this issue Aug 29, 2022
@michaeljmarshall
[enh][broker] Add metrics for entry cache insertion, eviction (apache…
b2b73f4 
…#17248)

Fixes apache#16584

### Motivation

With the `RangeCache`, it is hard to reason about its behavior other than cache hits/misses or the cache's size hitting the limit and triggering a size based eviction. This PR adds 3 new metrics to help provide additional insight into the cache's behavior. It adds `pulsar_ml_cache_inserted_entries_total`, `pulsar_ml_cache_evicted_entries_total`, and `pulsar_ml_cache_entries`.

### Modifications

* Add new metrics for cache insertion, eviction, and current number of entries.
* Add new methods to the `ManagedLedgerFactoryMXBean` interface.
* Update several method return values in the `RangeCache`.
* Update tests.

### Verifying this change

This change is covered by modified tests that already existed.

### Does this pull request potentially affect one of the following parts:

There is a breaking change to the `RangeCache` class for the `clear` and the `evictLEntriesBeforeTimestamp` methods. The previous result was a `long`, and now it is a `Pair<Integer, Long>`. The new result matches the same style as `evictLeastAccessedEntries`. Given that this class is only meant for use within the broker, I think it is reasonable to break these methods. I will send a note to the mailing list.

### Documentation
  
- [x] `doc`
Technoboy- pushed a commit that referenced this issue Oct 13, 2022
@michaeljmarshall @Technoboy-
[enh][broker] Add metrics for entry cache insertion, eviction (#17248)
df33f59 
Fixes #16584

### Motivation

With the `RangeCache`, it is hard to reason about its behavior other than cache hits/misses or the cache's size hitting the limit and triggering a size based eviction. This PR adds 3 new metrics to help provide additional insight into the cache's behavior. It adds `pulsar_ml_cache_inserted_entries_total`, `pulsar_ml_cache_evicted_entries_total`, and `pulsar_ml_cache_entries`.

### Modifications

* Add new metrics for cache insertion, eviction, and current number of entries.
* Add new methods to the `ManagedLedgerFactoryMXBean` interface.
* Update several method return values in the `RangeCache`.
* Update tests.

### Verifying this change

This change is covered by modified tests that already existed.

### Does this pull request potentially affect one of the following parts:

There is a breaking change to the `RangeCache` class for the `clear` and the `evictLEntriesBeforeTimestamp` methods. The previous result was a `long`, and now it is a `Pair<Integer, Long>`. The new result matches the same style as `evictLeastAccessedEntries`. Given that this class is only meant for use within the broker, I think it is reasonable to break these methods. I will send a note to the mailing list.

### Documentation
  
- [x] `doc`
congbobo184 pushed a commit that referenced this issue Nov 17, 2022
@michaeljmarshall
[enh][broker] Add metrics for entry cache insertion, eviction (#17248)
bd0c56a 
Fixes #16584

With the `RangeCache`, it is hard to reason about its behavior other than cache hits/misses or the cache's size hitting the limit and triggering a size based eviction. This PR adds 3 new metrics to help provide additional insight into the cache's behavior. It adds `pulsar_ml_cache_inserted_entries_total`, `pulsar_ml_cache_evicted_entries_total`, and `pulsar_ml_cache_entries`.

* Add new metrics for cache insertion, eviction, and current number of entries.
* Add new methods to the `ManagedLedgerFactoryMXBean` interface.
* Update several method return values in the `RangeCache`.
* Update tests.

This change is covered by modified tests that already existed.

There is a breaking change to the `RangeCache` class for the `clear` and the `evictLEntriesBeforeTimestamp` methods. The previous result was a `long`, and now it is a `Pair<Integer, Long>`. The new result matches the same style as `evictLeastAccessedEntries`. Given that this class is only meant for use within the broker, I think it is reasonable to break these methods. I will send a note to the mailing list.

- [x] `doc`

(cherry picked from commit e3b2540)
congbobo184 pushed a commit that referenced this issue Dec 7, 2022
@michaeljmarshall
[enh][broker] Add metrics for entry cache insertion, eviction (#17248)
9df0048 
Fixes #16584

With the `RangeCache`, it is hard to reason about its behavior other than cache hits/misses or the cache's size hitting the limit and triggering a size based eviction. This PR adds 3 new metrics to help provide additional insight into the cache's behavior. It adds `pulsar_ml_cache_inserted_entries_total`, `pulsar_ml_cache_evicted_entries_total`, and `pulsar_ml_cache_entries`.

* Add new metrics for cache insertion, eviction, and current number of entries.
* Add new methods to the `ManagedLedgerFactoryMXBean` interface.
* Update several method return values in the `RangeCache`.
* Update tests.

This change is covered by modified tests that already existed.

There is a breaking change to the `RangeCache` class for the `clear` and the `evictLEntriesBeforeTimestamp` methods. The previous result was a `long`, and now it is a `Pair<Integer, Long>`. The new result matches the same style as `evictLeastAccessedEntries`. Given that this class is only meant for use within the broker, I think it is reasonable to break these methods. I will send a note to the mailing list.

- [x] `doc`

(cherry picked from commit e3b2540)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@michaeljmarshall michaeljmarshall

Labels
area/broker area/metrics
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[enh][broker] Add metrics for entry cache insertion, eviction michaeljmarshall/pulsar
2 participants
@michaeljmarshall @tjiuming