Implement TracerClock and abstract away "now" #98
Conversation
|
Just need to get 5.6 done in the overloads maddness and we're done here 🥳 |
ktoso
force-pushed
the
wip-clock-no-time-like-the-present
branch
from
0746edd to
61cde0d
Compare
**Motivation:** we do not want to be bound to DispatchWallTime, but we cannot require Clock. We need to introduce a shim type TracerClockProtocol that in the future we'll be able to conform a "good Swift clock" to. This removes any dependencies on dispatch and makes the Time source swappable. We also implement our own unix timestamp clock.
ktoso
force-pushed
the
wip-clock-no-time-like-the-present
branch
from
8b813dc to
b869093
Compare
ktoso
force-pushed
the
wip-clock-no-time-like-the-present
branch
from
b869093 to
8b2be68
Compare
Sources/Tracing/SpanProtocol.swift
Outdated
| @@ -154,17 +148,28 @@ public struct SpanEvent: Equatable { | |||
| public var attributes: SpanAttributes | |||
|
|
|||
| /// The `DispatchWallTime` at which this event occurred. | |||
| public let time: DispatchWallTime | |||
| public let time: UInt64 | |||
There was a problem hiding this comment.
Can we add some docs what this timestamp is e.g. millis since Epoch? Interestingly enough should we make this opaque or not?
There was a problem hiding this comment.
Opaque is rather tricky what it would mean... Practically speaking we need timestamps, a tracer could decide it needs some specific way of those but by the end of the day it all ends up as ints I guess.
Making it all a type bound to Tracer is a huge mess type wise if we need to retain the 5.6 support :-\
| function: String = #function, | ||
| file fileID: String = #fileID, | ||
| line: UInt = #line, | ||
| _ operation: (any Span) async throws -> T |
There was a problem hiding this comment.
Aren't we missing a @_inheritActorContext and @Sendable annotation here. Otherwise this could runs into warnings right?
actor Foo {
var bar = 0
func foo() async {
await withSpan {
self.bar += 1
}
}
}
The downside of adding these annotations is that the story inside classes/structs is becoming worse because they don't have any actor context so the @Sendable constraint makes it impossible for them to access state even though the operation closure is run "inline".
In the end, we really need a productionized version of @_unsafeInheritExecutor. We are running into the same problem over in service-lifecycle.
There was a problem hiding this comment.
I checked and we don't get warnings for this 🤔
I think the function should NOT be sendable though
Sources/Tracing/TracingTime.swift
Outdated
| } | ||
|
|
||
| public protocol SwiftDistributedTracingInstantProtocol: Comparable, Hashable, Sendable { | ||
| // associatedtype Duration: SwiftDistributedTracingDurationProtocol |
Sources/Tracing/TracingTime.swift
Outdated
| public protocol TracerInstantProtocol: SwiftDistributedTracingInstantProtocol { | ||
| /// Representation of this instant as the number of milliseconds since UNIX Epoch (January 1st 1970) | ||
| @inlinable | ||
| var millisSinceEpoch: UInt64 { get } |
There was a problem hiding this comment.
Do we want to spell this out?
| var millisSinceEpoch: UInt64 { get } | |
| var millisecondsSinceEpoch: UInt64 { get } |
| let nowNanos = UInt64(ts.tv_sec) &* 1_000_000_000 &+ UInt64(ts.tv_nsec) | ||
| let nowMillis = UInt64(nowNanos / 1_000_000) // nanos to millis |
There was a problem hiding this comment.
I thought we were going to use a different method to calculate UTC time?
There was a problem hiding this comment.
Yeah I had a whole complicated dance which... ended up at the same number ¯_(ツ)_/¯
Discussed with @Lukasa and we're unsure why Foundation does the extreme dance there; This is good enough for us. And we could support nanos even but for now I didn't
|
Thanks for the review @FranzBusch I'm going to merge so we can give this a round of testing |
Motivation:
we do not want to be bound to DispatchWallTime, but we cannot require Clock. We need to introduce a shim type TracerClockProtocol that in the future we'll be able to conform a "good Swift clock" to.
This removes any dependencies on dispatch and makes the Time source swappable. We also implement our own unix timestamp clock.
The core of the change is:
resolves #71
WIll get to clean this up and make all versions work soon. At least locally confirmed swiftlang-5.7.0.123.8 worked fine