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

Manually generated UserAgent #353

Merged
merged 7 commits into from
Oct 19, 2020
Merged

Conversation

brototyp
Copy link
Member

@brototyp brototyp commented Oct 4, 2020

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
Copy link

brototyp-bot commented Oct 4, 2020

1 Warning
⚠️ Are there any changes that should be explained in the README.md?

Generated by 🚫 Danger

@Findus23 Findus23 requested a review from sgiehl October 4, 2020 10:27
@Findus23
Copy link
Member

Findus23 commented Oct 4, 2020

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
Copy link
Member Author

brototyp commented Oct 4, 2020

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.

@brototyp brototyp force-pushed the feature/manual-generated-useragent branch from 141ae43 to 9e02982 Compare October 4, 2020 10:39
@sgiehl
Copy link
Member

sgiehl commented Oct 4, 2020

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
Copy link
Member

Findus23 commented Oct 4, 2020

@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
Copy link
Member Author

brototyp commented Oct 4, 2020

@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.

  1. Adding support for the macOS and tvOS formats to the devicedetector.
  2. 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
Copy link

gereons commented Oct 5, 2020

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
Copy link

gereons commented Oct 8, 2020

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
Copy link
Member Author

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 brototyp merged commit cd4804e into develop Oct 19, 2020
@brototyp
Copy link
Member Author

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

@brototyp brototyp deleted the feature/manual-generated-useragent branch October 19, 2020 16:36
@Guiboune
Copy link

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
Copy link
Member Author

Hey @Guiboune, this was just released in version 7.3

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.

6 participants