-
Notifications
You must be signed in to change notification settings - Fork 17
Add otel metrics for seidb #114
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
Conversation
masih
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One blocker about measuring latency in relation to errors. Left a bunch of suggestions.
sc/memiavl/db.go
Outdated
|
|
||
| startTime := time.Now() | ||
| defer func() { | ||
| metrics.SeiDBMetrics.RestartLatency.Record(context.Background(), time.Since(startTime).Seconds()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tag by "status" in case there is an error. Otherwise the times measured could be missleading.
Ditto for all such defer statements for functions that may also return error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make sense, will add
sc/memiavl/db.go
Outdated
| startTime := time.Now() | ||
| defer func() { | ||
|
|
||
| metrics.SeiDBMetrics.CommitLatency.Record(context.Background(), time.Since(startTime).Milliseconds()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
declare context once and reuse it?
sc/memiavl/mem_node.go
Outdated
| left Node, | ||
| right Node) *MemNode { | ||
| TotalNumOfMemNode.Add(1) | ||
| TotalMemNodeSize.Add(int64(120 + len(key))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move these increments to when the branch node is being added to its parent struct? Instantiation alone does not mean increase in size, right?
Ditto re newLeafNode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Each time we call newBranchNode or newLeafNode, it's always gonna mean the size would be increase, so I feel this is the best place to add size and count increase
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually ignore my previous comment, let me check again, the setRecursive function might be a bit confusing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, just double checked, there are only 2 places where new memNodes are created:
- setRecursive ()
Line 37 in cc4d74f
func setRecursive(node Node, key, value []byte, version, cowVersion uint32) (*MemNode, bool) { - mutate ()
sei-db/sc/memiavl/persisted_node.go
Line 147 in cc4d74f
func (node PersistedNode) Mutate(version, _ uint32) *MemNode {
So both places are creating memnodes and making it as part of the tree
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could either choose to adding the bump logic to all these places or I can choose to just add the bump size logic in initialization, which is less code duplicates. Feel like that latter is easier?
Codecov Report❌ Patch coverage is
❌ Your project status has failed because the head coverage (40.00%) is below the target coverage (60.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #114 +/- ##
==========================================
+ Coverage 39.80% 40.00% +0.19%
==========================================
Files 58 59 +1
Lines 7477 7510 +33
==========================================
+ Hits 2976 3004 +28
- Misses 4156 4161 +5
Partials 345 345
🚀 New features to boost your workflow:
|
| fileLock FileLock | ||
| ) | ||
|
|
||
| startTime := time.Now() |
Check warning
Code scanning / CodeQL
Calling the system time Warning
| db.mtx.Lock() | ||
| defer db.mtx.Unlock() | ||
|
|
||
| startTime := time.Now() |
Check warning
Code scanning / CodeQL
Calling the system time Warning
| go func() { | ||
| defer close(ch) | ||
|
|
||
| startTime := time.Now() |
Check warning
Code scanning / CodeQL
Calling the system time Warning
sc/memiavl/mem_node.go
Outdated
|
|
||
| func newLeafNode(key, value []byte, version uint32) *MemNode { | ||
| TotalNumOfMemNode.Add(1) | ||
| TotalMemNodeSize.Add(int64(120 + len(key) + len(value))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we add comments about why it's 120?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, added
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alignment & offsets (64-bit)
height = 1
padding after height = 3 (to align uint32)
version = 4 → offset now 8
size = 8 → offset 16
key = 24 → offset 40
value = 24 → offset 64
left = 16 → offset 80
right = 16 → offset 96
hash = 24 → offset 120
Total struct size = 120 bytes (already a multiple of 8, so no trailing padding).
| SnapshotCreationLatency metric.Float64Histogram | ||
| CommitLatency metric.Int64Histogram | ||
| ApplyChangesetLatency metric.Int64Histogram | ||
| MemNodeTotalSize metric.Int64Gauge |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from chatGPT: MemNodeSizeObs metric.Int64ObservableGauge type is good for “now” value , same for MemNodeCount, for your consideration
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
masih
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of gosec issues to fix, otherwise LGTM 👍

Describe your changes and provide context
This PR add couple of new metrics using otel for seidb
Testing performed to validate your change
Example:
