ME-3787: device stats #41
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes introduce two new structs, Changes
Possibly related PRs
Suggested reviewers
Warning Rate limit exceeded@waltzofpearls has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 16 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. Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 buf (1.47.2)common/messages.protoConfig file YAML parsing error: yaml: unmarshal errors: device/device.protoConfig file YAML parsing error: yaml: unmarshal errors: 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
CodeRabbit Configuration File (
|
![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
🧹 Outside diff range and nitpick comments (2)
common/messages.proto (2)
21-25: Add field documentation for better maintainability.The message structure is well-designed with appropriate types, but could benefit from field documentation similar to other messages in this file.
Consider adding field comments:
message DeviceStatsMessage { - google.protobuf.Timestamp timestamp = 1; - int64 bytes_sent = 2; - int64 bytes_received = 3; + google.protobuf.Timestamp timestamp = 1; // timestamp when the stats were collected + int64 bytes_sent = 2; // total number of bytes sent by the device + int64 bytes_received = 3; // total number of bytes received by the device }
21-25: Consider additional monitoring metrics for comprehensive device health tracking.While the current DeviceStatsMessage provides basic network statistics, consider if additional metrics would be valuable for monitoring device health and performance:
- CPU usage
- Memory usage
- Connection latency
- Error counts
This would provide a more complete picture of device health and could help with proactive maintenance and troubleshooting.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
common/messages.pb.gois excluded by!**/*.pb.godevice/device.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (4)
common/messages.pb.swift(2 hunks)common/messages.proto(3 hunks)device/device.pb.swift(4 hunks)device/device.proto(1 hunks)
🔇 Additional comments (8)
device/device.proto (1)
23-23: LGTM! The changes follow protobuf best practices.
The addition of the device_stats field is well-structured:
- Maintains backward compatibility using oneof
- Follows consistent field numbering sequence
- Uses snake_case naming convention
- Properly references the common package
Let's verify the referenced message type exists:
✅ Verification successful
The DeviceStatsMessage type is properly defined and matches the usage
The verification confirms that DeviceStatsMessage is correctly defined in common/messages.proto with appropriate fields for device statistics tracking (timestamp, bytes_sent, bytes_received).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify DeviceStatsMessage is properly defined in common/messages.proto
# Check if the DeviceStatsMessage is defined in common/messages.proto
rg -A 5 "message DeviceStatsMessage" common/messages.proto
Length of output: 193
common/messages.proto (2)
10-11: LGTM: Import statement is correctly placed and necessary.
The addition of the google/protobuf/timestamp.proto import is appropriate for the new DeviceStatsMessage structure.
83-83: LGTM: Proper file formatting.
device/device.pb.swift (3)
58-64: LGTM: Property and enum case additions follow the established pattern.
The new deviceStats property and its corresponding enum case in OneOf_Message are correctly implemented, following the same pattern as other message types in the struct.
Also applies to: 72-72
271-283: LGTM: Decoder and traversal implementations are consistent.
The new deviceStats case implementations in both decodeMessage and traverse methods follow the established pattern and maintain consistency with other message types.
Also applies to: 307-310
223-223: Verify the field number assignment.
The field number 4 is assigned to device_stats in the protobuf name map. Please ensure this field number doesn't conflict with any existing field numbers in the original .proto file.
✅ Verification successful
Field number 4 for device_stats is correctly assigned
Looking at the device/device.proto file, the field number 4 is properly assigned to device_stats in the DeviceToServerMessage message definition and doesn't conflict with any other field numbers within the same message. The field numbering is sequential (1 through 4) in the oneof Message block.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for field number 4 usage in proto files
rg -U "field.*=.*4.*;" --type proto
Length of output: 67
Script:
#!/bin/bash
# Search for .proto files first
fd -e proto
# Then search for field number assignments in those files
rg -U ".*=\s*4\s*;"
Length of output: 1450
Script:
#!/bin/bash
# Let's look at the specific proto file content to understand the context
cat device/device.proto
Length of output: 1711
common/messages.pb.swift (2)
385-431: LGTM! Protocol implementation is complete and efficient.
The SwiftProtobuf.Message implementation correctly handles all required aspects:
- Proper field number mapping
- Efficient decoding and traversal
- Complete equality comparison
133-156: LGTM! Verify timestamp handling across time zones.
The struct definition is well-structured with appropriate types and thread-safety considerations. The implementation properly handles the timestamp field with necessary helper methods.
Let's verify the timestamp handling implementation in other files:
✅ Verification successful
The previous search revealed that timestamps are consistently handled using Google_Protobuf_Timestamp across protobuf messages, but we need to verify how these timestamps are actually initialized and used in the application code. This will help us confirm if there are any timezone-related concerns.
Based on the code analysis results, I can now generate the final response:
Timestamp implementation is safe and timezone-agnostic
The timestamp implementation in Border0_Common_V1_DeviceStatsMessage is correct and safe because:
- It uses Google's Protobuf Timestamp which internally stores time as UTC seconds/nanoseconds since epoch
- The implementation is consistent with other timestamp usages across the codebase (connector.pb.swift)
- Protobuf's timestamp handling is inherently timezone-independent, eliminating timezone concerns
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check timestamp field usage across the codebase
# Look for timestamp initialization and handling patterns
# Search for timestamp usage patterns
rg -A 3 "Google_Protobuf_Timestamp" --type swift
# Search for potential timezone handling code
rg -A 3 "TimeZone|DateFormatter" --type swift
Length of output: 4060
Script:
#!/bin/bash
# Let's check for timestamp-related code in non-generated files
# First, find all Swift files excluding .pb.swift (protobuf generated)
fd -e swift -E "*.pb.swift" --exec rg -A 5 "timestamp|Timestamp|Date" {}
# Also check for any timestamp conversion or handling utilities
rg -A 5 "init.*Timestamp|Timestamp.*init" --type swift
Length of output: 371
adrianosela
left a comment
adrianosela
left a comment
There was a problem hiding this comment.
LGTM, but as discussed, would be good to add one deeper level e.g.
message Statistics {
oneof Message {
TxRxStats txrx_stats = 1;
UptimeStats uptime_stats = 2;
PeerHealthSnapshot peers_health_snapshot = 3;
}
}
in order to have handlers per type of statistics... (since they will be sent at different intervals depending on relevancy/etc).
common/messages.proto
Outdated
|
|
||
| message DeviceStatsMessage { | ||
| google.protobuf.Timestamp timestamp = 1; | ||
| int64 bytes_sent = 2; |
There was a problem hiding this comment.
Let's add packets off the bat.
waltzofpearls
force-pushed
the
ME-3787-device-stats
branch
from
a8c4c82 to
ac040db
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
common/messages.proto (1)
21-25: LGTM: Well-structured for future extensibilityThe StatsMessage is well-designed using oneof for extensibility. This will allow adding more stats types in the future without breaking changes.
Consider documenting potential future stat types that might be added to this oneof field to help guide future development.
common/messages.pb.swift (1)
172-178: Consider usingUInt64for count properties to prevent negative valuesThe properties
bytesIn,bytesOut,packetsIn, andpacketsOutrepresent counts that cannot be negative. UsingUInt64instead ofInt64would prevent negative values and accurately reflect these quantities.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
common/messages.pb.gois excluded by!**/*.pb.godevice/device.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (4)
common/messages.pb.swift(2 hunks)common/messages.proto(3 hunks)device/device.pb.swift(4 hunks)device/device.proto(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- device/device.proto
🔇 Additional comments (10)
common/messages.proto (2)
10-11: LGTM: Appropriate timestamp import
The import of google/protobuf/timestamp.proto is correctly placed and necessary for the network stats implementation.
27-33: LGTM: Comprehensive network stats implementation
The message includes all essential network interface metrics (bytes and packets for both in/out) with appropriate types. The previous review comment about including packets has been addressed.
Let's verify this matches the requirements by checking related files:
✅ Verification successful
LGTM: Network interface stats implementation is consistent and well-integrated
The verification confirms that:
- The NetworkInterfaceStatsMessage is properly defined in common/messages.proto
- The message is correctly integrated into the StatsMessage type
- The fields are consistently used across generated code (Go and Swift)
- No conflicting definitions exist in other proto files (connector.proto and device.proto)
- The implementation is referenced appropriately in the device management service for stats collection
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if these stats fields are consistently used across the codebase
rg -t proto "bytes_|packets_"
Length of output: 61
Script:
#!/bin/bash
# Search for any mentions of these network stats fields without file type restriction
rg "bytes_(in|out)|packets_(in|out)"
# Also search for proto files to ensure we're not missing anything
fd "\.proto$"
Length of output: 1156
Script:
#!/bin/bash
# Let's check the other proto files to ensure there are no conflicting or duplicate definitions
cat connector/connector.proto device/device.proto
# Also check for any network-related types or references
rg -i "network.*interface|interface.*stat"
Length of output: 11277
device/device.pb.swift (6)
58-64: LGTM: Stats property implementation follows the established pattern
The implementation of the stats property follows the same pattern as other message properties in the struct, maintaining consistency in the codebase.
72-72: LGTM: OneOf_Message enum case addition is correct
The new stats case is properly added to the OneOf_Message enum, maintaining the type safety of the message handling.
223-223: LGTM: Protobuf field mapping is correctly defined
The field mapping for stats uses the correct field number (4) and appropriate mapping strategy (.same), consistent with the protobuf schema.
271-283: LGTM: Message decoding logic is properly implemented
The decode logic for the stats field follows the established pattern:
- Proper handling of oneof conflicts
- Correct field number handling
- Consistent error handling
307-310: LGTM: Message traversal logic is correctly implemented
The traverse logic for the stats field is properly implemented, maintaining consistency with other message types.
Line range hint 1-1: Verify the protobuf generation version
Since this is a generated file, let's verify that it was generated with the correct version of protoc-gen-swift to ensure compatibility.
✅ Verification successful
Generated file uses Swift Protobuf API version 2
The file was generated with protoc-gen-swift and is using Swift Protobuf API version 2, which is the current stable version of the Swift Protobuf runtime. This is confirmed by the presence of the version check struct and its typealias to version 2.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for protoc and protoc-gen-swift version markers in the file
rg -A 5 "Generated by the Swift generator plugin|_GeneratedWithProtocGenSwiftVersion" device/device.pb.swift
Length of output: 564
common/messages.pb.swift (2)
133-156: LGTM!
The new struct Border0_Common_V1_StatsMessage is properly defined and integrates well with the existing codebase.
158-185: LGTM!
The struct Border0_Common_V1_NetworkInterfaceStatsMessage is correctly implemented with appropriate properties and initializations.
waltzofpearls
force-pushed
the
ME-3787-device-stats
branch
from
ac040db to
ccf39e0
Compare
waltzofpearls
force-pushed
the
ME-3787-device-stats
branch
from
ccf39e0 to
2c70a93
Compare
Summary by CodeRabbit
New Features
StatsMessageandNetworkDeviceStatsMessagefor enhanced network statistics reporting, including timestamp, bytes sent, and bytes received.DeviceToServerMessageto accommodate the newstatsfield for device statistics.Bug Fixes
statsfield.Documentation