Add a TracingMacros module providing the @Traced macro #157

porglezomp · 2024-11-22T21:38:09Z

This provides a @Traced macro which adds a tracing span around the body of the function. This macro allows customizing the operationName, the context, and the kind of the span, same as the withSpan function. It also exposes the span itself into the scope of the function, with a customizable name.

This doesn't attempt to support automatically adding parameters as attributes, because the scoping rules of attached macros aren't very amenable to controlling that. That could be added in the future if it's judged necessary.

Examples:

@Traced("preheat oven")
func preheatOven(temperature: Int) async throws -> Oven {
    span.attributes["oven.targetTemperature"] = temperature
    await sleep(for: .seconds(6))
    return Oven()
}

expands to:

func preheatOven(temperature: Int) async throws -> Oven {
    withSpan("preheat oven") { span in
        span.attributes["oven.targetTemperature"] = temperature
        await sleep(for: .seconds(6))
        return Oven()
    }
}

Notes:

This places the macros into a separate product so that users who don't want to pay the compile-time cost of macros don't have to use it, you opt-in to that cost by depending on the TracingMacros module.

This adds minimum OS versions for Apple platforms in order to be able to depend on SwiftSyntax. This applies to the whole package since there are no per-target platform specifications. Most notably: This raises the macOS minimum deployment target from the (implicit) 10.13 to 10.15 (matching SwiftSyntax).

Resolves #125

@Traced

This is defined as a separate product so that users who don't want to pay the compile-time cost of macros don't have to use it, you opt-in to that cost by depending on the TracingMacros module. The @Traced macro is only available in Swift 6.0 compilers since function body macros were introduced in Swift 6.0. This adds minimum OS versions for Apple platforms in order to be able to depend on SwiftSyntax. This applies to the whole package since there are no per-target platform specifications. Most notably: This raises the macOS minimum deployment target from the (implicit) 10.13 to 10.15.

Based on the effects signature of the attached function we apply try/await as appropriate. But, if the function is async/throws but those effects aren't actually used in the function body, this causes a new warning because the closure isn't inferred to be async and/or throws. To avoid those warnings, we also apply matching effects specifiers to the withSpan closure.

First, allow overriding the regular parameters of the withSpan call: setting the operationName, the context, and the kind. If it's not specified, it's not passed to the function, which means the function remains the source of truth for default arguments. Also allow overriding the span name, which controls the variable binding in the closure body. This is primarily useful for avoiding shadowing an outer "span" variable.

porglezomp · 2024-11-22T21:43:06Z

