feat: add func BdevGetIostat #192
Conversation
|
Warning Rate limit exceeded@derekbit has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 27 minutes and 32 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughThe changes introduce a new method Changes
Sequence Diagram(s)sequenceDiagram
participant U as User/Caller
participant C as Client
participant J as JSON Client (jsonCli)
U->>C: Call BdevGetIostat(name, perChannel)
C->>J: Construct and send "bdev_get_iostat" request
J-->>C: Return JSON output or error
C->>C: Unmarshal JSON into BdevIostatResponse
C-->>U: Return response and error (if any)
Suggested Reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/spdk/types/bdev.go (1)
149-163: Add documentation comments for BdevStats fields.While the struct is well-defined, adding documentation for each field would improve code maintainability and make it easier for users to understand what each statistic represents.
Example of improved documentation:
type BdevStats struct { + // Name is the identifier of the block device Name string `json:"name"` + // BytesRead is the total number of bytes read from the device BytesRead uint64 `json:"bytes_read"` + // NumReadOps is the count of read operations performed NumReadOps uint64 `json:"num_read_ops"` // Continue documenting other fields...
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/spdk/client/basic.go(1 hunks)pkg/spdk/types/bdev.go(1 hunks)
🔇 Additional comments (3)
pkg/spdk/client/basic.go (1)
1317-1339: Well-implemented method for retrieving block device I/O statistics.The
BdevGetIostatmethod properly implements the functionality to retrieve I/O statistics for block devices. Good job providing clear documentation for the parameters and properly handling potential errors.pkg/spdk/types/bdev.go (2)
139-142: Clean type definition for BdevIostatRequest.The struct is well-defined with appropriate JSON tags, following the project's conventions.
144-147: Good response structure design.The
BdevIostatResponsetype properly encapsulates the response data with the tick rate and a collection of device statistics.
hookak
force-pushed
the
LH10472_v2_volume_metrics
branch
from
5a30bb5 to
8c9f9d4
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/spdk/client/basic.go (1)
1317-1339: Ensure consistent error handling with the rest of the codebaseThe implementation is functional, but the error handling style differs slightly from other methods in the file.
Consider using the
errors.Wrapffunction from the "github.com/pkg/errors" package for consistency with other methods in the file:func (c *Client) BdevGetIostat(name string, perChannel bool) (resp *spdktypes.BdevIostatResponse, err error) { req := spdktypes.BdevIostatRequest{ Name: name, PerChannel: perChannel, } cmdOutput, err := c.jsonCli.SendCommand("bdev_get_iostat", req) if err != nil { - return nil, fmt.Errorf("failed to execute bdev_get_iostat: %v", err) + return nil, errors.Wrapf(err, "failed to execute bdev_get_iostat") } resp = &spdktypes.BdevIostatResponse{} if err := json.Unmarshal(cmdOutput, resp); err != nil { - return nil, fmt.Errorf("failed to parse bdev_get_iostat response: %v", err) + return nil, errors.Wrapf(err, "failed to parse bdev_get_iostat response") } return resp, nil }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/spdk/client/basic.go(1 hunks)pkg/spdk/types/bdev.go(1 hunks)
🔇 Additional comments (4)
pkg/spdk/types/bdev.go (3)
139-142: Well-defined request structureThe new
BdevIostatRequesttype is correctly structured with appropriate JSON tags and optional fields marked withomitempty, following the pattern of other request types in this file.
144-148: Clear response structure designThe
BdevIostatResponsetype properly captures the expected response format withTickRate,Ticks, and an array ofBdevStats.
150-164: Comprehensive statistics structureThe
BdevStatstype thoroughly captures all relevant I/O metrics including read/write operations, bytes transferred, latency measurements, and queue depths.pkg/spdk/client/basic.go (1)
1319-1322: Clear documentation for the methodThe documentation clearly explains the purpose and parameters of the method, following the same style as other methods in the file.
| Bdevs []BdevStats `json:"bdevs"` | ||
| } | ||
|
|
||
| type BdevStats struct { |
There was a problem hiding this comment.
| WriteLatencyTicks uint64 `json:"write_latency_ticks"` | ||
| UnmapLatencyTicks uint64 `json:"unmap_latency_ticks"` | ||
| QueueDepth uint64 `json:"queue_depth"` | ||
| IoTime uint64 `json:"io_time"` |
There was a problem hiding this comment.
@hookak What is io_time and weighted_io_time in the stat?
There was a problem hiding this comment.
@derekbit weighted_io_time is the sum of all I/O request execution times, calculated as I/O execution time × queue depth.
This link will help with understanding(https://github.com/spdk/spdk/blob/e01cb43b8578f9155d07a9bc6eee4e70a3af96b0/include/spdk/bdev.h#L1052-L1068)
There was a problem hiding this comment.
Good to know. Thank you @hookak.
|
cc @c3y1huang @shuo-wu @PhanLe1010 for review. |
hookak
force-pushed
the
LH10472_v2_volume_metrics
branch
from
8c9f9d4 to
7de4084
Compare
hookak
force-pushed
the
LH10472_v2_volume_metrics
branch
from
7de4084 to
d5274fc
Compare
|
CI failed due to the zypper package installation. I will manage to resolve it soon. |
Signed-off-by: jinhong.kim0 <[email protected]>
derekbit
force-pushed
the
LH10472_v2_volume_metrics
branch
from
d5274fc to
77616cd
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #192 +/- ##
==========================================
- Coverage 22.52% 22.46% -0.07%
==========================================
Files 34 34
Lines 5007 5022 +15
==========================================
Hits 1128 1128
- Misses 3702 3717 +15
Partials 177 177
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Which issue(s) this PR fixes:
longhorn/longhorn#10472