search

swift-server / swift-aws-lambda-runtime

Swift implementation of AWS Lambda Runtime
Apache License 2.0
1.13k stars 102 forks source link

RFC: go back to swift 5 #160

Closed tomerd closed 2 years ago

tomerd commented 4 years ago

motivation: ease integration with middle-tier libraries that want to continue and support older versions of swift

changes:

tomerd commented 4 years ago

@helge5 @fabianfett this is what going back to 5.0 look like. biggest pain is loosing property wrappers and needing to generate linux tests. 5.1 may be a more compelling proposition, but I am open to both

ktoso commented 4 years ago

I'm not sure tracing works on 5.0 (or could work, with the dynamic member lookup keypath magic...) I'll check and report back.

stevapple commented 4 years ago

If we give up PropertyWrappers, we should write custom encode and decode methods. I’m doing such things in this fork.

However, since the first official release of Amazon Linux 2 toolchain is 5.2.4, I don’t think it’s really necessary to support 5.0 and 5.1. Although Swift 5 provides an improved backward support, this will make the code a lot uglier and less flexible.

helje5 commented 4 years ago

Is it necessary to declare the Package.swift as 5.2 to get the automatic tests on Linux? Shouldn't that still work if you use an actual Swift 5.2 toolchain to process/test the 5.0 package?

If features are optional, you can still use 5.x, you'd just have to guard such with #if swift(>=5.1).

For info about this change (whether it is going to happen or not), see the long thread in the Slack. It's a package resolution issue (for libraries where Lambda is an option, not a requirement), not a deployment issue.

helje5 commented 4 years ago

Wrt the @ISO8601WithFractionalSecondsCoding, I'm not a fan of such things, it feels very much like a hack. I'd either do that in init(coder:), or better: in a custom Decoder which can deal with those inputs (i.e. adds appropriate date decoding strategies). (IMHO)

helje5 commented 4 years ago

I'm not sure tracing works on 5.0 (or could work, with the dynamic member lookup keypath magic...) I'll check and report back.

I think 4.2 added the string based dynamicMemberLookup, 5.0 added dynamicCallable. The "static" dynamicMemberLookup stuff was 5.1 or 5.2. (I think)

tomerd commented 4 years ago

Is it necessary to declare the Package.swift as 5.2 to get the automatic tests on Linux? Shouldn't that still work if you use an actual Swift 5.2 toolchain to process/test the 5.0 package?

@helje5 if we are declaring that the package is compatible with tools-version 5.0 we need to be able and test it on that toolchain, so we are forced to generate the linuxmain for that to work. in >= 5.1 we would test with --enable-test-discovery

pokryfka commented 4 years ago

Personally I am not sure eager decoding of date is a necessary at all; note that:

This may vary case to case, I considered my very own uses of date attributes:

For my very personal use I decided to use String types (or unsigned integer if "unix time" is provided).

ktoso commented 4 years ago

Overall: I'm not sure the supporting of all the way 5.0 in newly developed libraries is the highest priority... We have to keep moving forward, and incentivize doing so.

Specifically: Swift-Tracing and it's way to declare attributes in a pleasant (non insane) and type-safe way depends on typesafe dynamic member lookup. This won't work on Swift 5.0 ever, and could 5.1 but I'm hitting some compiler crashes right now (could be my environment's fault, not sure).

To be specific what we'd lose:

Attributes are normally plain APIs where string keys and a predefined set of value types are allowed:

import TracingInstrumentation

span.attributes["http.url"] = "\(niorequest.uri)
span.attributes["http.method"] = "\(niorequest.method)"

Though we're able to add industry specified well known values, and not tie the core library to those standards (as we do not want to, because we may diverge in the core slightly):

import OpenTelemetrySemanticConventions // (currently named OpenTelemetryInstrumentationSupport)

span.attributes.http.url = niorequest.uri // type-safe
span.attributes.http.method = niorequest.method // type-safe
// AND do note that this is type-safe, user extensible and can autocomplete / show docs etc, 
// so even newcomers will set the _right_ attributes

or even pre-baked "set all the interesting attributes": 

import OpenTelemetrySemanticConventionsNIO // does not exist (yet)

// and we can even optionally do:
span.attributes.http.request = niorequest

// which would populate all the "we want to keep those" attributes (there can be _many_)

This depends on 

    subscript<T>(dynamicMember dynamicMember: KeyPath<NestedAttributes, SpanAttributeKey<T>>) -> T? where T: SpanAttributeConvertible { get set }

