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

[WICKET-6544] mobile browser detection is improved (with YAUAA) #275

Closed
wants to merge 5 commits into from

Conversation

Projects
None yet
4 participants
@klopfdreh
Copy link
Member

commented Apr 12, 2018

Hi all,

here my 5 cent regarding the browser detection with https://github.com/nielsbasjes/yauaa

you have to load the diff of the test to see the real benefit.

kind regards

Tobias

*/
private final String userAgent;

/** Client properties object. */
private final ClientProperties properties;

private final static UserAgentAnalyzer UAA = UserAgentAnalyzer.newBuilder()

This comment has been minimized.

Copy link
@svenmeier

svenmeier Apr 12, 2018

Contributor

UserAgentAnalyzer isn't reentrant. Since it is expensive to create one, instances have to be pooled.

This comment has been minimized.

Copy link
@klopfdreh

klopfdreh Apr 13, 2018

Author Member

The initialization is expensive, but the UserAgentAnalyzer is thread safe so it can be reused. Maybe we are able to use MetaDataKey

This comment has been minimized.

Copy link
@svenmeier

svenmeier Apr 13, 2018

Contributor

UserAgentAnalyzer#parse(UserAgent) is synchronized, please check its code. This is even mentioned on the project home page.

This comment has been minimized.

Copy link
@klopfdreh

klopfdreh Apr 13, 2018

Author Member

Have a look at the recent changes. A lot of stuff changed already and I think it is ok like it is now (except the tests)

setOperaProperties(parsedUserAgent);
setChromeProperties(parsedUserAgent);
setEdgeProperties(parsedUserAgent);
setSafariProperties(parsedUserAgent);

This comment has been minimized.

Copy link
@solomax

solomax Apr 13, 2018

Contributor

All these methods looks very much the same ....

This comment has been minimized.

Copy link
@solomax

solomax Apr 13, 2018

Contributor

can be replaced with
String agent = parsedUserAgent.getValue("AgentName");
properties.setBrowserKonqueror(UserAgent.KONQUEROR.getUaStrings().contains(agent));
properties.setBrowserChrome(UserAgent.CHROME.getUaStrings().contains(agent));
.................
setBrowserVersion(parsedUserAgent);

This comment has been minimized.

Copy link
@klopfdreh

klopfdreh Apr 13, 2018

Author Member

Hi @solomax - yes indeed - it was due to the refactoring and those methods were present before - I just refactored them.

@klopfdreh

This comment has been minimized.

Copy link
Member Author

commented Apr 13, 2018

Following changes have been made:

  • The init() method has been changed into gatherExtendedInfo() which is only be executed if org.apache.wicket.settings.RequestCycleSettings.setGatherExtendedBrowserInfo(boolean) is set to true
  • MetaDataKey has been used to initialize YAUAA
  • All browser specific methods have been moved into detectBrowserProperties
  • gatherExtendedInfo / detectBrowserProperties are protected / public so that the user may override them

The reason why I moved gatherExtendedInfo() from out of the constructor is, that YAUAA takes a long time for initialization and if not required might be a bit to much overhead.


// FireFox
boolean isFireFox = UserAgent.FIREFOX.getUaStrings()
.contains(parsedUserAgent.getValue("AgentName"));

This comment has been minimized.

Copy link
@solomax

solomax Apr 13, 2018

Contributor

userAgentName

