Server Stats Collection #1834
Server Stats Collection #1834
Conversation
Motivation: In order to implement the 'runServer' RPC on the WorkerService we need to capture a snapshot with the resorces usage for the server and compute the difference between 2 of these snapshots. Modifications: - Created the ServerStats struct that contains all stats needed in the QPS benchmarking from the server - Implemented the init() that collects all these stats - Implemented the function that computes the differences between 2 snapshots of ServerStats Result: We will be able to implement the `runServer` RPC.
| #if os(macOS) || os(iOS) || os(watchOS) || os(tvOS) | ||
| import Darwin | ||
| #elseif os(Linux) || os(FreeBSD) || os(Android) | ||
| import Glibc | ||
| #else | ||
| let badOS = { fatalError("unsupported OS") }() | ||
| #endif |
There was a problem hiding this comment.
We generally try to use canImport now as it's a bit more flexible (e.g. visionOS) and musl is now being used instead of Glibc:
#if canImport(Darwin)
import Darwin
#elseif canImport(Glibc)
import Glibc
#elseif canImport(Musl)
import Musl
#else
// ...
#endif| #if os(macOS) || os(iOS) || os(watchOS) || os(tvOS) | ||
| private let OUR_RUSAGE_SELF: Int32 = RUSAGE_SELF | ||
| #elseif os(Linux) || os(FreeBSD) || os(Android) | ||
| private let OUR_RUSAGE_SELF: Int32 = RUSAGE_SELF.rawValue | ||
| #endif |
There was a problem hiding this comment.
Same here, we should use canImport as the guard
| let time: Double | ||
| let userTime: Double | ||
| let systemTime: Double | ||
| let totalCpuTime: UInt64 | ||
| let idleCpuTime: UInt64 |
There was a problem hiding this comment.
These should all be var, also Cpu should be CPU
| totalCpuTime: UInt64, | ||
| idleCPuTime: UInt64 |
| internal func changeInStats(initialStats: ServerStats, currentStats: ServerStats) -> ServerStats { | ||
| return ServerStats( | ||
| time: currentStats.time - initialStats.time, | ||
| userTime: currentStats.userTime - initialStats.userTime, | ||
| systemTime: currentStats.systemTime - initialStats.systemTime, | ||
| totalCpuTime: currentStats.totalCpuTime - initialStats.totalCpuTime, | ||
| idleCPuTime: currentStats.idleCpuTime - initialStats.idleCpuTime | ||
| ) | ||
| } |
There was a problem hiding this comment.
I think it'd make more sense to have this as a function on ServerStats instead, e.g. func difference(to stats: ServerStats) -> ServerStats
| let contents = try await ByteBuffer( | ||
| contentsOf: "/proc/stat", | ||
| maximumSizeAllowed: .kilobytes(20) | ||
| ) |
There was a problem hiding this comment.
This is the only part that throws so we should minimise the do-catch to this
| maximumSizeAllowed: .kilobytes(20) | ||
| ) | ||
|
|
||
| guard let index = contents.readableBytesView.firstIndex(where: { $0 == UInt8("\n") }) else { |
There was a problem hiding this comment.
You can use firstIndex(of: UInt8(ascii: "\n")) instead of firstIndex(where:) in this case.
| return (0, 0) | ||
| } | ||
|
|
||
| guard let firstLine = contents.getString(at: 0, length: index) else { |
There was a problem hiding this comment.
This isn't quite right because the buffer isn't necessarily zero-indexed. The view has a start index so you could use that and then compute the right length, however, the preferred way is to create a slice of the buffer view, then turn that back into a buffer and then convert it to a String:
let view = contents.readableBytesView
guard let index = view.firstIndex(of: UInt8(ascii: "\n")) else {
return (0, 0)
}
let line = String(buffer: ByteBuffer(view))| /// The totalCpuTime is computed as follows: | ||
| /// total = user + nice + system + idle | ||
| @available(macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0, *) | ||
| private func getTotalAndIdleCpuTime() async throws -> (UInt64, UInt64) { |
There was a problem hiding this comment.
What is each value in the return, can you add labels to the tuple?
| /// The totalCpuTime is computed as follows: | ||
| /// total = user + nice + system + idle | ||
| @available(macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0, *) | ||
| private func getTotalAndIdleCpuTime() async throws -> (UInt64, UInt64) { |
There was a problem hiding this comment.
Generally avoid global functions, this can be a private static func on ServerStats instead
glbrntt
left a comment
glbrntt
left a comment
There was a problem hiding this comment.
Two small things but looks great otherwise
| self.userTime = 0 | ||
| self.systemTime = 0 | ||
| } | ||
| if #available(macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0, *) { |
There was a problem hiding this comment.
Can we bubble up this availability and just put it on the type definition? These guards are required for Swift concurrency but the whole performance worker needs that, so we can just bubble it up to the top-level.
| /// CPU [user] [nice] [system] [idle] [iowait] [irq] [softirq] | ||
| /// The totalCPUTime is computed as follows: | ||
| /// total = user + nice + system + idle | ||
| @available(macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0, *) |
There was a problem hiding this comment.
We can remove this is the availability goes on the type
gjcairo
added
semver/none
3 participants
Motivation:
In order to implement the 'runServer' RPC on the WorkerService we need to capture a snapshot with the resorces usage for the server and compute the difference between 2 of these snapshots.
Modifications:
Result:
We will be able to implement the
runServerRPC.