Manually generated UserAgent

brototyp commented 4 years ago

I think we need to rethink our UserAgent generation. Right now the SDK asks the WebView to generate the UserAgent and will then replace iPhone with the actual iPhone version. There are just to many issues with it:

WebKit as a dependency
Asynchronous code: There is no synchronous way to let a WKWebView generate the UserAgent
When the format of the UserAgent returned by WkWebView changes our code might break (I guess that's what is happening here)

This PR changes this and completely manually generates the UserAgent. The new, generated format is iOSExampleApp/1.0 iPhone10,4 iOS/13.3 MatomoTrackerIOSSDK/7.2.2 Darwin/18.7.0.

iOSExampleApp/1.0 display name and marketing version of the app
iPhone10,4 device identifier
iOS/13.3 iOS Version
MatomoTrackerIOSSDK/7.2.2 Matomo SKD version
Darwin/18.7.0 Darwin version

According to the test website by @Findus23, Matomo parses this format perfectly well: have a look

brototyp-bot commented 4 years ago

	1 Warning
:warning:	Are there any changes that should be explained in the `README.md`?

Generated by :no_entry_sign: Danger

Findus23 commented 4 years ago

It would be great if @sgiehl could take a quick look to make sure this method makes sense from a device-detector point of view.

brototyp commented 4 years ago

That would be great. It looks like we don't parse it to well if we use the SDK in a Mac app. This is how it looks like.

Edit: The same goes for the recognition when run on tvOS.

sgiehl commented 4 years ago

It would be great if @sgiehl could take a quick look to make sure this method makes sense from a device-detector point of view.

Looks good from my side. Looks like a typical useragent of a mobile app...

Findus23 commented 4 years ago

@brototyp For the MacOS strings, I guess they could be added to devicedetector, but that would mean only Matomo 4 would detect them properly.

brototyp commented 4 years ago

@brototyp For the MacOS strings, I guess they could be added to devicedetector, but that would mean only Matomo 4 would detect them properly.

Hm. I guess both would be great.

Adding support for the macOS and tvOS formats to the devicedetector.
Finding a way that the older devicedetector and thus matomo < 4 are supported. I noticed that once I add Safari/605.1.15 to the UserAgent String it is (at least) partially recognized. But that's actually not correct. We are not sending these events using Safari. And in order for the OS version to be recognized we also have to add Mac OS X/10_14_6 to the UserAgent.

What do you think about if we add the MatomoTrackerSDK as a client to the devicedetector and I add a Safari/0.0 so it is recognized for the older versions in the meantime?

gereons commented 4 years ago

With the exception of one minor issue, this looks good to me!

The issue is in matomoSDKVersionForCurrentApplication() - when an app links its pods statically (using use_frameworks! :linkage => :static in the Podfile), the bundle for MatomoTracker.self will be the main bundle, and so the app's version will be duplicated as the Matomo SDK version.

gereons commented 4 years ago

Just to add another tidbit: When using Xcode 12/iOS 14, I see these log entries on app startup:

2020-10-08 12:46:56.215218+0200 MyApp[88734:3304849] [assertion] Error acquiring assertion: <Error Domain=RBSAssertionErrorDomain Code=3 "Target is not running or required target entitlement is missing" UserInfo={RBSAssertionAttribute=<RBSDomainAttribute| domain:"com.apple.webkit" name:"Background" sourceEnvironment:"(null)">, NSLocalizedFailureReason=Target is not running or required target entitlement is missing}>
2020-10-08 12:46:56.215359+0200 MyApp[88734:3304849] [ProcessSuspension] 0x107cfed00 - ProcessAssertion: Failed to acquire RBS Background assertion 'WebProcess Background Assertion' for process with PID 88736, error: Error Domain=RBSAssertionErrorDomain Code=3 "Target is not running or required target entitlement is missing" UserInfo={RBSAssertionAttribute=<RBSDomainAttribute| domain:"com.apple.webkit" name:"Background" sourceEnvironment:"(null)">, NSLocalizedFailureReason=Target is not running or required target entitlement is missing}

these disappear when using the code from this branch.

brototyp commented 4 years ago

With the exception of one minor issue, this looks good to me!

The issue is in matomoSDKVersionForCurrentApplication() - when an app links its pods statically (using use_frameworks! :linkage => :static in the Podfile), the bundle for MatomoTracker.self will be the main bundle, and so the app's version will be duplicated as the Matomo SDK version.

Oh, yeah. Great catch! I think for a static library, the only way for us is to add the version in the code somewhere. This or so.

extension MatomoTracker {
    static let sdkVersion = "7.2.2"
}

Any other idea?

brototyp commented 4 years ago

Alright. This is merged and will be part of the next release.

Guiboune commented 4 years ago

Alright. This is merged and will be part of the next release.

Thanks for the fix. Any idea (date) when it should be released ? days ? weeks ? (for production purpose)

brototyp commented 4 years ago

Hey @Guiboune, this was just released in version 7.3

matomo-org / matomo-sdk-ios

Manually generated UserAgent #353