Adopt ServiceContext 1.0 #127

ktoso · 2023-05-26T22:21:59Z

Resolves #99

Motivation:
The baggage type ended up more useful than anticipated and we're renaming it to ServiceContext for the 1.0 release.

Many libraries and production systems end up passing context around which may not necessarily be related to tracing, which made us make this naming change. Functionally ServiceContext and Baggage are the same, but the way we talk about context propagation in general makes more sense if we call this a context stype.

Modifications:

Use the new type; The Baggage still exists in the InstrumentationBaggage module so people can still use it if they wanted to but it is a TYPEALIAS to the ServiceContext.

slashmo

LGTM 👍

slashmo · 2023-05-27T07:03:37Z

README.md

@@ -14,7 +14,7 @@ While Swift Distributed Tracing allows building all kinds of _instruments_, whic

 This project uses the context progagation type defined independently in:

- 🧳 [swift-distributed-tracing-baggage](https://github.com/apple/swift-distributed-tracing-baggage) -- [`Baggage`](https://apple.github.io/swift-distributed-tracing-baggage/docs/current/InstrumentationBaggage/Structs/Baggage.html) (zero dependencies)
+- 🧳 [swift-service-context](https://github.com/apple/swift-service-context) -- [`Baggage`](https://apple.github.io/swift-service-context/docs/current/ServiceContextModule/Structs/Baggage.html) (zero dependencies)


Should these links point to the Swift Package Index hosted docs instead? This one gives me a 404.

This one should be 404 because Baggage is renamed. +1 for switching to SPI though.

slashmo · 2023-05-27T07:04:26Z

README.md

@@ -330,7 +330,7 @@ InstrumentationSystem.bootstrap(MultiplexInstrument([FancyInstrument(), OtherFan
 <a name="passing-context-objects"></a>
 ### Context propagation, by explicit `LoggingContext` passing

-> `LoggingContext` naming has been carefully selected and it reflects the type's purpose and utility: It binds a [Swift Log `Logger`](https://github.com/apple/swift-log) with an associated distributed tracing [Baggage](https://github.com/apple/swift-distributed-tracing-baggage).
+> `LoggingContext` naming has been carefully selected and it reflects the type's purpose and utility: It binds a [Swift Log `Logger`](https://github.com/apple/swift-log) with an associated distributed tracing [Baggage](https://github.com/apple/swift-service-context).


Suggested change

> `LoggingContext` naming has been carefully selected and it reflects the type's purpose and utility: It binds a [Swift Log `Logger`](https://github.com/apple/swift-log) with an associated distributed tracing [Baggage](https://github.com/apple/swift-service-context).

> `LoggingContext` naming has been carefully selected and it reflects the type's purpose and utility: It binds a [Swift Log `Logger`](https://github.com/apple/swift-log) with an associated service context [ServiceContext](https://github.com/apple/swift-service-context).

I guess the intention was to embed type names in the sentence, so this should be better:

Suggested change

> `LoggingContext` naming has been carefully selected and it reflects the type's purpose and utility: It binds a [Swift Log `Logger`](https://github.com/apple/swift-log) with an associated distributed tracing [Baggage](https://github.com/apple/swift-service-context).

> `LoggingContext` naming has been carefully selected and it reflects the type's purpose and utility: It binds a [`Logger`](https://github.com/apple/swift-log) with an associated [`ServiceContext`](https://github.com/apple/swift-service-context).

README.md

Sources/Tracing/TracerProtocol.swift

ktoso · 2023-05-27T11:13:54Z

Thanks for review! I’ll push this to completion on Monday I think

stevapple

With the renaming it’s also worth clarifying the relationship between Swift ServiceContext and W3C Baggage / Trace Context, especially the latter — since this is package for distributed tracing.

stevapple · 2023-05-27T13:12:04Z

Package.swift

@@ -14,7 +14,7 @@ let package = Package(
        .library(name: "Tracing", targets: ["Tracing"]),
    ],
    dependencies: [
-        .package(url: "https://github.com/apple/swift-distributed-tracing-baggage.git", .upToNextMinor(from: "0.4.1")),
+        .package(url: "https://github.com/apple/swift-service-context.git", .upToNextMinor(from: "1.0.0")),


With a stable 1.0 we don’t need .upToMextMinor any more.

Suggested change

.package(url: "https://github.com/apple/swift-service-context.git", .upToNextMinor(from: "1.0.0")),

.package(url: "https://github.com/apple/swift-service-context.git", from: "1.0.0"),

stevapple · 2023-05-27T13:13:12Z

README.md

@@ -14,7 +14,7 @@ While Swift Distributed Tracing allows building all kinds of _instruments_, whic

 This project uses the context progagation type defined independently in:

- 🧳 [swift-distributed-tracing-baggage](https://github.com/apple/swift-distributed-tracing-baggage) -- [`Baggage`](https://apple.github.io/swift-distributed-tracing-baggage/docs/current/InstrumentationBaggage/Structs/Baggage.html) (zero dependencies)
+- 🧳 [swift-service-context](https://github.com/apple/swift-service-context) -- [`Baggage`](https://apple.github.io/swift-service-context/docs/current/ServiceContextModule/Structs/Baggage.html) (zero dependencies)


This one should be 404 because Baggage is renamed. +1 for switching to SPI though.

stevapple · 2023-05-27T13:15:19Z

README.md

@@ -330,7 +330,7 @@ InstrumentationSystem.bootstrap(MultiplexInstrument([FancyInstrument(), OtherFan
 <a name="passing-context-objects"></a>
 ### Context propagation, by explicit `LoggingContext` passing

-> `LoggingContext` naming has been carefully selected and it reflects the type's purpose and utility: It binds a [Swift Log `Logger`](https://github.com/apple/swift-log) with an associated distributed tracing [Baggage](https://github.com/apple/swift-distributed-tracing-baggage).
+> `LoggingContext` naming has been carefully selected and it reflects the type's purpose and utility: It binds a [Swift Log `Logger`](https://github.com/apple/swift-log) with an associated distributed tracing [Baggage](https://github.com/apple/swift-service-context).


I guess the intention was to embed type names in the sentence, so this should be better:

Suggested change

> `LoggingContext` naming has been carefully selected and it reflects the type's purpose and utility: It binds a [Swift Log `Logger`](https://github.com/apple/swift-log) with an associated distributed tracing [Baggage](https://github.com/apple/swift-service-context).

> `LoggingContext` naming has been carefully selected and it reflects the type's purpose and utility: It binds a [`Logger`](https://github.com/apple/swift-log) with an associated [`ServiceContext`](https://github.com/apple/swift-service-context).

README.md

Sources/Tracing/Docs.docc/Guides/InstrumentYourLibrary.md

stevapple · 2023-05-27T13:38:11Z

Sources/Tracing/Docs.docc/Guides/InstrumentYourLibrary.md

      await userCode(request)
    }
  }
 }
 ```

-This is introducing multiple layers of nesting, and we have un-necessarily restored, picked-up, and restored the baggage again. In order to avoid this duplicate work, it is beneficial to use the ``withSpan(_:baggage:ofKind:at:function:file:line:_:)-4o2b`` overload, which also accepts a `Baggage` as parameter, rather than picking it up from the task-local value:
+This is introducing multiple layers of nesting, and we have un-necessarily restored, picked-up, and restored the context again. In order to avoid this duplicate work, it is beneficial to use the ``withSpan(_:context:ofKind:at:function:file:line:_:)-4o2b`` overload, which also accepts a `ServiceContext` as parameter, rather than picking it up from the task-local value:


It’s curious to me whether Baggage and ServiceContext are countable for the library — it seems we’re skipping the article somewhere. I really think we need it, but cleaning up should take an extra pass.

Sorry I didn't understand the question/concern here?

Sources/Tracing/Docs.docc/Guides/TraceYourApplication.md

stevapple · 2023-05-27T13:45:10Z

Sources/Instrumentation/Instrument.swift

    ///   - extractor: The ``Extractor`` that extracts values from the given `Carrier`.
-    func extract<Carrier, Extract>(_ carrier: Carrier, into baggage: inout Baggage, using extractor: Extract)
+    func extract<Carrier, Extract>(_ carrier: Carrier, into baggage: inout ServiceContext, using extractor: Extract)


It seems we already picked context?

Suggested change

func extract<Carrier, Extract>(_ carrier: Carrier, into baggage: inout ServiceContext, using extractor: Extract)

func extract<Carrier, Extract>(_ carrier: Carrier, into context: inout ServiceContext, using extractor: Extract)

Sources/Instrumentation/Instrument.swift

ktoso · 2023-05-30T20:42:40Z

Sorry to have you folks pinged while this was still a draft; I've gone through now cleaning up more of the baggage word use and will be doing another pass tomorrow to clean things up and add any necessary wording about what the context is.

The entire README is to be replaced by the reference docs pages.

**Motivation:** The baggage type ended up more useful than anticipated and we're renaming it to ServiceContext for the 1.0 release. Many libraries and production systems and up passing context around which may not necessarily be related to tracing, which made us make this naming change. Functionally ServiceContext and Baggage are the same, but the way we talk about context propagation in general makes more sense if we call this a context stype. **Modifications:** Use the new type; The `Baggage` still exists in the `InstrumentationBaggage` module so people can still use it if they wanted to but it is a TYPEALIAS to the ServiceContext.

FranzBusch · 2023-05-31T10:17:30Z

Tests/TracingTests/LegacyBaggageTests.swift

+
+@testable import Instrumentation
+import ServiceContextModule
+import InstrumentationBaggage // legacy module, kept for easier migrations


I thought we are going to pull out the legacy baggage module? Should we remove this test?

So this module exists in the context repo and it is optional and immediately deprecated in there. It is a module with a typealias to the new context style, so if someone uses it explicitly they’ll get a migration warning. Though we don’t reexport this module anymore in tracing…

I did not make any APIs in tracing use baggage though.

it’s a bit tricky to judge how worth it is

IMO if we are going to drop baggage here we should also do it over in the service-context package. This way we don't release software with immediate deprecations

Yeah I can be convinced to do this I guess hmmm... we had this idea to be less breaking to folks with @tomerd bur maybe it doesn't matter? It would only work for those that explicitly used the baggage MODULE which should be very few... like just @weissi probably hm

Heh actually, we can't have this test since we're testing here with warnings-as-errors here so the deprecation becomes an error and fails the build 😉

error: 'Baggage' is deprecated: Use 'ServiceContext' from 'ServiceContextModule' instead.

FranzBusch

LGTM! Just one minor comment

…ors)

ktoso requested review from tomerd, yim-lee and slashmo May 26, 2023 22:22

ktoso marked this pull request as draft May 26, 2023 22:57

slashmo approved these changes May 27, 2023

View reviewed changes

stevapple reviewed May 27, 2023

View reviewed changes

ktoso force-pushed the wip-service-context branch 3 times, most recently from bc0d3c1 to 68f75b2 Compare May 30, 2023 21:37

ktoso force-pushed the wip-service-context branch from 9114e51 to 5dbdb4f Compare May 31, 2023 10:12

FranzBusch reviewed May 31, 2023

View reviewed changes

FranzBusch approved these changes May 31, 2023

View reviewed changes

ktoso marked this pull request as ready for review May 31, 2023 12:55

Remove the legacy test, cannot test with deprecation warnings (as err…

3677cbc

…ors)

ktoso merged commit 394441c into main May 31, 2023

ktoso deleted the wip-service-context branch May 31, 2023 20:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adopt ServiceContext 1.0 #127

Adopt ServiceContext 1.0 #127

ktoso commented May 26, 2023 •

edited by slashmo

Loading

slashmo left a comment

slashmo May 27, 2023

stevapple May 27, 2023

slashmo May 27, 2023

stevapple May 27, 2023

ktoso commented May 27, 2023

stevapple left a comment

stevapple May 27, 2023

stevapple May 27, 2023

stevapple May 27, 2023

stevapple May 27, 2023

ktoso May 31, 2023

stevapple May 27, 2023

ktoso commented May 30, 2023

FranzBusch May 31, 2023

ktoso May 31, 2023

FranzBusch May 31, 2023

ktoso May 31, 2023

ktoso May 31, 2023

FranzBusch left a comment

	> `LoggingContext` naming has been carefully selected and it reflects the type's purpose and utility: It binds a [Swift Log `Logger`](https://github.com/apple/swift-log) with an associated distributed tracing [Baggage](https://github.com/apple/swift-service-context).
	> `LoggingContext` naming has been carefully selected and it reflects the type's purpose and utility: It binds a [Swift Log `Logger`](https://github.com/apple/swift-log) with an associated service context [ServiceContext](https://github.com/apple/swift-service-context).

	.package(url: "https://github.com/apple/swift-service-context.git", .upToNextMinor(from: "1.0.0")),
	.package(url: "https://github.com/apple/swift-service-context.git", from: "1.0.0"),

	func extract<Carrier, Extract>(_ carrier: Carrier, into baggage: inout ServiceContext, using extractor: Extract)
	func extract<Carrier, Extract>(_ carrier: Carrier, into context: inout ServiceContext, using extractor: Extract)

Adopt ServiceContext 1.0 #127

Adopt ServiceContext 1.0 #127

Conversation

ktoso commented May 26, 2023 • edited by slashmo Loading

slashmo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ktoso commented May 27, 2023

stevapple left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ktoso commented May 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FranzBusch left a comment

Choose a reason for hiding this comment

ktoso commented May 26, 2023 •

edited by slashmo

Loading