Skip to content

associated type Span, Tracer as short-hand, and *Protocol types #92

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

Closed
wants to merge 5 commits into from

Conversation

ktoso
Copy link
Member

@ktoso ktoso commented Mar 13, 2023

Motivation:

This is a revival of
#84 where we try to KEEP compatibility with versions below 5.7 with a compatibility "legacy" tracer type, but otherwise move towards requiring 5.7 for all the "nice" apis that use associated types and any TracerProtocol and friends

Modifications:

  • Tracer -> TracerProtocol
  • Tracer is now a namespace in order to Tracer.withSpan {} easily
  • Span -> SpanProtocol
  • Introduce LegacyTracerProtocol which does not make use of associated type Span, and can be used in 5.6 libraries; they can deprecate and move away form it ASAP as they start requiring 5.7

Result:

Offer the APIs we want in 5.7 but remain compatible with 5.6 until we drop it as soon as 5.9 is released as stable - this allows us to adopt eagerly in libraries without having to wait for 5.9 to drop.

Replaces #84

@@ -26,7 +26,7 @@ import struct Dispatch.DispatchWallTime
/// Creating a `Span` is delegated to a ``Tracer`` and end users should never create them directly.
///
/// - SeeAlso: For more details refer to the [OpenTelemetry Specification: Span](https://github.com/open-telemetry/opentelemetry-specification/blob/v0.7.0/specification/trace/api.md#span) which this type is compatible with.
public protocol Span: AnyObject, _SwiftTracingSendableSpan {
Copy link
Member

Choose a reason for hiding this comment

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

Something that has just come to my mind, if we try to make not using a tracer as noop as possible we should lift the AnyObject requirement here. This way we can implement Noops with empty spans.

Copy link
Member Author

@ktoso ktoso Mar 13, 2023

Choose a reason for hiding this comment

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

Not sure that makes sense... Semantically they really are reference types.

You also wanted to return concrete types making it not possible to return NoopSpan "sometimes" and the "real one" usually. since that's different types.

So I guess the only trick this could pull then is:

  • to have a struct or enum that is "the noop one or the real one that contains a class box with the actual impl. I'm assuming we'd keep the methods not mutating even then, since value semantics are wrong for spans.

Copy link
Member

Choose a reason for hiding this comment

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

Not sure that makes sense... Semantically they really are reference types.

100% agree. In NIO we sometimes use structs that wrap reference types and we document heavily that they have reference semantics.

to have a struct or enum that is "the noop one or the real one that contains a class box with the actual impl. I'm assuming we'd keep the methods not mutating even then, since value semantics are wrong for spans.

Yes! 100% this.

Copy link
Member Author

@ktoso ktoso Mar 13, 2023

Choose a reason for hiding this comment

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

This "let's pretend nonmutating" won't work, I feel like I've been through this multiple times already if I'm honest.

I won't do this dance in this PR.

Things break e.g. here:

        let tracer: any TracerProtocol = InstrumentationSystem.tracer

        var baggage = Baggage.topLevel
        InstrumentationSystem.instrument.extract(request.headers, into: &baggage, using: HTTPHeadersExtractor())

        let span = tracer.startSpan("GET \(request.path)", baggage: baggage)

        let response = self.catchAllHandler(span.baggage, request, self.client)
        span.attributes["http.status"] = response.status // requires mutating guarantee or AnyObject
/Users/ktoso/code/swift-distributed-tracing/Tests/TracingTests/TracerTests.swift:359:24: error: cannot assign through subscript: 'span' is a 'let' constant
        span.attributes["http.status"] = response.status
        ~~~~           ^
/Users/ktoso/code/swift-distributed-tracing/Tests/TracingTests/TracerTests.swift:356:9: note: change 'let' to 'var' to make it mutable
        let span = tracer.startSpan("GET \(request.path)", baggage: baggage)
        ^~~
        var

making the span a var sends a VERY wrong message so that's not an option.

We can't just remove this subscript either since the typesafe properties need subscripts.

Copy link
Member Author

Choose a reason for hiding this comment

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

solved in #94

@ktoso ktoso requested a review from fabianfett March 13, 2023 08:42
ktoso added 2 commits March 13, 2023 22:08
**Motivation:**

This is a revival of
#84
where we try to KEEP compatibility with versions below 5.7 with a
compatibility "legacy" tracer type, but otherwise move towards requiring
5.7 for all the "nice" apis that use associated types and `any
  TracerProtocol` and friends

**Modifications:**

- `Tracer` -> `TracerProtocol`
- `Tracer` is now a namespace in order to `Tracer.withSpan {}` easily
- `Span` -> `SpanProtocol`
- Introduce `LegacyTracerProtocol` which does not make use of associated
  type Span, and can be used in 5.6 libraries; they can deprecate and
  move away form it ASAP as they start requiring 5.7

**Result:**

Offer the APIs we want in 5.7 but remain compatible with 5.6 until we
drop it as soon as 5.9 is released as stable - this allows us to adopt
eagerly in libraries without having to wait for 5.9 to drop.
@ktoso ktoso force-pushed the wip-towards-5.7 branch from e401e3c to cb24ba3 Compare March 13, 2023 13:09
@ktoso
Copy link
Member Author

ktoso commented Mar 15, 2023

Can't merge this since we stopped running 5.5 CI but this PR requires it 😆

Making a new PR

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.

None yet

2 participants