-
Notifications
You must be signed in to change notification settings - Fork 48
Add a TracingMacros module providing the @Traced macro #157
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
Changes from 6 commits
82ea68d
50446c1
101df66
2cbfb9f
f00afb1
8ae7100
24364ca
8b3cfd3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
//===----------------------------------------------------------------------===// | ||
// | ||
// This source file is part of the Swift Distributed Tracing open source project | ||
// | ||
// Copyright (c) 2020-2024 Apple Inc. and the Swift Distributed Tracing project authors | ||
// Licensed under Apache License v2.0 | ||
// | ||
// See LICENSE.txt for license information | ||
// See CONTRIBUTORS.txt for the list of Swift Distributed Tracing project authors | ||
// | ||
// SPDX-License-Identifier: Apache-2.0 | ||
// | ||
//===----------------------------------------------------------------------===// | ||
@_exported import ServiceContextModule | ||
import Tracing | ||
|
||
#if compiler(>=6.0) | ||
@attached(body) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Needs some docs. The shape is good though! I like the |
||
public macro Traced( | ||
_ operationName: String? = nil, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if this may want to be an
WDYT? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was going back and forth on whether it should be implemented with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yup, right next to this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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:
withSpan(.baseName.text(baseName: "foo", fullName: "foo(bar:)")) { span in ... } And then doing the selection in regular code on the expression. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, it would be in TracingMacros I think. I think basename as basic is okey |
||
context: ServiceContext? = nil, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, technically is the default Should the type be the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not entirely sure about the answer here since this is my first major macro.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I did some experimentation: The default value isn't passed to the macro at all, it seems to just be for documentation purposes? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since it seems to be only documentation—do we want to duplicate the default values from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see, yeah let's document to just look at the real method maybe |
||
ofKind kind: SpanKind? = nil, | ||
span spanName: String? = nil | ||
) = #externalMacro(module: "TracingMacrosImplementation", type: "TracedMacro") | ||
#endif |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,152 @@ | ||
//===----------------------------------------------------------------------===// | ||
// | ||
// This source file is part of the Swift Distributed Tracing open source project | ||
// | ||
// Copyright (c) 2020-2024 Apple Inc. and the Swift Distributed Tracing project authors | ||
// Licensed under Apache License v2.0 | ||
// | ||
// See LICENSE.txt for license information | ||
// See CONTRIBUTORS.txt for the list of Swift Distributed Tracing project authors | ||
// | ||
// SPDX-License-Identifier: Apache-2.0 | ||
// | ||
//===----------------------------------------------------------------------===// | ||
import SwiftCompilerPlugin | ||
import SwiftSyntax | ||
import SwiftSyntaxBuilder | ||
import SwiftSyntaxMacros | ||
|
||
#if compiler(>=6.0) | ||
public struct TracedMacro: BodyMacro { | ||
public static func expansion( | ||
of node: AttributeSyntax, | ||
providingBodyFor declaration: some DeclSyntaxProtocol & WithOptionalCodeBlockSyntax, | ||
in context: some MacroExpansionContext | ||
) throws -> [CodeBlockItemSyntax] { | ||
guard let function = declaration.as(FunctionDeclSyntax.self), | ||
let body = function.body | ||
else { | ||
throw MacroExpansionErrorMessage("expected a function with a body") | ||
} | ||
|
||
// Construct a withSpan call matching the invocation of the @Traced macro | ||
let (operationName, context, kind, spanName) = try extractArguments(from: node) | ||
|
||
var withSpanCall = FunctionCallExprSyntax("withSpan()" as ExprSyntax)! | ||
withSpanCall.arguments.append( | ||
LabeledExprSyntax( | ||
expression: operationName ?? ExprSyntax(StringLiteralExprSyntax(content: function.name.text)) | ||
) | ||
) | ||
func appendComma() { | ||
withSpanCall.arguments[withSpanCall.arguments.index(before: withSpanCall.arguments.endIndex)] | ||
.trailingComma = .commaToken() | ||
} | ||
if let context { | ||
appendComma() | ||
withSpanCall.arguments.append(LabeledExprSyntax(label: "context", expression: context)) | ||
} | ||
if let kind { | ||
appendComma() | ||
withSpanCall.arguments.append(LabeledExprSyntax(label: "ofKind", expression: kind)) | ||
} | ||
|
||
// Introduce a span identifier in scope | ||
var spanIdentifier: TokenSyntax = "span" | ||
if let spanName { | ||
spanIdentifier = .identifier(spanName) | ||
} | ||
|
||
// We want to explicitly specify the closure effect specifiers in order | ||
// to avoid warnings about unused try/await expressions. | ||
// We might as well explicitly specify the closure return type to help type inference. | ||
|
||
let asyncClause = function.signature.effectSpecifiers?.asyncSpecifier | ||
let returnClause = function.signature.returnClause | ||
var throwsClause = function.signature.effectSpecifiers?.throwsClause | ||
// You aren't allowed to apply "rethrows" as a closure effect | ||
// specifier, so we have to convert this to a "throws" effect | ||
if throwsClause?.throwsSpecifier.tokenKind == .keyword(.rethrows) { | ||
throwsClause?.throwsSpecifier = .keyword(.throws) | ||
} | ||
var withSpanExpr: ExprSyntax = """ | ||
\(withSpanCall) { \(spanIdentifier) \(asyncClause)\(throwsClause)\(returnClause)in \(body.statements) } | ||
""" | ||
|
||
// Apply a try / await as necessary to adapt the withSpan expression | ||
|
||
if function.signature.effectSpecifiers?.asyncSpecifier != nil { | ||
withSpanExpr = "await \(withSpanExpr)" | ||
} | ||
|
||
if function.signature.effectSpecifiers?.throwsClause != nil { | ||
withSpanExpr = "try \(withSpanExpr)" | ||
} | ||
|
||
return ["\(withSpanExpr)"] | ||
} | ||
|
||
static func extractArguments( | ||
from node: AttributeSyntax | ||
) throws -> ( | ||
operationName: ExprSyntax?, | ||
context: ExprSyntax?, | ||
kind: ExprSyntax?, | ||
spanName: String? | ||
) { | ||
// If there are no arguments, we don't have to do any of these bindings | ||
guard let arguments = node.arguments?.as(LabeledExprListSyntax.self) else { | ||
return (nil, nil, nil, nil) | ||
} | ||
|
||
func getArgument(label: String) -> ExprSyntax? { | ||
arguments.first(where: { $0.label?.identifier?.name == label })?.expression | ||
} | ||
|
||
// The operation name is the first argument if it's unlabeled | ||
var operationName: ExprSyntax? | ||
if let firstArgument = arguments.first, firstArgument.label == nil { | ||
operationName = firstArgument.expression | ||
} | ||
|
||
let context = getArgument(label: "context") | ||
let kind = getArgument(label: "ofKind") | ||
var spanName: String? | ||
let spanNameExpr = getArgument(label: "span") | ||
if let spanNameExpr { | ||
guard let stringLiteral = spanNameExpr.as(StringLiteralExprSyntax.self), | ||
stringLiteral.segments.count == 1, | ||
let segment = stringLiteral.segments.first, | ||
let segmentText = segment.as(StringSegmentSyntax.self) | ||
else { | ||
throw MacroExpansionErrorMessage("span name must be a simple string literal") | ||
} | ||
let text = segmentText.content.text | ||
let isValidIdentifier = DeclReferenceExprSyntax("\(raw: text)" as ExprSyntax)?.hasError == false | ||
let isValidWildcard = text == "_" | ||
guard isValidIdentifier || isValidWildcard else { | ||
throw MacroExpansionErrorMessage("'\(text)' is not a valid parameter name") | ||
} | ||
spanName = text | ||
} | ||
return ( | ||
operationName: operationName, | ||
context: context, | ||
kind: kind, | ||
spanName: spanName | ||
) | ||
} | ||
|
||
} | ||
#endif | ||
|
||
@main | ||
struct TracingMacroPlugin: CompilerPlugin { | ||
#if compiler(>=6.0) | ||
let providingMacros: [Macro.Type] = [ | ||
TracedMacro.self | ||
] | ||
#else | ||
let providingMacros: [Macro.Type] = [] | ||
#endif | ||
} |
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.
This change is a semver major, are we actually intending to mint a new semver for this?
Uh oh!
There was an error while loading. Please reload this page.
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.
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.
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.
Alternative PR over there: apple/swift-distributed-tracing-extras#42
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.
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.
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.
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?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.
I think extras might be fine -- this was replaced by apple/swift-distributed-tracing-extras#42