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

natikgadzhi · 2023-06-23T03:54:41Z

This pull request adds a convenience API to Timer that uses Swift 5.7's Duration.

Motivation:

I'm looking for ways to learn Swift, and it's open source ecosystem by doing, and this looked like a beginner-friendly little addition that @ktoso requested (well, a year ago) in #114.
Closes #114.

Modifications:

Timer.record(_ duration: Duration) that's available on iOS16 and friends.
A unit test
Added the test case to Linux tests
Soundness check passes.
Record duration in nanoseconds.

Where to start the review

My @available check looks weird, I thought Duration is available in Swift 5.7, but looks like it's platform-specific. Do they look okay?
Looks like DocC should pick up the function variant automatically, but I'm happy to add example / other documentation as needed.
Duration conversion to seconds / nanoseconds is lossy and looks a bit janky, but seems like that's how folks are doing it for now?

Motivation: `Duration` is available starting Swift 5.7, and we should probably support it in our convinience API. Modifications: This commit adds: - Timer.record(_ duration: Duration) implementation - A unit test case - Generated Linux tests

Sources/Metrics/Metrics.swift

Tests/MetricsTests/MetricsTests.swift

ktoso · 2023-06-23T04:07:36Z

@swift-server-bot add to allowlist

natikgadzhi · 2023-06-23T04:37:19Z

I see I broke the formatting, one sec, will fix.

natikgadzhi · 2023-06-23T04:41:34Z

@ktoso, fixed the formatting issues in tests, but swiftformat with default config has a few more complaints. Won't add them to this PR to keep this isolated.

How does that look?

ktoso · 2023-06-23T05:41:33Z

Sources/Metrics/Metrics.swift

+        // `Duration` doesn't have a nice way to convert it back to `Double` of seconds,
+        // so instead we can multiply attoseconds by 1e18 and add the number of seconds to it.
+        let durationSeconds = Double(duration.components.seconds) + Double(duration.components.attoseconds) / 1e18
+        self.recordSeconds(durationSeconds)


Actually, this is too lossy, we have recordNanoseconds that this should call into, as it's the most precision we have. With just seconds this would not be very useful for fast operations

Sources/Metrics/Metrics.swift

ktoso · 2023-06-23T05:44:03Z

Minor nitpick still, looking good though

natikgadzhi · 2023-06-24T03:36:31Z

@ktoso thank you for the review! Cleaned up, added more comments, recording in nanoseconds now.

tomerd

nice. thanks @natikgadzhi

tomerd · 2023-06-26T17:26:59Z

Sources/Metrics/Metrics.swift

+        // `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 durationNanoseconds = duration.components.seconds * 1_000_000_000 + duration.components.attoseconds / 1_000_000_000


can duration.components.attoseconds ever be zero?

should we be using multipliedReportingOverflow and dividedReportingOverflow to address overflow risk?

can duration.components.attoseconds ever be zero?
Unlikely, but they can. That shouldn't be a problem, though? I think it's Int64 divided by Int64 that is not zero, so it'll just be zero?

should we be using multipliedReportingOverflow and dividedReportingOverflow to address overflow risk?
TIL! I was suspicious about overflows, thank you for pointing out the direction. I'll take a look and use those, happy to address in this or a new PR.

I'll take a look and use those, happy to address in this or a new PR.

lets add it here to make sure the code is safe

@tomerd, here's an attempt: https://github.com/apple/swift-metrics/pull/133/files#diff-19a0ef33862d9079451ca7ba346b734175c62c5ad95fb30d565a41f9ea32a770R95

dividedReportingOverflow should not be applicable in here, since we know we're not dividing by zero.

multipliedReportingOverflow and addingReportingOverflow will return a Bool flag on error. Not a very common way of error handling in Swift, but that's cheaper than making a new Error and throwing. So, I considered keeping a similar API, and perhaps providing a convenience func that would be throwing, but decided throws provides a more familiar and simple API.

Added a test to keep all code paths tested and verify it won't fatal out.

What I don't know is whether there's a performance hit we take for marking the function as throwing, in situations where we are not actually throwing errors. I think it's negligible?

tomerd · 2023-06-27T17:47:09Z

Sources/Metrics/Metrics.swift

+    /// - 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 {


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

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.

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) } }

@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?

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.

tomerd · 2023-06-28T20:44:02Z

Sources/Metrics/Metrics.swift