Which is not a thing in early Swift 5 releases. We worked on this specific API spelling with Swift team to ensure it not only looks great but also will be efficient, and our goal with the swift-tracing API is to provide really best in class and "awesome to use" API to those things. Thus, I don't think we should drop the language requirement lower.

Now, Lambda should depend on swift-tracing because it should startSpan and end spans automatically on behalf of users, and thanks to the xRay backend users will automatically get the full tracing experience end to end. Thus, lambda can require at least 5.1 (if I figure out the compiler crashes), but definitely not 5.0.

Summing up though: I don't think it is unreasonable to require new projects to support "two language versions back", this makes a lot of sense, but I don't think the 5.0 requirement is reasonable. 5.1 is on the edge... We can/should try to make tracing work there and I'll give it some more looking why that compiler crash I'm seeing now. But realistically I think new libraries now should aim to support 5.2 and 5.3 right now, and we'll make a "special effort" for the 5.1 IMO.

I'll try a bit harder and see if we can get tracing to support 5.1, if that'd help?

pokryfka commented 4 years ago

@ktoso

I dont know min Swift version requirement as far as Lambda is concerned, SwiftNIO 2 supports Swift 5.0 and arguably all libraries using SwiftNIO should be able to use and benefit from swift-tracing.

or even pre-baked "set all the interesting attributes":

import OpenTelemetrySemanticConventionsNIO // does not exist (yet)

// and we can even optionally do:
span.attributes.http.request = niorequest

this could also be achieved with a type safe extension without dynamic member lookup and without (required by subscript) attributes getter (which goes against OTel recommendations), example from https://github.com/pokryfka/aws-xray-sdk-swift/blob/master/Sources/AWSXRayRecorder/Segment%2BNIOHTTP.swift:

import NIOHTTP1

extension XRayRecorder.Segment {
    public func setHTTPRequest(_ request: HTTPRequestHead) {
        let userAgent = request.headers["User-Agent"].first
        let clientIP = request.headers["X-Forwarded-For"].first
        setHTTPRequest(method: request.method.rawValue, url: request.uri, userAgent: userAgent, clientIP: clientIP)
    }
}
helje5 commented 4 years ago

I don't think it is unreasonable to require new projects to support "two language versions back"

I 100% agree with that, that would be Swift 4 and 5. It is reasonable to expect support for both if you are really serious about all this (like IBM has been). For my own projects I decided to start being lazy and only support 5.0+ (essentially stick to what NIO 2 is doing, though NIO 1 is also still available, and I did support that for quite a while to have Swift 4).

new libraries

My complaint isn't about new libraries. It's about adding (optional) Lambda support to existing things (either libraries or apps). That's what started this whole thing. I'd also like to repeat that you don't have to do this for me, I personally don't really care that much, though I think you should absolutely do it. The Lambda is conceptually "infrastructure software", i.e. not a leaf module like say SwiftBlocksUI. As such, it should support the lowest possible version.

Besides existing code, another issue are platforms which may not have the latest toolchain. Which funny enough affects Amazon Linux itself :-) (no 5.3 yet) More commonly Raspi's.

ktoso commented 4 years ago

With due respect, the Swift 4 support requirement is your opinion and not a fact; and the passive aggressive nitpicking tone is not welcome or helpful in getting the situation improved.

Looping back to the actual task though:

I think we can #if swift/compiler(>=5.1/2) around the subscript based features causing issues on Swift 5.1 (and not existing on 5.0). The 5.1 compiler crash I'm not sure we'll get a fix or workaround for, so we may need to disable it there too. (For that matter, it would allow 5.0 users as well). Those can nice accessors can be seen as "optional", because users can keep using the string based not-so-well-typed API (like @pokryfka mentions).

We would not offer mirroed "set... style" APIs I think, though. It is a specific goal of those packages to expose the best "swifty" API for those types as possible, even if we have to require e.g. > 5.1 for the nicer API.

It would allow 5.1/5.0 users, so that's a win, and would enable this PR -- if we want to merge it, that's a road we can take, though I'll leave the final decision how we want to approach supporting Swift versions up to @tomerd. I do not think we're committing "in general" to always support a X.0 though, but here we can take the extra effort if necessary.

ktoso commented 4 years ago

FYI: For what it's worth we ensured compatibility of swift-tracing with 5.0+ now: https://github.com/slashmo/gsoc-swift-tracing/commit/1986c52e6bba970c5de30fd2a679e3c388b08a72 Thanks to @slashmo for taking on the work 👍

Lamba is free to make its own decision, whatever it might be -- would not be hampered by swift-tracing.

fabianfett commented 2 years ago

SwiftNIO now only goes back to Swift 5.2 and we support that.