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

Introducing Timer.record(_ duration:), closes #114 #133

Merged
merged 11 commits into from
Jun 28, 2023
36 changes: 36 additions & 0 deletions Sources/Metrics/Metrics.swift
Original file line number Diff line number Diff line change
Expand Up @@ -74,3 +74,39 @@ extension Timer {
}
}
}

#if swift(>=5.7)

public enum TimerError: Error {
Copy link
Member

Choose a reason for hiding this comment

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

can be removed now?

case durationToIntOverflow
}

extension Timer {
/// Convenience for recording a duration based on ``Duration``.
///
natikgadzhi marked this conversation as resolved.
Show resolved Hide resolved
/// `Duration` will be converted to an `Int64` number of nanoseconds, and then recorded with nanosecond precision.
///
/// - Parameters:
/// - duration: The `Duration` to record.
///
/// - Throws: `TimerError.durationToIntOverflow` if conversion from `Duration` to `Int64` of Nanoseconds overflowed.
@available(macOS 13, iOS 16, tvOS 15, watchOS 8, *)
@inlinable
public func record(_ duration: Duration) throws {
Copy link
Member

@tomerd tomerd Jun 27, 2023

Choose a reason for hiding this comment

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

I am not convinced we want these to throw as it will make the call sites awkward to use. I think we should record Int64.max on overflow as we do in recordNanoseconds

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can do that! The way I understand the trade-off:

  • Having an error out on overflow is useful in a very rare cases that I can't quite imagine.
  • Typing try! all the time and explaining to your team why it's okay is not ideomatic.
  • When an overflow does happen, it would mean that the duration is VERY big, which should never happen in a Timer metric. When it does, we can choose to error out (not quite useful), crash (bad), or just record a really, really long duration, even if it does not match an actual reading. Because that actual reading is very likely wrong anyway.

Do I understand the problem correctly?

I'll clean up a code in a bit when I have a minute.

Copy link
Member

Choose a reason for hiding this comment

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

exactly. we already take that approach in recordXXX, eg


    @inlinable
    public func recordNanoseconds<DataType: BinaryInteger>(_ duration: DataType) {
        self.recordNanoseconds(duration >= Int64.max ? Int64.max : Int64(duration))
    }

    @inlinable
    public func recordSeconds<DataType: BinaryInteger>(_ duration: DataType) {
        guard duration <= Int64.max else { return self.recordNanoseconds(Int64.max) }

        let result = Int64(duration).multipliedReportingOverflow(by: 1_000_000_000)
        if result.overflow {
            self.recordNanoseconds(Int64.max)
        } else {
            self.recordNanoseconds(result.partialValue)
        }
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tomerd, pushed up a change.

Performing overflow checks one at a time to not perform the second operation if the first already overflowed.

I don't yet know the separation into CoreMetrics and plain Metrics directories. How do you split them? My gut feeling was that it's about cross-platform support, band CoreMetrics is designed to work everywhere where, only relies on standard library, but Metrics uses Foundation as well?

Is it important where that Timer extension lives?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aside: I know an engineer at Apple could put that PR and merge it in under an hour, and I appreciate your patience and guidance, @tomerd.

// `Duration` doesn't have a nice way to convert it nanoseconds or seconds,
// so we'll do the multiplication manually.
// Nanoseconds are the smallest unit Timer can track, so we'll record in that.
let (seconds, overflow) = duration.components.seconds.multipliedReportingOverflow(by: 1_000_000_000)
guard !overflow else {
throw TimerError.durationToIntOverflow
}

let (nanoseconds, attosecondsOverflow) = seconds.addingReportingOverflow(duration.components.attoseconds / 1_000_000_000)
guard !attosecondsOverflow else {
throw TimerError.durationToIntOverflow
}

self.recordNanoseconds(nanoseconds)
}
}
#endif
1 change: 1 addition & 0 deletions Tests/MetricsTests/MetricsTests+XCTest.swift
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ extension MetricsExtensionsTests {
("testTimerWithTimeInterval", testTimerWithTimeInterval),
("testTimerWithDispatchTime", testTimerWithDispatchTime),
("testTimerWithDispatchTimeInterval", testTimerWithDispatchTimeInterval),
("testTimerDuration", testTimerDuration),
("testTimerUnits", testTimerUnits),
("testPreferDisplayUnit", testPreferDisplayUnit),
]
Expand Down
29 changes: 29 additions & 0 deletions Tests/MetricsTests/MetricsTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,35 @@ class MetricsExtensionsTests: XCTestCase {
XCTAssertEqual(metrics.timers.count, 1, "timer should have been stored")
}

func testTimerDuration() throws {
// Wrapping only the insides of the test case so that the generated
// tests on Linux in MetricsTests+XCTest don't complain that the func does not exist.
#if swift(>=5.7)
guard #available(iOS 16, macOS 13, tvOS 15, watchOS 8, *) else {
return
Copy link
Member

@tomerd tomerd Jun 28, 2023

Choose a reason for hiding this comment

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

use XCTSkip or XCTSkipIf where we are skipping the test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TIL.

}

let metrics = TestMetrics()
MetricsSystem.bootstrapInternal(metrics)

let name = "timer-\(UUID().uuidString)"
let timer = Timer(label: name)

let duration = Duration(secondsComponent: 3, attosecondsComponent: 123_000_000_000_000_000)
let durationInNanoseconds = duration.components.seconds * 1_000_000_000 + duration.components.attoseconds / 1_000_000_000

XCTAssertNoThrow(try timer.record(duration))

let testTimer = try metrics.expectTimer(timer)
XCTAssertEqual(testTimer.values.count, 1, "expected number of entries to match")
XCTAssertEqual(testTimer.values.first, durationInNanoseconds, "expected value to match")
XCTAssertEqual(metrics.timers.count, 1, "timer should have been stored")

let overflowDuration = Duration(secondsComponent: 10_000_000_000, attosecondsComponent: 123)
XCTAssertThrowsError(try timer.record(overflowDuration))
#endif
}

func testTimerUnits() throws {
let metrics = TestMetrics()
MetricsSystem.bootstrapInternal(metrics)
Expand Down
6 changes: 6 additions & 0 deletions scripts/soundness.sh
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,12 @@ fi
printf "\033[0;32mokay.\033[0m\n"

printf "=> Checking format... "

if [[ ! -x $(which swiftformat) ]]; then
printf "\033[0;31mswiftformat not found!\033[0m\n"
exit 1
fi

FIRST_OUT="$(git status --porcelain)"
swiftformat . > /dev/null 2>&1
SECOND_OUT="$(git status --porcelain)"
Expand Down