+
+#if swift(>=5.7)
+
+public enum TimerError: Error {


can be removed now?

tomerd · 2023-06-28T20:45:19Z

Tests/MetricsTests/MetricsTests.swift

+        // 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


use XCTSkip or XCTSkipIf where we are skipping the test?

tomerd · 2023-06-28T20:46:29Z

looks good, a couple of nits and we should merge this!

tomerd · 2023-06-28T20:57:27Z

Tests/MetricsTests/MetricsTests.swift

@@ -101,7 +101,7 @@ class MetricsExtensionsTests: XCTestCase {
        // 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
+            throw XCTSkip("Timer.record(_ duration: Duration) is available on Swift 5.7+")


do the same for earlier versions of Swift? this one is for the availability.

Tried that — #available doesn't like to be treated as an expression and only works with guard and if, seemingly. Didn't try for the language check, though. Let me see if that works.

UPD: I think wrapping the check in a closure before feeding it to XCSkipIf works. Might look ugly, trying it out now.

adding an #else does not work?

try! XCTSkipIf({ #if swift(<5.7) return true #else if #available(iOS 16, macOS 13, tvOS 15, watchOS 8, *) { return false } else { return true } #endif }(), "Timer.record(_ duration: Duration) is available on Swift 5.7+")

That works! But mixing #if and regular if looks weird to me. Perhaps two separate for #available and #if swift are okay?

#if swift(<5.7) throw XCTSkip("Timer.record(_ duration: Duration) is not available on this version of Swift") #endif guard #available(iOS 16, macOS 13, tvOS 15, watchOS 8, *) else { throw XCTSkip("Timer.record(_ duration: Duration) is not available on this version of Swift") }

Looks more readable? The closure with nested ifs and explicit returns seems too verbose.

Should I stick with two separate checks?

I think we will have to #if swift(>=5.7) around the entire body of the test as done now since it used types that are not available in previous versions (build will fail) but we can add an #else to throw XCTSkip there

note tested, but...

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 { throw XCTSkip("Timer.record(_ duration: Duration) is available on Swift 5.7+") } 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 nanoseconds = duration.components.seconds * 1_000_000_000 + duration.components.attoseconds / 1_000_000_000 timer.record(duration) // Record a Duration that would overflow, // expect Int64.max to be recorded. timer.record(Duration(secondsComponent: 10_000_000_000, attosecondsComponent: 123)) let testTimer = try metrics.expectTimer(timer) XCTAssertEqual(testTimer.values.count, 2, "expected number of entries to match") XCTAssertEqual(testTimer.values.first, nanoseconds, "expected value to match") XCTAssertEqual(testTimer.values[1], Int64.max, "expected to record Int64.max if Durataion overflows") XCTAssertEqual(metrics.timers.count, 1, "timer should have been stored") #else throw XCTSkip("Timer.record(_ duration: Duration) is available on Swift 5.7+") #endif }

Alright! Pushed up.

UPD wait a sec, broke CI. XCTSkip is not available on Swift 5.0 and 5.1 🤦🏼

Okay,

#if swift(>=5.7) // ... #elseif swift(>=5.2) throw XCTSkip("Timer.record(_ duration: Duration) is only available on Swift >=5.7") #endif

I think that'd be good enough, Swift 5.1 is not that common anymore anyway?

…5.2+

tomerd · 2023-06-28T21:57:04Z

thanks @natikgadzhi

natikgadzhi · 2023-06-28T22:04:11Z

@tomerd, thank you for your time and patience 🙃

ktoso · 2023-06-29T22:43:47Z

Very nice work, late LGTM :)

Adding Timer.record(_ duration: Duration)

37ae7aa

Motivation: `Duration` is available starting Swift 5.7, and we should probably support it in our convinience API. Modifications: This commit adds: - Timer.record(_ duration: Duration) implementation - A unit test case - Generated Linux tests

natikgadzhi commented Jun 23, 2023

View reviewed changes

Sources/Metrics/Metrics.swift Outdated Show resolved Hide resolved

ktoso reviewed Jun 23, 2023

View reviewed changes

Tests/MetricsTests/MetricsTests.swift Outdated Show resolved Hide resolved

Wrap Duration code in Swift 5.7 checks

e0ce4e8

Fix formatting in tests

d0bd9aa

natikgadzhi requested a review from ktoso June 23, 2023 04:41

scripts/sounness check if swiftformat is installed

9cccde4

natikgadzhi mentioned this pull request Jun 23, 2023

Check if swiftformat is installed in scripts/soundness.sh #134

Closed

ktoso reviewed Jun 23, 2023

View reviewed changes

Sources/Metrics/Metrics.swift Show resolved Hide resolved

AndriusA mentioned this pull request Jun 23, 2023

adds support for swift-concurrency Duration and async closures for Timer measurements #135

Open

Review feedback: recordin in nanoseconds

b019460

natikgadzhi requested a review from ktoso June 24, 2023 03:31

tomerd approved these changes Jun 26, 2023

View reviewed changes

tomerd reviewed Jun 26, 2023

View reviewed changes

Timer.record(_ duration:): protect from overflows

02fc732

tomerd reviewed Jun 27, 2023

View reviewed changes

Make Timer.record(_ duration: Duration) record Int64.max on overflow

f8316f2

tomerd reviewed Jun 28, 2023

View reviewed changes

Timer.record(_ duration:) review feedback cleanup

9b835a7

tomerd reviewed Jun 28, 2023

View reviewed changes

natikgadzhi added 2 commits June 28, 2023 14:39

Timer.record(_ duration:) review feedback: XCTSkip on Swift version

fd5b5d2

Timer.record(_ duration:) review feedback: formatting

84db69d

Timer.record(_ duration:) review feedback: only use XCTSkip on Swift …

99ec049

…5.2+

tomerd approved these changes Jun 28, 2023

View reviewed changes

tomerd merged commit 13ea1fe into apple:main Jun 28, 2023

ktoso added this to the 2.5.0 milestone Jun 29, 2023

hamzahrmalik mentioned this pull request May 14, 2024

Change Timer.record(duration: Duration) signature to avoid source breakage #144

Merged

ktoso modified the milestones: 2.5.0, 2.4.2 May 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

natikgadzhi commented Jun 23, 2023 •

edited

Loading

ktoso commented Jun 23, 2023

natikgadzhi commented Jun 23, 2023

natikgadzhi commented Jun 23, 2023

ktoso Jun 23, 2023

ktoso commented Jun 23, 2023

natikgadzhi commented Jun 24, 2023

tomerd left a comment

tomerd Jun 26, 2023

tomerd Jun 26, 2023

natikgadzhi Jun 26, 2023

tomerd Jun 27, 2023

natikgadzhi Jun 27, 2023 •

edited

Loading

tomerd Jun 27, 2023 •

edited

Loading

natikgadzhi Jun 27, 2023

tomerd Jun 27, 2023

natikgadzhi Jun 28, 2023

natikgadzhi Jun 28, 2023

tomerd Jun 28, 2023

tomerd Jun 28, 2023 •

edited

Loading

natikgadzhi Jun 28, 2023

tomerd commented Jun 28, 2023

tomerd Jun 28, 2023 •

edited

Loading

natikgadzhi Jun 28, 2023 •

edited

Loading

tomerd Jun 28, 2023

natikgadzhi Jun 28, 2023

tomerd Jun 28, 2023

tomerd Jun 28, 2023

natikgadzhi Jun 28, 2023 •

edited

Loading

natikgadzhi Jun 28, 2023

tomerd commented Jun 28, 2023

natikgadzhi commented Jun 28, 2023

ktoso commented Jun 29, 2023

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

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

Conversation

natikgadzhi commented Jun 23, 2023 • edited Loading

Motivation:

Modifications:

Where to start the review

ktoso commented Jun 23, 2023

natikgadzhi commented Jun 23, 2023

natikgadzhi commented Jun 23, 2023

Choose a reason for hiding this comment

ktoso commented Jun 23, 2023

natikgadzhi commented Jun 24, 2023

tomerd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natikgadzhi Jun 27, 2023 • edited Loading

Choose a reason for hiding this comment

tomerd Jun 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomerd Jun 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomerd commented Jun 28, 2023

tomerd Jun 28, 2023 • edited Loading

Choose a reason for hiding this comment

natikgadzhi Jun 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natikgadzhi Jun 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomerd commented Jun 28, 2023

natikgadzhi commented Jun 28, 2023

ktoso commented Jun 29, 2023

natikgadzhi commented Jun 23, 2023 •

edited

Loading

natikgadzhi Jun 27, 2023 •

edited

Loading

tomerd Jun 27, 2023 •

edited

Loading

tomerd Jun 28, 2023 •

edited

Loading

tomerd Jun 28, 2023 •

edited

Loading

natikgadzhi Jun 28, 2023 •

edited

Loading

natikgadzhi Jun 28, 2023 •

edited

Loading