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

metric: Add memdb size and key count metric #495

Closed
wants to merge 3 commits into from

Conversation

longfangsong
Copy link
Member

Add size and keys count metric for memdb

Signed-off-by: longfangsong <longfangsong@icloud.com>
@longfangsong longfangsong force-pushed the memdb-metric branch 2 times, most recently from 4578e55 to 3ead4f6 Compare May 18, 2022 03:55
@longfangsong longfangsong marked this pull request as ready for review May 18, 2022 03:55
@longfangsong longfangsong changed the title [WIP]: Add memdb metric metric: Add memdb metric May 18, 2022
@longfangsong longfangsong changed the title metric: Add memdb metric metric: Add memdb size and key count metric May 18, 2022
@cfzjywxk cfzjywxk requested review from MyonKeminta and ekexium May 19, 2022 07:53
@@ -358,6 +361,7 @@ func (db *MemDB) setValue(x memdbNodeAddr, value []byte) {
}
x.vptr = db.vlog.appendValue(x.addr, x.vptr, value)
db.size = db.size - len(oldVal) + len(value)
metrics.MemdbSize.Set(float64(db.size))
Copy link
Contributor

Choose a reason for hiding this comment

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

Should Add be used for the MemdbSize as it's a server metric?

Copy link
Contributor

Choose a reason for hiding this comment

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

@ekexium
Copy link
Contributor

ekexium commented May 19, 2022

There could be multiple MemDBs. I think Gauge for MemdbSize is inappropriate. Maybe a Histogram? Gauge makes sense. We just need to differentiate memdbs.

@ekexium
Copy link
Contributor

ekexium commented May 19, 2022

I'm wondering about the purpose of these metrics. It answers "how much data is in memdb" but cannot tell us a lot about "how much memory is used by memdb".

I see, it's for measuring the actual memory consumed by memdb. db.size is just the sum of lengths of keys and values we put in which is the theoretical lower bound of the actual memory consumption. Could we consider calculating the memories allocated for the memdbArenas?

We can run some experiments with large transactions and compare our metrics with the profiling results to see if it's accurate.

@ekexium
Copy link
Contributor

ekexium commented May 20, 2022

We don't have a clear idea of the relationship between db.size and the memory allocated. I'm thinking could we export both of them so that we will be able to estimate the memory usage of certain kinds of jobs, e.g.insert into select a large table?

@@ -563,6 +567,8 @@ func (db *MemDB) deleteNode(z memdbNodeAddr) {

db.count--
db.size -= int(z.klen)
metrics.MemdbCount.Dec()
Copy link
Collaborator

Choose a reason for hiding this comment

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

I prefer Set(db.count)

@cfzjywxk cfzjywxk requested a review from you06 June 15, 2022 09:47
@@ -163,6 +164,8 @@ func (db *MemDB) Reset() {
db.vlogInvalid = false
db.size = 0
db.count = 0
metrics.MemdbSize.Set(0.0)
metrics.MemdbCount.Set(0.0)
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry I didn't understand it well. The metrics looks like global gauges. May there be many MemDB instances in a single process and they all Set values to the single gauge by themselves?

Copy link
Contributor

Choose a reason for hiding this comment

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

I've mentioned it above and discussed it with @longfangsong , it's needed to be changed I think.

@ekexium
Copy link
Contributor

ekexium commented Jun 16, 2022

#520 tracks the memory allocated to the arena. That could be reused here.

@cfzjywxk
Copy link
Contributor

@longfangsong
Please address the above comments.

@longfangsong
Copy link
Member Author

@longfangsong

Please address the above comments.

I plan to continue this pr after #520 is done.

@disksing
Copy link
Collaborator

@cfzjywxk @MyonKeminta do we want to continue this?

@disksing
Copy link
Collaborator

closing due to inactivity. Please reopen it if need.

@disksing disksing closed this Sep 13, 2022
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.

5 participants