Skip to content
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

Introduce top level withSpan; reclaim Tracer and Instrument for protocols #113

Merged
merged 2 commits into from
Mar 31, 2023

Conversation

ktoso
Copy link
Member

@ktoso ktoso commented Mar 30, 2023

Motivation:
The TracerProtocol naming was annoyingly problematic and turns out we can and should actually move the withSpan APIs as top level functions which is typical Swift practice for such "global functionality".

This way we can withSpan() {} with less noise, and also gain back the Tracer protocol name.

resolves #109

@ktoso ktoso force-pushed the wip-reclaim-tracer-protocol-introduce-top-level-funcs branch 2 times, most recently from 470ab6a to d95bf10 Compare March 30, 2023 09:13
@ktoso ktoso changed the title Introduce top level withSpan; reclaim Tracer for protocol Introduce top level withSpan; reclaim Tracer and Instrument for protocols Mar 30, 2023
Copy link
Contributor

@stevapple stevapple left a comment

Choose a reason for hiding this comment

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

This show be fixed in multiple places(

Sources/Tracing/TracerProtocol+Legacy.swift Show resolved Hide resolved
Sources/Tracing/Tracer.swift Show resolved Hide resolved
Sources/Tracing/Tracer.swift Show resolved Hide resolved
Sources/Tracing/Tracer.swift Show resolved Hide resolved
Sources/Tracing/Tracer.swift Show resolved Hide resolved
/// - kind: The ``SpanKind`` of the new ``Span``.
/// - function: The function name in which the span was started
/// - fileID: The `fileID` where the span was started.
/// - line: The file line where the span was started.
Copy link
Member

Choose a reason for hiding this comment

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

Parameters don't match actual

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks will fix

Sources/Tracing/Tracer.swift Show resolved Hide resolved
/// - kind: The ``SpanKind`` of the new ``Span``.
/// - function: The function name in which the span was started
/// - fileID: The `fileID` where the span was started.
/// - line: The file line where the span was started.
Copy link
Member

Choose a reason for hiding this comment

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

Parameters don't match actual

/// - function: The function name in which the span was started
/// - fileID: The `fileID` where the span was started.
/// - line: The file line where the span was started.
/// - operation: The operation that this span should be measuring
Copy link
Member

Choose a reason for hiding this comment

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

Parameters don't match actual

/// - function: The function name in which the span was started
/// - fileID: The `fileID` where the span was started.
/// - line: The file line where the span was started.
/// - operation: The operation that this span should be measuring
Copy link
Member

Choose a reason for hiding this comment

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

Parameters don't match actual

Sources/Tracing/Tracer.swift Show resolved Hide resolved
**Motivation:**
The TracerProtocol naming was annoyingly problematic and turns out we
can and should actually move the withSpan APIs as top level functions
which is typical Swift practice for such "global functionality".

This way we can `withSpan() {}` with less noise, and also gain back the
Tracer protocol name.
@ktoso ktoso force-pushed the wip-reclaim-tracer-protocol-introduce-top-level-funcs branch from d859910 to 74b2beb Compare March 31, 2023 06:16
@ktoso ktoso force-pushed the wip-reclaim-tracer-protocol-introduce-top-level-funcs branch from 74b2beb to be47d00 Compare March 31, 2023 06:17
@ktoso
Copy link
Member Author

ktoso commented Mar 31, 2023

I think I fixed the parameter docs, thanks @yim-lee

I'll do another huge "docs" pass over all the docs in #69 as well tho

@ktoso ktoso merged commit 63a9c24 into main Mar 31, 2023
@ktoso ktoso deleted the wip-reclaim-tracer-protocol-introduce-top-level-funcs branch March 31, 2023 06:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove Tracer namespace and replace
4 participants