-
Notifications
You must be signed in to change notification settings - Fork 48
Add TestTracer to new TestTracingKit #180
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
Conversation
17f4c8d
to
7df3df8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great idea! Overall can we add public API docs and some docs into our DocC catalog for this please?
public struct TestSpan: Span { | ||
public let context: ServiceContext | ||
public let spanContext: TestSpanContext | ||
public let kind: SpanKind | ||
public let startInstant: any TracerInstant |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add some doc comments to this? Also can all of these properties be var
s instead?
self.onEnd = onEnd | ||
} | ||
|
||
public var isRecording: Bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here. Can we add doc comments to all of those please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These have docs on Span
already so not sure if that's necessary here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then we could add // swift-format-ignore: AllPublicDeclarationsHaveDocumentation
and enable AllPublicDeclarationsHaveDocumentation
in the swift-format config file. I'd also like to see all public APIs having comments or opt out this way if they inherit them.
That said - probably worth doing in a separate PR.
Filed #182
} | ||
} | ||
|
||
public struct FinishedTestSpan: Sendable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we move this into a new file, add doc comments and make all the properties var
s instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure this needs new file but +1 on the var, there's no reason not to :)
@_spi(Locking) import Instrumentation | ||
import Testing | ||
import Tracing | ||
@testable import TracingTestKit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need a testable import here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to add a test that uses the global withSpan
APIs and the test tracer?
Thanks for the review. Yes, I'll add docs next but wanted to get some general feedback on the API first. |
self.onEnd = onEnd | ||
} | ||
|
||
public var isRecording: Bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These have docs on Span
already so not sure if that's necessary here
} | ||
} | ||
|
||
public struct FinishedTestSpan: Sendable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure this needs new file but +1 on the var, there's no reason not to :)
@_spi(Locking) import Instrumentation | ||
import Tracing | ||
|
||
public struct TestTracer: Tracer { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The implementation is fine, but we should call it some InMemoryStorageTracer
, and rename the module as well perhaps. This isn't strictly required for tests but just accumulates in memory; you could imagine some silly use-cases that would want to use this and I don't necessarily want to taint them with the Test...
word, WDYT?
errors: errors, | ||
status: status | ||
) | ||
_isRecording.withValue { $0 = false } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is racy with the assertIsRecording; you have to check + swap to false at the same time as asserting, otherwise you can get two tasks end()-ing
they'll both pass the assert and then both try to set -- but we would have double-ended.
@@ -6,6 +6,7 @@ let package = Package( | |||
products: [ | |||
.library(name: "Instrumentation", targets: ["Instrumentation"]), | |||
.library(name: "Tracing", targets: ["Tracing"]), | |||
.library(name: "TracingTestKit", targets: ["TracingTestKit"]), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should call this InMemoryTracing maybe, and not have it be strictly called "just for testing" there may be various use cases to use this
We discussed this one a bit more -- just pending a rename and the cleanups and happy to merge afterwards! |
Co-authored-by: Moritz Lang <[email protected]>
Motivation
Library authors and end users integrating Swift Distributed Tracing need some form of test tracer implementation that they can use in unit tests. Up until now, Swift Distributed Tracing didn't provide a built-in solution to this problem, leading to
TestTracer
being redefined in many libraries and presumably some end-user applications. Most of these implementations share the common need of inspecting spans and context propagation to verify their code behaves as expected so they end up being extremely similar or straight up duplicates.Therefore, I'd like to create a first-party
TestTracer
in a newTracingTestKit
library. The nameTracingTestKit
aligns withMetricsTestKit
from swift-metrics.Modifications
TracingTestKit
libraryTestTracer
toTracingTestKit
to allow inspection of spans in unit testsRemaining tasks
TracingTestKit
Result
Library users can now extensively test the parts of their code which make use of Swift Distributed Tracing without needing to build their own
TestTracer
type. Instead, they can now depend onTracingTestKit
.Future Direction
Using
TestTracer
inTracingTests
To keep this pull request focused on providing a fully unit-tested
TestTracer
implementation I opted not to use the newTestTracer
in the existingTracingTests
. IMHO, this can come in a follow-up PR.Task-local
TestTracer
@adam-fowler built a great task-local tracer for Hummingbird which allows parallel test execution while still using the global
InstrumentationSystem
: https://github.com/hummingbird-project/hummingbird/blob/3ae359b1bb1e72378ed43b59fdcd4d44cac5d7a4/Tests/HummingbirdTests/TestTracer.swift#L184Now that we'd have a built-in
TestTracer
it'd be great to also add this toTracingTestKit
.