else
{
properties.setBrowserMozilla(
UserAgent.MOZILLA.getUaStrings().contains(parsedUserAgent.getValue("AgentName")));

This comment has been minimized.

Copy link
@solomax

solomax Apr 13, 2018

Contributor

userAgentName

*/
@Test
public void testBrowsersAndVersionsOfUserAgents() {
// Internet Explorers

This comment has been minimized.

Copy link
@solomax

solomax Apr 13, 2018

Contributor

I would rather have all these as individual tests, to be able to see which one was failed
And run all of them regardless failure state of others

This comment has been minimized.

Copy link
@klopfdreh

klopfdreh Apr 13, 2018

Author Member

This might slow down the test execution because of the init of YAUAA - try it out.

This comment has been minimized.

Copy link
@klopfdreh

klopfdreh Apr 13, 2018

Author Member

You will see if something fails in the test results btw.

This comment has been minimized.

Copy link
@solomax

solomax Apr 13, 2018

Contributor

You can move init to @BeforeClass .....

This comment has been minimized.

Copy link
@solomax

solomax Apr 13, 2018

Contributor

All subsequent lines will not be executed in case of failure ....

This comment has been minimized.

Copy link
@klopfdreh

klopfdreh Apr 13, 2018

Author Member

Yes that is true, but I think if you change the code / the browser detection you are going to investigate why the test is failing and fix the integration as long as every test is back working again.

@BeforeClass is not working, because WicketTestCase and at this stage no Application is attached to the main thread.

This comment has been minimized.

Copy link
@klopfdreh

klopfdreh Apr 13, 2018

Author Member

May you have any other good idea to keep the tests individual.

Just send a patch to me if something would fit the requirement and I am going to integrate it.

This comment has been minimized.

Copy link
@solomax

solomax Apr 13, 2018

Contributor

Will try to provide patch later tonight
Have to do day time job right now :(

This comment has been minimized.

Copy link
@klopfdreh

klopfdreh Apr 13, 2018

Author Member

Hehe me too 😄 - no hurry, because I think this is going to be released in a future version.

This comment has been minimized.

Copy link
@solomax

solomax Apr 20, 2018

Contributor

Sorry for delay, here is the code works for me:

	private static ThreadLocal<UserAgentAnalyzer> localAnalyzer = new ThreadLocal<UserAgentAnalyzer>();

	@Before
	public void before()
	{
		requestCycleMock = mock(RequestCycle.class);

		webRequest = mock(ServletWebRequest.class);
		when(requestCycleMock.getRequest()).thenReturn(webRequest);

		servletRequest = mock(HttpServletRequest.class);
		when(webRequest.getContainerRequest()).thenReturn(servletRequest);

		if (localAnalyzer.get() == null) {
			WebClientInfo webClientInfo = new WebClientInfo(requestCycleMock, "test");
			webClientInfo.gatherExtendedInfo();
			localAnalyzer.set(Application.get().getMetaData(WebClientInfo.UAA_META_DATA_KEY));
		} else {
			Application.get().setMetaData(WebClientInfo.UAA_META_DATA_KEY, localAnalyzer.get());
		}
	}
	
	@AfterClass
	public static void staticAfter()
	{
		localAnalyzer.set(null);
	}

Here are the measurment results:

Combined tests
real	0m24.123s
user	1m30.748s
sys	0m1.804s


Individual tests (after above changes were applied)
real	0m22.844s
user	1m23.948s
sys	0m1.832s

{
String userAgent = getUserAgentStringLc();

This comment has been minimized.

Copy link
@solomax

solomax Apr 20, 2018

Contributor

getUserAgentStringLc is private and not used anymore

@klopfdreh

This comment has been minimized.

Copy link
Member Author

commented Apr 20, 2018

Hi @solomax - thanks a lot for the review. Because I am not able to work on it currently I am going to grant you access to my repo for further commits. Is this ok for you?

@solomax

This comment has been minimized.

Copy link
Contributor

commented Apr 20, 2018

OK :) I'll merge my changes

@klopfdreh

This comment has been minimized.

Copy link
Member Author

commented Apr 20, 2018

I send you an invitation to the repo in which the branch of the PR is located. just push to it and merge if you are done. I think we are on a good way with this integration.

@klopfdreh

This comment has been minimized.

Copy link
Member

commented on 299aa15 Apr 20, 2018

Great changes! 👍

@klopfdreh

This comment has been minimized.

Copy link
Member Author

commented Apr 26, 2018

What about merging this? Or do we proceed with this PR after the release @bitstorm ?

@svenmeier

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2018

My concerns still hold :(

Most importantly: with that single UserAgentAnalyzer instance you've introduced a bottleneck for all concurrent request that need to gatherExtendedInfo().

* Initializes the {@link WebClientInfo} user agent detection. This can be overridden to choose
* a different detection as YAUAA (https://github.com/nielsbasjes/yauaa) - if you do so, you
* might exclude the maven dependency from your project in favor of a different framework.
*/

This comment has been minimized.

Copy link
@svenmeier

svenmeier Apr 26, 2018

Contributor

No, you can't exclude the dependency - with UserAgentAnalyzer in the signature of #detectBrowserProperties(UserAgentAnalyzer) you will get a NoClassDefFoundError if the dependency is not present.

This comment has been minimized.

Copy link
@klopfdreh

klopfdreh Apr 26, 2018

Author Member

Ok, then keep the dependency and just override it. We have to adjust the documentation here.

@klopfdreh

This comment has been minimized.

Copy link
Member Author

commented Apr 26, 2018

To create UserAgentAnalyzer takes rather a long time. Maybe we are able to find a way to resolve the bottleneck, but my concerns to implement the browser detection in such a bad way like it was before also still hold.

Also when I see the benchmarks I would consider to just inform about the bottleneck and keep it that way, or to turn gatherExtendedInfo() off if 0.011ms are to much.

@nielsbasjes

This comment has been minimized.

Copy link

commented Jun 25, 2018

@klopfdreh
A few points about the slow startup.
When you explicitly specify which fields you really need the system will completely 'not load' the rules that have no impact. Especially if you do not need the DeviceBrand/DeviceName the impact will be quite large. You can simply pass something like this .withField("AgentName") .withField("DeviceClass") .withField("AgentVersion") and it will have a big impact on both startup times and run times for a not yet cached parse.

I had a look at your project and with this patch the startup time drops to less than 1.5 seconds.

15:30:01,824 [INFO ] YauaaVersion                            :   58: 
15:30:01,827 [INFO ] YauaaVersion                            :   59: /------------------------------------------------------------\
15:30:01,827 [INFO ] YauaaVersion                            :   85: | Yauaa 4.5 (v4.5 @ 2018-05-30T18:31:47Z)                    |
15:30:01,827 [INFO ] YauaaVersion                            :   61: +------------------------------------------------------------+
15:30:01,827 [INFO ] YauaaVersion                            :   85: | For more information: https://github.com/nielsbasjes/yauaa |
15:30:01,832 [INFO ] YauaaVersion                            :   85: | Copyright (C) 2013-2018 Niels Basjes - License Apache 2.0  |
15:30:01,832 [INFO ] YauaaVersion                            :   72: \------------------------------------------------------------/
15:30:01,832 [INFO ] YauaaVersion                            :   73: 
15:30:01,832 [INFO ] UserAgentAnalyzerDirect                 :  231: Loading from: "classpath*:UserAgents/**/*.yaml"
15:30:02,295 [INFO ] UserAgentAnalyzerDirect                 :  287: Loaded 69 files in 462 msec
15:30:02,299 [INFO ] UserAgentAnalyzerDirect                 :  307: Building all needed matchers for the requested 2 fields.
15:30:02,336 [INFO ] UserAgentAnalyzerDirect                 :  357: Loading  218 (dropped 2361) matchers, 27 lookups, 2 lookupsets, 0 testcases from   63 files took    34 msec
15:30:02,339 [INFO ] UserAgentAnalyzerDirect                 :  367: Initializing Analyzer data structures
15:30:02,847 [INFO ] UserAgentAnalyzerDirect                 :  372: Built in 506 msec : Hashmap 52919, Ranges map:2740

Note that it dropped (= skipped because no useful output) about 90% of all rules.

diff --git pom.xml pom.xml
index 00cc8f8..7910bd9 100644
--- pom.xml
+++ pom.xml
@@ -146,7 +146,7 @@
                        <dependency>
                            <groupId>nl.basjes.parse.useragent</groupId>
                            <artifactId>yauaa</artifactId>
-                           <version>4.2</version>
+                           <version>4.5</version>
                        </dependency>
                        <dependency>
                                <groupId>com.google.code.findbugs</groupId>
diff --git wicket-core/src/main/java/org/apache/wicket/protocol/http/request/WebClientInfo.java wicket-core/src/main/java/org/apache/wicket/protocol/http/request/WebClientInfo.java
index 5a02a3d..1e488e9 100644
--- wicket-core/src/main/java/org/apache/wicket/protocol/http/request/WebClientInfo.java
+++ wicket-core/src/main/java/org/apache/wicket/protocol/http/request/WebClientInfo.java
@@ -151,6 +151,9 @@ public class WebClientInfo extends ClientInfo
                {
                        userAgentAnalyzer = UserAgentAnalyzer.newBuilder()
                                .hideMatcherLoadStats()
+                               .dropTests()
+                               .withField("AgentName")
+                               .withField("AgentVersion")
                                .withCache(25000)
                                .build();
                        Application.get().setMetaData(UAA_META_DATA_KEY, userAgentAnalyzer);
@@ -217,7 +220,7 @@ public class WebClientInfo extends ClientInfo
         */
        protected void setBrowserVersion(nl.basjes.parse.useragent.UserAgent parsedUserAgent)
        {
-               String value = parsedUserAgent.get("AgentVersion").getValue();
+               String value = parsedUserAgent.getValue("AgentVersion");
                if (!"Hacker".equals(value))
                {
                        properties.setBrowserVersion(value);
@nielsbasjes

This comment has been minimized.

Copy link

commented Jun 25, 2018

On a more general note: I think the DeviceClass field might be useful for Wicket as it indicates if the device is a phone, tablet or desktop.

@svenmeier

This comment has been minimized.

Copy link
Contributor

commented Jun 25, 2018

Hi Niels,
thanks for chiming in, but just today we decided to drop all code related to user agent detection, as Wicket itself doesn't need/utilizes it.
Actually your remarks affirm my position that user agent detection is hard business and Wicket shouldn't hide the details and configuration behind our very broad API.

@solomax

This comment has been minimized.

Copy link
Contributor

commented Jun 25, 2018

I believe we can add browser detection as wicketstuff module and/or as confluence example
Should be easy and useful

@klopfdreh

This comment has been minimized.

Copy link
Member Author

commented Jun 27, 2018

Due to the vote that Wicket is going to drop the support of Browser detection I am closing this branch right now.

@solomax - yes why not a wicketstuff module. 👍

/closed

@klopfdreh klopfdreh closed this Jun 27, 2018

@solomax

This comment has been minimized.

Copy link
Contributor

commented Jul 16, 2018

@klopfdreh can you create wicketstuff module?

@klopfdreh

This comment has been minimized.

Copy link
Member Author

commented Jul 17, 2018

Hey @solomax - sadly I am totally busy right now, but I try my best to change this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.