(Sorry @Traced that the commit messages tag you, I didn't mean to do that! 🙇🏻‍♀️)

porglezomp · 2024-11-22T21:58:27Z

My bad with the trailing commas, I tested on 5.9 but not on 6.0, and I was using a toolchain with swiftlang/swift-evolution#2344

ktoso

Very nice! I had some minor comments, please have a look

ktoso · 2024-11-22T23:01:31Z

Sources/TracingMacros/TracedMacro.swift

+import Tracing
+
+#if compiler(>=6.0)
+@attached(body)


Needs some docs.

The shape is good though! I like the spanName as well, that's quite niffty.

ktoso · 2024-11-22T23:05:59Z

Sources/TracingMacros/TracedMacro.swift

+#if compiler(>=6.0)
+@attached(body)
+public macro Traced(
+    _ operationName: String? = nil,


I wonder if this may want to be an TracedOperationName like this:

public struct TracedOperationName: ExpressibleByStringLiteral{ let value: TracedOperationName package enum TracedOperationName { case baseName // the default "foo" case fullName // "foo(bar:) case string(String) } public static var baseName: ... // for all the above

WDYT?

I was going back and forth on whether it should be implemented with baseName / fullName semantics, so this looks good to me! Should this type be in TracingMacros?

Yup, right next to this Traced declaration would be fine

Actually I just realized this is also made slightly harder by the macro—we want this conceptually but we only have access to the syntax and not the evaluated value. Two options here:

Directly match looking for .baseName / .fullName / string expressions, and not being able to handle other kinds of expressions.

Or add a bit of runtime support expanding to something like:

withSpan(.baseName.text(baseName: "foo", fullName: "foo(bar:)")) { span in ... }

And then doing the selection in regular code on the expression.

Yes, it would be in TracingMacros I think. I think basename as basic is okey

ktoso · 2024-11-22T23:13:12Z

Sources/TracingMacros/TracedMacro.swift

+@attached(body)
+public macro Traced(
+    _ operationName: String? = nil,
+    context: ServiceContext? = nil,


Hmm, technically is the default .current rather than nil?

Should the type be the @autoclosure () -> ServiceContext as well, so we avoid reading the local when tracing is disabled (not bootstrapped)

I'm not entirely sure about the answer here since this is my first major macro.

For the default: I'm using nil just to delegate the default handling to withSpan itself—we don't even construct the syntax to pass a parameter when it's omitted here. I don't fully understand how default parameters are even passed to the macro.

For the type: since this is a macro it's handled via syntax expansion, so the type is used for checking the interface, not as a runtime type. When the expression is interpolated into the call, I think it will end up becoming an autoclosure at the withSpan callsite?

I did some experimentation: The default value isn't passed to the macro at all, it seems to just be for documentation purposes?

Since it seems to be only documentation—do we want to duplicate the default values from withSpan here, or cross-reference to say "look at the default values in the function documentation"? I can see arguments for both. I leaned towards omitting them here to not duplicate code across modules, but it's already duplicated across many overload so it's maybe worth it for the documentation's sake?

Oh good point about the autoclosure not mattering!

the nil maybe is fine after all, since it just means "don't pass anything" makes sense to me

For the docs: i think it's fine to not duplicate the docs, just a short note that it is passed to the withSpan will be enough

I see, yeah let's document to just look at the real method maybe

**Motivation:** We want to allow richer customization of the operation name in the `@Traced` macro, in particular the difference between the different ways you can view a function name. **Modifications:** - Add a TracedOperationName type that represents the kinds of operation names we want to support. - Change the `@Traced` macro interface and expansion to use the new operation name type. **Result:** Now you can write `@Traced(.baseName)`, `@Traced(.fullName)` as well as `@Traced("custom name here")`, giving more flexibility between types of operation names.

Lukasa · 2024-11-25T08:43:30Z

Package.swift

+        .tvOS(.v13),
+        .watchOS(.v6),
+        .macCatalyst(.v13),
+    ],


This change is a semver major, are we actually intending to mint a new semver for this?

This is required because swiftpm doesn't allow specifying this at any finer granularity than the whole package, and it can't handle the dependencies in only a single product.

I was unsure how strictly this would be a breaking change since there was no versions explicitly stated here, but wanted to discuss it.

I'm noticing that these are already the minimum platforms for swift-distributed-tracing-extras and so we could put these macros there instead of here without a semver major bump. I'd be happy with that as a place for this to live, Konrad just initially suggested here.

Alternative PR over there: apple/swift-distributed-tracing-extras#42

Yeah, unfortunately this is a breaking change because SwiftPM doesn’t take the minimum deployment targets into consideration when resolving versions. The result is broken builds, caused by picking up the new minima.

While we’re on the topic of breakage, Swift-syntax also represents a risk. Because of their versioning strategy it’s very easy to produce unsolveable build graphs, or forcing users to not be able to update past the current version. I’m generally nervous about putting this anywhere it might accidentally get picked up.

For that at least—am I correct that package versions are only resolved for products that are dependent on, so that at least requires libraries to depend on the macros to get breakage? But yes I don't have any experience with releasing a package providing macros, I have no idea what is sufficient here. Just putting it in -extras? Putting it in a whole separate package?

I think extras might be fine -- this was replaced by apple/swift-distributed-tracing-extras#42

ktoso · 2024-11-26T08:06:33Z

Replaced by: in apple/swift-distributed-tracing-extras#42 -extras

porglezomp added 3 commits November 18, 2024 22:33

ktoso self-requested a review November 22, 2024 21:55

porglezomp added 3 commits November 22, 2024 14:03

fixup! Add support for parameters passed to the @Traced macro

2cbfb9f

[WIP] Fixup formatting. Will do commit-by-commit post-review

f00afb1

[WIP] Fix license comment ordering

8ae7100

ktoso added the 🆕 semver/minor Adds new public API. label Nov 22, 2024

ktoso reviewed Nov 22, 2024

View reviewed changes

porglezomp added 2 commits November 22, 2024 16:12

Add documentation for @Traced

24364ca

Lukasa requested changes Nov 25, 2024

View reviewed changes

porglezomp mentioned this pull request Nov 25, 2024

Add a TracingMacros module providing the @Traced macro apple/swift-distributed-tracing-extras#42

Merged

ktoso closed this Nov 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a TracingMacros module providing the @Traced macro #157

Add a TracingMacros module providing the @Traced macro #157

porglezomp commented Nov 22, 2024

porglezomp commented Nov 22, 2024

porglezomp commented Nov 22, 2024

ktoso left a comment

ktoso Nov 22, 2024

ktoso Nov 22, 2024

porglezomp Nov 22, 2024

ktoso Nov 22, 2024

porglezomp Nov 23, 2024 •

edited

Loading

ktoso Nov 26, 2024

ktoso Nov 22, 2024

porglezomp Nov 22, 2024

porglezomp Nov 22, 2024

porglezomp Nov 22, 2024 •

edited

Loading

ktoso Nov 22, 2024

ktoso Nov 26, 2024

Lukasa Nov 25, 2024

porglezomp Nov 25, 2024 •

edited

Loading

porglezomp Nov 25, 2024

Lukasa Nov 26, 2024

porglezomp Nov 26, 2024

ktoso Nov 26, 2024

ktoso commented Nov 26, 2024

Add a TracingMacros module providing the @Traced macro #157

Add a TracingMacros module providing the @Traced macro #157

Conversation

porglezomp commented Nov 22, 2024

Examples:

Notes:

porglezomp commented Nov 22, 2024

porglezomp commented Nov 22, 2024

ktoso 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

porglezomp Nov 23, 2024 • edited Loading

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

porglezomp Nov 22, 2024 • edited Loading

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

porglezomp Nov 25, 2024 • edited Loading

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 Nov 26, 2024

porglezomp Nov 23, 2024 •

edited

Loading

porglezomp Nov 22, 2024 •

edited

Loading

porglezomp Nov 25, 2024 •

edited

Loading