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

Add latency stats for entry location index lookup so that possible RocksDB bottleneck can be detected #3444

Merged

Conversation

lhotari
Copy link
Member

@lhotari lhotari commented Aug 10, 2022

Motivation

  • this metric will help to detect when the bottleneck is in the entry location index lookup operations
    and RocksDB tuning is needed

Changes

Add new operation latency metrics lookup-entry-location which will be provided at the same level of metrics as the existing entries-count metric.

- this metric will help detecting when the bottleneck is in the entry location index lookup operations
  and RocksDB tuning is needed
@lhotari lhotari force-pushed the lh-add-metric-for-entry-location-lookup branch from 8383e47 to f315686 Compare August 10, 2022 07:56
@zymap zymap requested review from eolivelli and hangc0276 August 11, 2022 01:40
@zymap zymap added this to the 4.17.0 milestone Aug 11, 2022
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
Contributor

@hangc0276 hangc0276 left a comment

Choose a reason for hiding this comment

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

We already have the index lookup stats read-locations-index-time, does it has any case this metric can't cover?

private static final String READ_ENTRY_LOCATIONS_INDEX_TIME = "read-locations-index-time";

@lhotari lhotari marked this pull request as draft August 11, 2022 16:55
@lhotari
Copy link
Member Author

lhotari commented Aug 11, 2022

We already have the index lookup stats read-locations-index-time, does it has any case this metric can't cover?

private static final String READ_ENTRY_LOCATIONS_INDEX_TIME = "read-locations-index-time";

thanks for pointing that out @hangc0276 .

The problem with the read-locations-index-time is that it's not a latency metric. it's a counter metric that records the total time spent for getLocationIndex calls.
I also noticed that the metric is not used for the getLocation call in entryExists method:

// Read from main storage
long entryLocation = entryLocationIndex.getLocation(ledgerId, entryId);
if (entryLocation != 0) {
return true;
}
.

@lhotari lhotari marked this pull request as ready for review August 11, 2022 17:04
@lhotari lhotari requested a review from hangc0276 August 13, 2022 08:47
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.

LGTM

@eolivelli eolivelli merged commit 19fd8f7 into apache:master Aug 16, 2022
zymap pushed a commit that referenced this pull request Sep 21, 2022
…cksDB bottleneck can be detected (#3444)

* Add operation latency stats for entry location lookup

- this metric will help detecting when the bottleneck is in the entry location index lookup operations
  and RocksDB tuning is needed

* Address review feedback: fix issue with eventLatencyMillis variable

* Rename misleading parameter name

(cherry picked from commit 19fd8f7)
@hangc0276 hangc0276 modified the milestones: 4.17.0, 4.16.0 Oct 14, 2022
hangc0276 pushed a commit to hangc0276/bookkeeper that referenced this pull request Nov 7, 2022
…cksDB bottleneck can be detected (apache#3444)

* Add operation latency stats for entry location lookup

- this metric will help detecting when the bottleneck is in the entry location index lookup operations
  and RocksDB tuning is needed

* Address review feedback: fix issue with eventLatencyMillis variable

* Rename misleading parameter name

(cherry picked from commit 19fd8f7)
hangc0276 pushed a commit to hangc0276/bookkeeper that referenced this pull request Nov 7, 2022
…cksDB bottleneck can be detected (apache#3444)

* Add operation latency stats for entry location lookup

- this metric will help detecting when the bottleneck is in the entry location index lookup operations
  and RocksDB tuning is needed

* Address review feedback: fix issue with eventLatencyMillis variable

* Rename misleading parameter name

(cherry picked from commit 19fd8f7)
nicoloboschi pushed a commit to datastax/bookkeeper that referenced this pull request Jan 11, 2023
…cksDB bottleneck can be detected (apache#3444)

* Add operation latency stats for entry location lookup

- this metric will help detecting when the bottleneck is in the entry location index lookup operations
  and RocksDB tuning is needed

* Address review feedback: fix issue with eventLatencyMillis variable

* Rename misleading parameter name

(cherry picked from commit 19fd8f7)
(cherry picked from commit d2d8194)
Ghatage pushed a commit to sijie/bookkeeper that referenced this pull request Jul 12, 2024
…cksDB bottleneck can be detected (apache#3444)

* Add operation latency stats for entry location lookup

- this metric will help detecting when the bottleneck is in the entry location index lookup operations
  and RocksDB tuning is needed

* Address review feedback: fix issue with eventLatencyMillis variable

* Rename misleading parameter name
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants