Skip to content
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

feat: Server Side Swift #42

Closed
wants to merge 2 commits into from

Conversation

richardpiazza
Copy link

@richardpiazza richardpiazza commented Feb 23, 2023

Summary

This PR adds support for server-side-swift, specifically on Linux.

Although the repository specially called out Apple platforms (This is Amplitude's latest version of the iOS SDK, covering iOS/tvOS/macOS/watchOS.), Swift as a language is growing beyond those specific usages. With a few modifications, this library can be adapted to run on other Swift native environments.

  • Adds a 'test-linux' job to the GitHub workflow.
  • Isolates the OSLog by checking for the ability to import os.log
    • Ideally, a multi platform logging solution - such as swift-log - would be considered.
  • Adds additional import checks for FoundationNetworking which has the class definition for networking-related types.
  • Add a LinuxVendorSystem which uses the ProcessInfo to determine some OS specifics.
  • Minor refactor on some unit tests.
    • VSCode on Ubuntu was having difficulties with the type checking.
    • Using XCTUnwrap allowed for removing of the forced unwrap optionals.

Checklist

  • Does your PR title have the correct title format?
  • Does your PR have a breaking change?: no

@liuyang1520
Copy link
Collaborator

Hi @richardpiazza ,

Thank you for supporting Amplitude!! As we are not familiar with server-side Swift and don't have the Linux machine at the hand, some questions regarding this PR:

  • Could you please add an example of the server-side Swift under https://github.com/amplitude/Amplitude-Swift/tree/main/Examples with basic instruction inside for how to run it?
  • For storage, we use file storage in the iOS world, does the server-side Swift have the similar access to the file system? This is also related to the response handlers.
  • For unit tests, are there any server-side Swift specific tests we need to cover? (Thanks for adding the new test workflow!)

Thanks!

@richardpiazza
Copy link
Author

Happy to help out...

  • I've add a ServerSideSwiftExample project to the Examples folder. This swift package project uses Vapor: a popular open source swift web server. There is a readme in that folder with details on how to configure and run the project.
  • There shouldn't be any concerns about storage. Both UserDefaults and FileManager are part of the swift-corelibs-foundation project which implements much of Foundation on non-darwin (linux/windows/etc) systems. The status of those implementations are available here. Although both of those classes are marked as 'partial', they appear to be complete enough based on your library usages.
  • There are a few things with XCTest that are not implemented for non-apple systems (async tests for example), but I've already made the adjustments needed for running on Linux. I don't foresee any issues with how existing tests are written.

  • The SSW (Server-Side-Swift) example project also includes a unit test. This test verifies data written to the disk when using the PersistentStorage system. I've added an additional step in the GitHub workflow tests to run this test along with all of the rest of the tests on the 'ubuntu-latest' runner. (Asserting that the storage solution works on Linux).
  • The SSW example can be run on both macOS and Linux.

@liuyang1520
Copy link
Collaborator

Thank you for adding this and your detailed explanation! I will clone the fork and try the example in my local.

Copy link
Collaborator

@liuyang1520 liuyang1520 left a comment

Choose a reason for hiding this comment

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

The change looks good overall to me, thanks for the contribution! Some comments:

  • I tried to run the server locally in XCode (macOS), however, there seems a issue with the Timer (flush is triggered automatically). I am not sure whether it is OS issue or my local setup issue. Will check with my fellow to diagnose it.
  • For the server-side usage, I don't think we need session events. @yuhao900914 , I know we have the config trackingSessionEvents, do you have any suggestion here?

Sources/Amplitude/ConsoleLogger.swift Outdated Show resolved Hide resolved
Sources/Amplitude/ConsoleLogger.swift Outdated Show resolved Hide resolved
Sources/Amplitude/ConsoleLogger.swift Outdated Show resolved Hide resolved
Sources/Amplitude/ConsoleLogger.swift Outdated Show resolved Hide resolved
@liuyang1520
Copy link
Collaborator

I tried to run the server locally in XCode (macOS), however, there seems a issue with the Timer (flush is triggered automatically). I am not sure whether it is OS issue or my local setup issue. Will check with my fellow to diagnose it.

Tested with @yuhao900914 , the Timer doesn't work in her local either. To elaborate it a little bit:

  • SDK will fire the events when the unsent events count reaches 30 or the flush timer triggers every 30s.
  • When running the server-side example, if manually call amplitude.flush() or the event count reaches queue count max, the flush will be triggered, however, the scheduled flush trigger doesn't work.
  • Tried the same branch richardpiazza:server-side-swift with iOS UI example, the scheduled flush trigger works as expected.

@richardpiazza let us know if you have any clue on this, thanks!

@richardpiazza
Copy link
Author

richardpiazza commented Mar 2, 2023

I think I traced why the events wouldn't have been sending... It boils down to Timeline.process(event:) line 48:

if !(self.amplitude?._inForeground ?? false) {
    // This just gets called repeatedly.
    sessionEvents = self.amplitude?.startOrContinueSession(timestamp: eventTimeStamp)
} else {
    _ = self.amplitude?.refreshSessionTime(timestamp: eventTimeStamp)
}

On iOS and macOS there are plugins that track the lifecycle and indicate when the app is in the foreground. When running server-side, an application would always be considered foreground.

To work around this, I created a ForegroundLifecycle class that simply calls the internal func amplitude.onEnterForeground(timestamp:). Once done, then the 'startOrContinueSession' doesn't just get called repeatedly.


Might need to look at that logic or expose the ability to manually start a session (Amplitude.startSession())

@liuyang1520
Copy link
Collaborator

Sorry, I think events-not-sending-issue is for all events, not just for the session events. More details:

  • We have QueueTimer, it calls the handler every 30s, with DispatchSourceTimer
  • So flush should be running every 30s (default value), this is configured in the EventPipeline

I just pull the latest code with the plugin, think the Timer is still not working in my local. The Swift web server is running in the terminal, but there is no events sent out (once the total unsent events count reaches 30, it will trigger the flush too). Curious does the similar issue happen in your development environment? Thank you!

@richardpiazza
Copy link
Author

I dug further into the QueueTimer. It's very odd that the DispatchSourceTimer wasn't functioning as expected. I first tried to re-work that using a scheduled Timer (NSTimer), but that seems to have the same problems of not executing the expected block. Since the minimum supported versions of the sdk and the swift tools version calls for 5.7, I updated the solution to use a repeating async task.

Task {
    repeat {
        try? await Task.sleep(nanoseconds: UInt64(interval) * 1_000_000_000)
        fire()
    } while (repeating)
    // If the timer doesn't repeat, mark as 'suspended' after firing.
    suspend()
}

When suspended, the task is canceled and nullified. On resume, the task is recreated and runs once/repeatedly depending on the timers parameters.

I tested this on iOS, macOS, and Linux and seems to work across all platforms (plus it's using a swift language feature!)

Also, I reverted the addition of the ForegroundLifecycle as that did not seem to be needed with the corrected timer implementation.

@liuyang1520
Copy link
Collaborator

Thank you @richardpiazza! Seems github is having issue right now, the actions cannot be executed. I will give it a try later to see.

image

@liuyang1520
Copy link
Collaborator

@richardpiazza , sorry for the delay, and thank you for looking into the solution! I tried the new code locally, and I think the Timer is still not working for me, it doesn't automatically flush event. I ran the server in my Xcode (Version 14.2 (14C18)).

Steps to reproduce:

  • Update configure with my API key and logging configures
let amplitude = Amplitude(
    configuration: Configuration(
        apiKey: "xxx",
        flushIntervalMillis: 5000,
        logLevel: LogLevelEnum.DEBUG,
        callback: { (event: BaseEvent, code: Int, message: String) -> Void in
            print("eventcallback: \(event), code: \(code), message: \(message)")
        }
    )
)
  • Run the server with Xcode by clicking on the build button
  • Go to browser to trigger the route /hello
  • Wait on the Xcode for the flush

Result:

  • No logs output, seems no flush call
  • Check on Amplitude, no events are received

image

image

Would you mind sharing your configuration or steps to run the server-side app with events sent successfully?

Thanks!

@richardpiazza
Copy link
Author

@liuyang1520 Oh wow, entirely a screwup on my part. I only was confirming that the fire() was executing and not the rest of the chain.

I have finally identified exactly why the flush() hasn't been working though. It boils down to the use of the .main DispatchQueue. In a server (windowless/executable) environment, the normal CGD event loop may not be used. So any dispatches to the .main queue were being stacked up and never executed. This of course is problem 😄!

The most direct fix would be to use a .global() queue in place of the .main queue. The only issue here would be any unexpected reliance and having code executed on the .main queue when the timers fire. (Something that can be explored later and may become a bit more explicit as more Swift Concurrency features are adopted.) So, in order to avoid unintended bugs, the direction I chose was to be explicit about the fact of running on a server. I added a configuration flag: Configuration.runningOnServer. This defaults to false, and would need to be set on server instances (updated the server example & readme).

One other direction I explored was do pick a queue based on the environment, but I couldn't find a direct way of detecting in the execution was as part of a 'normal' application or a server.

static var queue: DispatchQueue {
    #if os(Linux)
    return .global()
    #elseif os(macOS)
    // running in application window or executable/server?
    return ?
    #else
    return .main
    #endif
}

I'd be happy to continue exploring other options as well.

@liuyang1520
Copy link
Collaborator

@richardpiazza good job on finding the root cause for the timer! I pulled the code and run the server example, it does fire the events periodically, I think the cause is identified here.

Agree with you that it might cause unexpected issues when modifying the queue to .global for other runtimes. Generally we don't do this diverging configuration for the SDK runtimes, and it is not align with our next-generation SDK interfaces. I will ask team for feedback on the potential solutions:

  1. use global queue (concurrent) instead of main, probably need more testing on all platforms, worry about the race conditions for the operations (or maybe we are good with some modifications)
  2. use macro to find out whether the current runtime is server-side use case or not, need more investigation on this, think it is also possible to run the server on iOS
  3. use a configuration option to control the queue in the runtime
  4. use the main queue as before, but require to call flush manually

Let me know your thoughts on this or any new option you found.

Thanks!

@richardpiazza
Copy link
Author

@liuyang1520 Sorry about the delay in getting back to you. It's been a busy week. Were you and your team able to have any discussions on the topics raised? I'll give a little feedback below, but I'll yield to the teams direction for the project.

  1. use global queue (concurrent) instead of main, probably need more testing on all platforms, worry about the race conditions for the operations (or maybe we are good with some modifications)
  2. use macro to find out whether the current runtime is server-side use case or not, need more investigation on this, think it is also possible to run the server on iOS
  3. use a configuration option to control the queue in the runtime
  4. use the main queue as before, but require to call flush manually
  1. Using DispatchQueue.global is an option, but without further diving into the specifics of the entire framework, I can't determine what else might be affected here. (more thoughts below)
  2. It is true that you can run in a 'server' mode on iOS, but everything on iOS would be running with the normal GCD event loop, and therefore DispatchQueue.main would word as expected. A runtime macro would be awesome, but I'm unable to propose a solution at the moment to identify macOS in an Application context vs a Server context.
  3. This is the option currently implemented in the PR. It requires anyone using the SDK to be explicit about that fact.
  4. Manually requiring a flush seems like a bad idea. It would expose to much of the inner workings of the sdk to the client.

I think one way to work around needing to specify the DispatchQueue for the queue timer would be to adopt more of the newer Swift Concurrency features. Task, async/await & Actors could all play a part. Though, I think that would involve a larger refactor and is far beyond the scope of what this PR is trying to achieve.

The runtime switch seems like a good stepping stone, but should ultimately be considered temporary.

@GeoffreyCzajkowskiK
Copy link

Hey there richardpiazza,

I see that you have made some updates to the Amplitude library to support server-side Swift on Linux. That's great news, as it expands the reach of the library and makes it even more versatile.

@richardpiazza
Copy link
Author

Hello. Just wondering if there has been any further internal discussion on this 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.

3 participants