WIP Support HttpClient based scenarios #25

wyrzyk · 2019-10-25T11:40:13Z

When JPT detects a regression and the regression is on a backend side, then
it's handy to test only the backend side, to avoid the browser's noise.
I had this problem twice and two persons asked me about the feature. As there was
no dedicated API, I've been creating a custom scenario, and translate web driver
to HttpClient inside of a custom action.

This approach has two disadvantages:

It requires expert-level knowledge about JPT
Each VU needs to run a real browser, even if it's not used. It limits the

The goal of this change is to expose API for lightweight HttpClient based scenarios.

wyrzyk · 2019-10-25T11:58:38Z

src/main/kotlin/com/atlassian/performance/tools/virtualusers/HttpClientWebDriver.kt

+    }
+
+    fun initHttpClient(userName: String, password: String) {
+        synchronized(lock) {


I'm not sure If It's needed.

wyrzyk · 2019-10-25T11:59:44Z

...st/kotlin/com/atlassian/performance/tools/virtualusers/api/scenarios/HttpClientScenarioIT.kt

+
+        loadTest.run()
+
+        TODO("assert")


How can I retrieve action metrics?

When JPT detects a regression and the regression is on a backend side, then it's handy to test only the backend side, to avoid the browser's noise. I had this problem twice and two persons asked me about the feature. As there was no dedicated API, I've been creating a custom scenario, and translate web driver to HttpClient inside of a custom action. This approach has two disadvantages: - It requires expert-level knowledge about JPT - Each VU needs to run a real browser, even if it's not used. It limits the The goal of this change is to expose API for lightweight HttpClient based scenarios.

lipinskipawel · 2019-10-28T09:52:55Z

...main/kotlin/com/atlassian/performance/tools/virtualusers/api/scenarios/HttpClientScenario.kt

+import java.net.URI
+import java.util.concurrent.Future
+
+interface HttpClientScenario {


Is it possible to expose only the new implementation of Scenario which uses HttpClient? Something similar to JiraCoreScenario. In that case, our clients can decide which implementation to use without changing the interface.

In the current API, the client can choose both scenario and browser. HttpClientScenario binds browser with a scenario, and the new API is needed:
Scenario <---> Webdriver based browser
HttpClientScenario <----> HttpClient (the dependency is hidden in implementation)

lipinskipawel · 2019-10-28T09:58:39Z

src/main/kotlin/com/atlassian/performance/tools/virtualusers/api/config/VirtualUserBehavior.kt

+        )
+
+        private fun getScenario(): Class<out Scenario> {
+            HttpClientScenarioAdapter.scenarioClass = httpClientScenario


Here you are manually managing the state of Adapter. Is there a place to improve?
e.g. what if httpClientScenario is null?

It's never null. But it's a global! 😱 Global state mutations not only are hard to understand, not only are hostile to concurrency, but also do not work across JVMs. We need a test, which runs across JVMs. Ie. a shadow JAR is ran via java -jar ... rather than calling main directly from JUnit.

@dagguh You're right I have a wrong test that didn't catch this problem!

lipinskipawel · 2019-10-28T10:01:36Z

src/main/kotlin/com/atlassian/performance/tools/virtualusers/HttpClientWebDriver.kt

+import java.util.concurrent.Future
+import java.util.logging.Level
+
+internal class HttpClientWebDriver : RemoteWebDriver() {


This file could be deleted when we decide to expose only a new implementation of Scenario (which uses HttpClient internally).

dagguh · 2019-10-29T13:19:54Z

a518658:

When JPT detects a regression and the regression is on a backend side, then
it's handy to test only the backend side, to avoid the browser's noise.

ActionMetric.drilldown already contains HTTP-level data.

Reductio ad absurdum:

When JPT detects a regression and the regression is on the REST endpoint side, then it's handy to test only the REST Java methods, to avoid browser/network/CPU/JVM/IO/Tomcat/servlet/Jersey noise.

Should we add JMH microbenchmark API just because someone considers some factors as noise?

In other words, instead of recapturing the same metrics multiple times with different tools, just collect all of them and cross-reference them to filter out the noise in post-processing..

dagguh · 2019-10-29T13:33:41Z

The only good reason for HTTP-based traffic is when customer's Jira is actually being used by non-frontend, e.g. bots or scripts: https://ecosystem.atlassian.net/browse/JPERF-126

wyrzyk · 2019-10-29T13:40:58Z

@dagguh You're right. We don't cover REST APIs in our performance tests. Not all the APIs are used from the front end.

dagguh · 2019-10-29T13:40:44Z

src/main/kotlin/com/atlassian/performance/tools/virtualusers/api/config/VirtualUserBehavior.kt

+        )
+
+        private fun getScenario(): Class<out Scenario> {
+            HttpClientScenarioAdapter.scenarioClass = httpClientScenario


It's never null. But it's a global! 😱 Global state mutations not only are hard to understand, not only are hostile to concurrency, but also do not work across JVMs. We need a test, which runs across JVMs. Ie. a shadow JAR is ran via java -jar ... rather than calling main directly from JUnit.

dagguh · 2019-10-29T13:50:13Z

src/main/kotlin/com/atlassian/performance/tools/virtualusers/HttpClientWebDriver.kt

+        return httpClient
+    }
+
+    fun initHttpClient(userName: String, password: String) {


After construction, the object should be fully functional. "init" and "set" methods introduce mutability, which introduce unnecessary concurrency and readability problems.

Create the client externally, pass it via the constructor.

dagguh · 2019-10-29T13:52:24Z

src/main/kotlin/com/atlassian/performance/tools/virtualusers/HttpClientWebDriver.kt

+    }
+
+    override fun <X : Any?> getScreenshotAs(outputType: OutputType<X>?): X {
+        return outputType!!.convertFromBase64Png("")


RemoteWebDriver doesn't do null checks. We can also skip that by declaring the parameter as non-null.

dagguh · 2019-10-29T14:01:48Z

src/main/kotlin/com/atlassian/performance/tools/virtualusers/HttpClientWebDriver.kt

+import java.util.concurrent.Future
+import java.util.logging.Level
+
+internal class HttpClientWebDriver : RemoteWebDriver() {


HTTP client pretending to be a WebDriver sounds like a good way to introduce a ton of mental overhead and bugs. We're promising abilities to take screenshots, take page source, access elements, etc. We fail most of these promises in runtime. This fake complexity spreads outwards, infecting the Browser and VirtualUserBehavior APIs.

Instead, we should generalize existing APIs. I didn't dig deep, but it seems the common structure is:

actual traffic generator - WebDriver or HTTP client
** gotta be closeable
** injectable supplier - Browser SPI for WD, a Supplier<CloseableHttpClient> for HTTP

diagnostics - screenshot/page source/URL for WD, HTTP request/response dump for HTTP?

It's an internal debt, but you're right, let's try to generalize the APIs instead of trying to fit into the existing one.

wyrzyk · 2019-11-04T08:22:54Z

src/main/kotlin/com/atlassian/performance/tools/virtualusers/api/config/VirtualUserBehavior.kt

    ): VirtualUserBehavior = Builder(this).load(load).build()

+    @Deprecated("TODO - can we provide more type safe builder?")
+    // TODO detect when the new scenario is used with parameters that are no longer supported in new Scenario


IMO we should keep the old builder and introduce a new one, without the non-supported parameters.

dagguh · 2019-11-04T09:25:26Z

src/main/java/com/atlassian/performance/tools/virtualusers/lib/api/Scenario.java

+import java.util.List;
+
+/**
+ * IT doesn't need to be thread safe. Each VU will have own copy of the scenario and own Thread


We should mark it as @NotThreadSafe
We could mention it's thread-confined

dagguh · 2019-11-04T09:30:00Z

src/main/java/com/atlassian/performance/tools/virtualusers/lib/api/Scenario.java

+/**
+ * IT doesn't need to be thread safe. Each VU will have own copy of the scenario and own Thread
+ */
+public abstract class Scenario {


It's not an interface anymore, so we don't have the Kotlin default method problems, so we can write it in Kotlin

dagguh · 2019-11-04T09:33:01Z

src/main/kotlin/com/atlassian/performance/tools/virtualusers/api/EntryPoint.kt

+            } else if (scenarioPackage == "com.atlassian.performance.tools.jiraactions.api.scenario") {
+                LoadTest(options).run()
+            } else {
+                throw Exception("not implemented")


Printing the unknown package name would help

dagguh · 2019-11-04T09:33:58Z

src/main/kotlin/com/atlassian/performance/tools/virtualusers/api/EntryPoint.kt

        } else {
-            LoadTest(options).run()
+            val scenarioPackage = options.behavior.scenario.packageName
+            if (scenarioPackage == "com.atlassian.performance.tools.virtualusers.lib.api") {


dagguh · 2019-11-04T11:47:45Z

src/main/kotlin/com/atlassian/performance/tools/virtualusers/NewExploratoryVirtualUser.kt

+ * Applies load on a Jira via page objects. Explores the instance to learn about data and choose pages to visit.
+ * Wanders preset Jira pages with different proportions of each page. Their order is random.
+ */
+internal class NewExploratoryVirtualUser(


The new Scenario should be able to express old features. So we should be able to write a bridge from old to new Scenario and only consume the new one. No need to copy all of the internals.

Copy-paste is just an intermediate state. I didn't want to be focused on backwards compatibility while designing the new API. I'm going to refactor it later.

dagguh · 2019-11-04T11:57:00Z

src/test/kotlin/com/atlassian/performance/tools/virtualusers/SimpleWebdriverScenario.kt

+        limit: Int = Int.MAX_VALUE
+    ) : Action {
+        companion object {
+            private val limitCounter = AtomicInteger(Int.MAX_VALUE)


We didn't have a global limit counter before. Each VU had its own limit.

dagguh · 2019-11-04T12:13:11Z

src/test/kotlin/com/atlassian/performance/tools/virtualusers/SimpleWebdriverScenario.kt

+        driver.close()
+    }
+
+    override fun getActions(): List<Action> {


We need a pattern for partially reusable/customizable scenarios. E.g. reuse this getActions, but with a different Browser.
I imagine, we could inject a Browser like this:

open class SimpleWebdriverScenario( private val browser: Browser, private val target: VirtualUserTarget, private val meter: ActionMeter ) : Scenario( target, meter ) { constructor( target: VirtualUserTarget, meter: ActionMeter ) : this( HeadlessChromeBrowser(), target, meter ) } class CustomizedWebdriverScenario( private val target: VirtualUserTarget, private val meter: ActionMeter ) : SimpleWebdriverScenario( FirefoxBrowser(), target, meter )

Alternatively we could extract each method into a "util" and let the user stitch a new scenario class from these components.
WDYT @wyrzyk?

We can support the injection of the browser, but it makes API focused on one load generator type. The new VUB builder will have to support Browser parameter which will be only usable in this kind of scenarios.

If we look at how the browser has been changed during the last years in our scenarios/tests, it was mainly bumping the version and one switch from chrome to chromium. We never needed the flexibility of decoupled load generator and scenario.

Beside generic scenarios, we provide in jira-actions and jira-software-actions scenario is part of the test code. We have a couple of different scenarios in our internal tests. For our internal tests, it's even easier to create an instance of a browser instead of passing it via the builder.

It also makes testing scenarios easier because the class provides all the capabilities to perform the test. All that needs to be provided is VUT and action meter.

If we really want to provide a way to reuse scenario with a different browser, then it doesn't have to be simple. It's not the main or common use case. We can put all the responsibility to provide the cross-browser scenario on the client's code. We could just provide passing properties to VU. It could look like:

class SimpleCrossBrowserScenario( private val target: VirtualUserTarget, private val meter: ActionMeter ) : Scenario( target, meter ) { val driver = SystemPropertyDriverFactory().getDriver() } class SystemPropertyDriverFactory { fun getDriver(): WebDriver { return when (System.getProperty("browser")) { "firefox" -> new Firefox () "chrome" -> new Chrome () } } }

It removes the mental overhead of the browser from the basic usages of VUB.

Creator of the scenario controls supported browsers/load generators (just by using a common webdriver API there's no guarantee the scenario will run with all the browsers and all the configurations).

It's up to the creator of the scenario to provide API (properties needed to configure load generator).

It can be used for different's kind of generators. For example, to provide for HTTP based generators on HttpClient and OkHttp. For DB based generators (different JDBC implementations) etc.

Properties can also be used to pass different information like admin credentials. It doesn't have to be part of VU API. It can be a Scenario's API. It's hard to provide a generic way for authorisation.

I didn't mention VUB or VUT. Did you see my code snippet suggestion?

Ok, I see now. We always use 2 parameters constructor, so there's no VUB or VUT problem. This way it's easier to use different browsers, but still, we don't have evidence that the feature is needed.

It's also more prone to errors. It's hard to enforce inheritance only the specific way.
This will not work as expected:

class CustomizedWebdriverScenario( private val browser: Browser, private val target: VirtualUserTarget, private val meter: ActionMeter ) : SimpleWebdriverScenario( browser, target, meter )

Yep, it could be a WTF moment.

It's also more prone to errors.

More prone to errors in comparison to SystemPropertyDriverFactory?
How would we pass the properties to the VU JVMs?

It's more prone to errors because it adds WTF moment to the basic scenario implementation journey instead of adding WTF moment to an imaginary scenario without a real need.

More prone to errors in comparison to SystemPropertyDriverFactory?
How would we pass the properties to the VU JVMs?

SystemPropertyDriverFactory is just one example of implementation. We can pass params via VUB (command line parameters).

We can also use the properties object as a 3rd parameter to the scenario. It would be passed via VUB to the scenario and will allow scenarios to implement own API (for example. load generators support, credentials support).

dagguh · 2019-11-04T12:13:45Z

src/test/kotlin/com/atlassian/performance/tools/virtualusers/SimpleWebdriverScenario.kt

+        adminPassword = "admin"
+    )
+
+    override fun before() { //todo don't use hardcoded credentials


todo don't use hardcoded credentials

dagguh · 2019-11-04T12:24:51Z

src/test/kotlin/com/atlassian/performance/tools/virtualusers/SimpleWebdriverScenario.kt

+import com.atlassian.performance.tools.virtualusers.lib.api.Scenario
+import java.util.concurrent.atomic.AtomicInteger
+
+// TODO show that RTE can be disabled via setup


TODO show that RTE can be disabled via setup

dagguh · 2019-11-04T12:26:25Z

src/test/kotlin/com/atlassian/performance/tools/virtualusers/SimpleWebdriverScenario.kt

+) : Scenario(
+    virtualUserTarget, meter
+) {
+    private val driver: CloseableRemoteWebDriver = HeadlessChromeBrowser().start()


Constructors shouldn't do real work or heavy-lifting like starting processes.
I might be able to create an instance, but never reach before or getActions (due to an if or a throw).

dagguh · 2019-11-04T12:36:13Z

src/test/kotlin/com/atlassian/performance/tools/virtualusers/api/NewScenarioApiSupportIT.kt

+                    password = "admin"
+                ),
+                behavior = VirtualUserBehavior.Builder(
+                    SimpleWebdriverScenario::class.java


We should also have an example of a HTTP scenario

wyrzyk requested a review from dagguh October 25, 2019 11:40

wyrzyk force-pushed the http-client-based-scenario branch from de4c564 to ab4acd2 Compare October 25, 2019 11:43

wyrzyk requested review from lipinskipawel and sebapawlak October 25, 2019 11:51

wyrzyk force-pushed the http-client-based-scenario branch from ab4acd2 to b9028b5 Compare October 25, 2019 11:54

wyrzyk commented Oct 25, 2019

View reviewed changes

wyrzyk force-pushed the http-client-based-scenario branch from b9028b5 to 05b4303 Compare October 25, 2019 12:24

wyrzyk force-pushed the http-client-based-scenario branch from 05b4303 to a518658 Compare October 25, 2019 12:28

lipinskipawel reviewed Oct 28, 2019

View reviewed changes

dagguh suggested changes Oct 29, 2019

View reviewed changes

wyrzyk force-pushed the http-client-based-scenario branch from 02d7e27 to d8d14fb Compare October 31, 2019 15:29

wip generalise api

6699a05

wyrzyk force-pushed the http-client-based-scenario branch from d8d14fb to 6699a05 Compare October 31, 2019 15:33

wyrzyk added 3 commits October 31, 2019 16:57

Add diagnostics to SimpleWebdriverScenario

3aa1aae

Limit diagnostics in SimpleWebdriverScenario

02448bf

add todo

3e455a6

wyrzyk commented Nov 4, 2019

View reviewed changes

dagguh suggested changes Nov 4, 2019

View reviewed changes

dagguh reviewed Nov 4, 2019

View reviewed changes

WIP Support HttpClient based scenarios #25

Are you sure you want to change the base?

WIP Support HttpClient based scenarios #25

Uh oh!

Conversation

wyrzyk commented Oct 25, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dagguh commented Oct 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dagguh commented Oct 29, 2019

Uh oh!

wyrzyk commented Oct 29, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wyrzyk Nov 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dagguh Nov 4, 2019 • edited by wyrzyk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dagguh commented Oct 29, 2019 •

edited

Loading

wyrzyk Nov 5, 2019 •

edited

Loading

dagguh Nov 4, 2019 •

edited by wyrzyk

Loading