Skip to content

consider TracerSpan assoc type -> Span #111

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Apr 21, 2023
Merged

consider TracerSpan assoc type -> Span #111

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
aee8645
consider TracerSpan assoc type -> Span
ktoso Mar 30, 2023
09706c0
Merge branch 'main' into wip-tracer-rename-again
ktoso Apr 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions Sources/Tracing/NoOpTracer.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import Dispatch
/// Tracer that ignores all operations, used when no tracing is required.
@available(macOS 10.15, iOS 13, tvOS 13, watchOS 6, *) // for TaskLocal Baggage
public struct NoOpTracer: LegacyTracer {
public typealias TracerSpan = NoOpSpan
public typealias Span = NoOpSpan

public init() {}

Expand All @@ -29,7 +29,7 @@ public struct NoOpTracer: LegacyTracer {
at instant: @autoclosure () -> Instant,
function: String,
file fileID: String,
line: UInt) -> any Span
line: UInt) -> any Tracing.Span
{
NoOpSpan(baggage: baggage())
}
Expand All @@ -48,7 +48,7 @@ public struct NoOpTracer: LegacyTracer {
// no-op
}

public struct NoOpSpan: Span {
public struct NoOpSpan: Tracing.Span {
Copy link
Member Author

Choose a reason for hiding this comment

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

This is one annoying spot, people MUST write Tracing.Span here

Copy link
Contributor

Choose a reason for hiding this comment

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

What about swapping TracerSpan and Span?

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe, what do @tomerd @fabianfett @Lukasa think?

Copy link
Member

Choose a reason for hiding this comment

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

I don't find this to annoying. The tough part here is really only on the tracer implementers. TracerSpan is on all the users. If we want to keep the pain small, the first group should deal with it, not the second.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah it's on implementors to do the dance, I don't mind either I guess.

Copy link
Contributor

@stevapple stevapple Apr 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I would prefer TracerSpan to [Tracing.]Span here. Swift’s name lookup is still tricky now (see https://forums.swift.org/t/pitch-fully-qualified-name-syntax/28482 for the context). If the client happens to have a top-level Tracing type, it will completely lose access to Tracing.Span. Not to say TracerSpan is shorter by two ASCII characters:)

Copy link
Member

@fabianfett fabianfett Apr 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure I agree with the argument here. Using Tracing.Span is an escape hatch, when you define the Span type inside a Tracer. If you implement a Tracer you must import Tracing. If you then introduce a local type Tracing, you are screwed anyway. Rule number one of Swift namespaces: Don't name a type like a module you import.

If we look at it from the perspective of a tracing user, it is much nicer to write Span everywhere instead of using TracerSpan. The argument you make, that Swift namespace lookup sometimes isn't working correctly (by breaking Rule number one of Swift namespaces: Don't name a type like a module you import) does apply to Span as it does to TracerSpan.

Copy link
Contributor

@stevapple stevapple Apr 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

A typical tracing user doesn’t use either [Tracing.]Span or Tracer.Span, as the Tracer.Span type can be inferred from Tracer.startSpan.

Both Span and TracerSpan can cause some confusion with Tracer.Span, but the fact that Span is ambiguous, despite being possible to work around and comfort the compiler, will inevitably troubles code review.

Given that the type name is not used very often, I would still suggest the longer but less error-prone TracerSpan.

public let baggage: Baggage
public var isRecording: Bool {
false
Expand Down
18 changes: 9 additions & 9 deletions Sources/Tracing/TracerProtocol+Legacy.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ public protocol LegacyTracer: Instrument {
function: String,
file fileID: String,
line: UInt
) -> any Span
) -> any Tracing.Span

/// Export all ended spans to the configured backend that have not yet been exported.
///
Expand Down Expand Up @@ -108,7 +108,7 @@ extension LegacyTracer {
function: String = #function,
file fileID: String = #fileID,
line: UInt = #line
) -> any Span {
) -> any Tracing.Span {
self.startAnySpan(
operationName,
baggage: baggage(),
Expand Down Expand Up @@ -155,7 +155,7 @@ extension LegacyTracer {
function: String = #function,
file fileID: String = #fileID,
line: UInt = #line
) -> any Span {
) -> any Tracing.Span {
self.startAnySpan(
operationName,
baggage: baggage(),
Expand Down Expand Up @@ -200,7 +200,7 @@ extension LegacyTracer {
function: String = #function,
file fileID: String = #fileID,
line: UInt = #line,
_ operation: (any Span) throws -> T
_ operation: (any Tracing.Span) throws -> T
) rethrows -> T {
let span = self.startAnySpan(
operationName,
Expand Down Expand Up @@ -253,7 +253,7 @@ extension LegacyTracer {
function: String = #function,
file fileID: String = #fileID,
line: UInt = #line,
_ operation: (any Span) throws -> T
_ operation: (any Tracing.Span) throws -> T
) rethrows -> T {
try self.withAnySpan(
operationName,
Expand Down Expand Up @@ -301,7 +301,7 @@ extension LegacyTracer {
function: String = #function,
file fileID: String = #fileID,
line: UInt = #line,
_ operation: (any Span) async throws -> T
_ operation: (any Tracing.Span) async throws -> T
) async rethrows -> T {
let span = self.startAnySpan(
operationName,
Expand Down Expand Up @@ -354,7 +354,7 @@ extension LegacyTracer {
function: String = #function,
file fileID: String = #fileID,
line: UInt = #line,
_ operation: (any Span) async throws -> T
_ operation: (any Tracing.Span) async throws -> T
) async rethrows -> T {
let span = self.startAnySpan(
operationName,
Expand Down Expand Up @@ -469,7 +469,7 @@ extension Tracer {
function: String = #function,
file fileID: String = #fileID,
line: UInt = #line,
_ operation: (any Span) throws -> T
_ operation: (any Tracing.Span) throws -> T
) rethrows -> T {
try self.withSpan(
operationName,
Expand Down Expand Up @@ -524,7 +524,7 @@ extension Tracer {
function: String = #function,
file fileID: String = #fileID,
line: UInt = #line,
@_inheritActorContext @_implicitSelfCapture _ operation: (any Span) async throws -> T
@_inheritActorContext @_implicitSelfCapture _ operation: (any Tracing.Span) async throws -> T
) async rethrows -> T {
try await self.withSpan(
operationName,
Expand Down
14 changes: 7 additions & 7 deletions Sources/Tracing/TracerProtocol.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
@available(macOS 10.15, iOS 13, tvOS 13, watchOS 6, *) // for TaskLocal Baggage
public protocol Tracer: LegacyTracer {
/// The concrete type of span this tracer will be producing/
associatedtype TracerSpan: Span
associatedtype Span: Tracing.Span

/// Start a new ``Span`` with the given `Baggage`.
///
Expand Down Expand Up @@ -60,7 +60,7 @@ public protocol Tracer: LegacyTracer {
function: String,
file fileID: String,
line: UInt
) -> TracerSpan
) -> Self.Span
}

@available(macOS 10.15, iOS 13, tvOS 13, watchOS 6, *) // for TaskLocal Baggage
Expand Down Expand Up @@ -97,7 +97,7 @@ extension Tracer {
function: String = #function,
file fileID: String = #fileID,
line: UInt = #line
) -> TracerSpan {
) -> Self.Span {
self.startSpan(
operationName,
baggage: baggage(),
Expand Down Expand Up @@ -145,7 +145,7 @@ extension Tracer {
function: String = #function,
file fileID: String = #fileID,
line: UInt = #line,
_ operation: (TracerSpan) throws -> T
_ operation: (Self.Span) throws -> T
) rethrows -> T {
let span = self.startSpan(
operationName,
Expand Down Expand Up @@ -197,7 +197,7 @@ extension Tracer {
function: String = #function,
file fileID: String = #fileID,
line: UInt = #line,
_ operation: (TracerSpan) throws -> T
_ operation: (Self.Span) throws -> T
) rethrows -> T {
let span = self.startSpan(
operationName,
Expand Down Expand Up @@ -249,7 +249,7 @@ extension Tracer {
function: String = #function,
file fileID: String = #fileID,
line: UInt = #line,
@_inheritActorContext @_implicitSelfCapture _ operation: (TracerSpan) async throws -> T
@_inheritActorContext @_implicitSelfCapture _ operation: (Self.Span) async throws -> T
) async rethrows -> T {
let span = self.startSpan(
operationName,
Expand Down Expand Up @@ -302,7 +302,7 @@ extension Tracer {
function: String = #function,
file fileID: String = #fileID,
line: UInt = #line,
@_inheritActorContext @_implicitSelfCapture _ operation: (TracerSpan) async throws -> T
@_inheritActorContext @_implicitSelfCapture _ operation: (Self.Span) async throws -> T
) async rethrows -> T {
let span = self.startSpan(
operationName,
Expand Down
2 changes: 1 addition & 1 deletion Sources/_TracingBenchmarks/SpanAttributesDSLBenchmark.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ public let SpanAttributesDSLBenchmarks: [BenchmarkInfo] = [
),
]

private var span: (any Span)!
private var span: (any Tracing.Span)!

private func setUp() {
span = InstrumentationSystem.legacyTracer.startAnySpan("something", baggage: .topLevel)
Expand Down
6 changes: 3 additions & 3 deletions Tests/TracingTests/DynamicTracepointTracerTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ final class DynamicTracepointTestTracer: LegacyTracer {
}

private(set) var spans: [TracepointSpan] = []
var onEndSpan: (any Span) -> Void = { _ in
var onEndSpan: (any Tracing.Span) -> Void = { _ in
}

func startAnySpan<Instant: TracerInstant>(
Expand All @@ -173,7 +173,7 @@ final class DynamicTracepointTestTracer: LegacyTracer {
function: String,
file fileID: String,
line: UInt
) -> any Span {
) -> any Tracing.Span {
let tracepoint = TracepointID(function: function, fileID: fileID, line: line)
guard self.shouldRecord(tracepoint: tracepoint) else {
return TracepointSpan.notRecording(file: fileID, line: line)
Expand Down Expand Up @@ -331,7 +331,7 @@ extension DynamicTracepointTestTracer {

#if compiler(>=5.7.0)
extension DynamicTracepointTestTracer: Tracer {
typealias TracerSpan = TracepointSpan
typealias Span = TracepointSpan

func startSpan<Instant: TracerInstant>(_ operationName: String,
baggage: @autoclosure () -> Baggage,
Expand Down
2 changes: 1 addition & 1 deletion Tests/TracingTests/TestTracer.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ final class TestTracer: LegacyTracer {
function: String,
file fileID: String,
line: UInt
) -> any Span {
) -> any Tracing.Span {
let span = TestSpan(
operationName: operationName,
startTime: instant(),
Expand Down
2 changes: 1 addition & 1 deletion Tests/TracingTests/TracedLock.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ final class TracedLock {
let name: String
let underlyingLock: NSLock

var activeSpan: (any Span)?
var activeSpan: (any Tracing.Span)?

init(name: String) {
self.name = name
Expand Down
4 changes: 2 additions & 2 deletions Tests/TracingTests/TracedLockTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ private final class TracedLockPrintlnTracer: LegacyTracer {
function: String,
file fileID: String,
line: UInt
) -> any Span {
) -> any Tracing.Span {
TracedLockPrintlnSpan(
operationName: operationName,
startTime: instant(),
Expand Down Expand Up @@ -97,7 +97,7 @@ private final class TracedLockPrintlnTracer: LegacyTracer {
Extract: Extractor,
Carrier == Extract.Carrier {}

final class TracedLockPrintlnSpan: Span {
final class TracedLockPrintlnSpan: Tracing.Span {
private let kind: SpanKind

private var status: SpanStatus?
Expand Down
20 changes: 10 additions & 10 deletions Tests/TracingTests/TracerTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,11 +123,11 @@ final class TracerTests: XCTestCase {
var spanEnded = false
tracer.onEndSpan = { _ in spanEnded = true }

func operation(span: any Span) -> String {
func operation(span: any Tracing.Span) -> String {
"world"
}

let value = tracer.withAnySpan("hello") { (span: any Span) -> String in
let value = tracer.withAnySpan("hello") { (span: any Tracing.Span) -> String in
XCTAssertEqual(span.baggage.traceID, Baggage.current?.traceID)
return operation(span: span)
}
Expand All @@ -152,7 +152,7 @@ final class TracerTests: XCTestCase {
var spanEnded = false
tracer.onEndSpan = { _ in spanEnded = true }

func operation(span: any Span) throws -> String {
func operation(span: any Tracing.Span) throws -> String {
throw ExampleSpanError()
}

Expand Down Expand Up @@ -182,12 +182,12 @@ final class TracerTests: XCTestCase {
var spanEnded = false
tracer.onEndSpan = { _ in spanEnded = true }

func operation(span: any Span) async throws -> String {
func operation(span: any Tracing.Span) async throws -> String {
"world"
}

try self.testAsync {
let value = try await tracer.withAnySpan("hello") { (span: any Span) -> String in
let value = try await tracer.withAnySpan("hello") { (span: any Tracing.Span) -> String in
XCTAssertEqual(span.baggage.traceID, Baggage.current?.traceID)
return try await operation(span: span)
}
Expand All @@ -212,14 +212,14 @@ final class TracerTests: XCTestCase {
var spanEnded = false
tracer.onEndSpan = { _ in spanEnded = true }

func operation(span: any Span) async -> String {
func operation(span: any Tracing.Span) async -> String {
"world"
}

self.testAsync {
var fromNonAsyncWorld = Baggage.topLevel
fromNonAsyncWorld.traceID = "1234-5678"
let value = await tracer.withAnySpan("hello", baggage: fromNonAsyncWorld) { (span: any Span) -> String in
let value = await tracer.withAnySpan("hello", baggage: fromNonAsyncWorld) { (span: any Tracing.Span) -> String in
XCTAssertEqual(span.baggage.traceID, Baggage.current?.traceID)
XCTAssertEqual(span.baggage.traceID, fromNonAsyncWorld.traceID)
return await operation(span: span)
Expand All @@ -244,7 +244,7 @@ final class TracerTests: XCTestCase {
var spanEnded = false
tracer.onEndSpan = { _ in spanEnded = true }

func operation(span: any Span) async throws -> String {
func operation(span: any Tracing.Span) async throws -> String {
throw ExampleSpanError()
}

Expand Down Expand Up @@ -274,7 +274,7 @@ final class TracerTests: XCTestCase {
var spanEnded = false
tracer.onEndSpan = { _ in spanEnded = true }

func operation(span: any Span) async throws -> String {
func operation(span: any Tracing.Span) async throws -> String {
throw ExampleSpanError()
}

Expand Down Expand Up @@ -304,7 +304,7 @@ final class TracerTests: XCTestCase {
var spanEnded = false
tracer.onEndSpan = { _ in spanEnded = true }

func operation(span: any Span) throws -> String {
func operation(span: any Tracing.Span) throws -> String {
throw ExampleSpanError()
}

Expand Down