-
Notifications
You must be signed in to change notification settings - Fork 50
Pushing InMemoryTracer (#180) over the finish line #183
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
|
||
/// Retrives a specific _active_ span, identified by the specific span, trace, and parent ID's | ||
/// stored in the `inMemorySpanContext` | ||
public func activeSpan(identifiedBy context: ServiceContext) -> InMemorySpan? { |
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.
Should we move this up into to the other Tracer
requirements like startSpan
instead of this extension?
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.
Well this is the // MARK: - InMemoryTracer querying
so i think this is right 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.
Okay, I thought you wanted to separate protocol conformance from additional InMemoryTracer
-APIs here. If not I agree it thematically fits better here.
f1caa85
to
be1b852
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.
Looks great overall, just a few suggestions
<!-- After your change, what will change. -->
We just ignore writes made after a span was closed; and we ensure a double close won't mutate the timestamp NOR call the the onEnd multiple times. That's the expected baseline behavior.
be1b852
to
7dcf986
Compare
adc1fd9
to
e5f451c
Compare
This follows up #180 to get it over the finish line.
Main changes:
InMemoryTracer
in theInMemoryTracing
module@slashmo please have a look, you can pick this into your PR if it looks good; I'd love to get this merged and released very soon, since we have PRs piling up in other repos waiting for this :)