Flank needs to respect the timeout value as that's a cap for billing purposes. #865

adamfilipow92 · 2020-06-23T14:32:08Z

Fixes #836

Flank billing calculation rules:

test execution time is rounded up to next minute and used in calculations

Execution time	Rounded value
`3m 22s`	`4m`
`15m 01s`	`16m`
`5m 00s`	`5m`

if test execution time is smaller then test timeout (default 15m) round up execution time and use it in billing calculations
if test execution time is bigger then test timeout use timeout value in billing calculations
if test execution state is ERROR flank should skip billing calculation and assume $0.00

Examples:

execution time is 13m 24s and timeout is default (15m), execution time is smaller then timeout therefor is rounded up to 14m and used in billing calculations
execution time is 20m 13s and timeout is set to 18m, execution time is bigger then timeout therefor timeout value (18m) is used in billing calculations

Test Plan

How do we know the code works?

Flank needs to respect the timeout value as that's a cap for billing purposes, and round time to nearest minutes.

Run flank with yml


gcloud:
  app: ./src/test/kotlin/ftl/fixtures/tmp/apk/app-debug.apk
  robo-script: ./src/test/kotlin/ftl/fixtures/test_app_cases/MainActivity_robo_script.json
  environment-variables:
    coverage: true
    coverageFilePath: /sdcard/
    clearPackageData: true

flank should return cost report with 2 min


CostReport
  Virtual devices
    $0.03 for 2m

Add in gcloud section timeout for 60s


gcloud:
  app: ./src/test/kotlin/ftl/fixtures/tmp/apk/app-debug.apk
  robo-script: ./src/test/kotlin/ftl/fixtures/test_app_cases/MainActivity_robo_script.json
  environment-variables:
    coverage: true
    coverageFilePath: /sdcard/
    clearPackageData: true
  timeout: 60s

flank should return cost report with 1 min


CostReport
  Virtual devices
    $0.02 for 1m

To check timeout is respected only when is lower than execution time set timeout to 5m and run


gcloud:
  app: ./src/test/kotlin/ftl/fixtures/tmp/apk/app-debug.apk
  robo-script: ./src/test/kotlin/ftl/fixtures/test_app_cases/MainActivity_robo_script.json
  environment-variables:
    coverage: true
    coverageFilePath: /sdcard/
    clearPackageData: true
  timeout: 5m

flank should return cost report with 2 min


CostReport
  Virtual devices
    $0.03 for 2m

Checklist

Documented
Unit tested
release_notes.md updated

bootstraponline · 2020-06-23T16:30:55Z

Cost report should contains seconds not rounded up minutes.

Hey, this is not a correct description of the problem. FTL does round to the nearest minute and that's how they bill.

The problem is that Flank needs to respect the timeout value as that's a cap for billing purposes.

Flank is over-reporting billable minutes for tests that hit their timeout limit. FTL caps the billed time at the timeout value, but Flank seems to report total time (e.g. a Robo test has a 1 minute timeout, but actually runs 75sec before the cancellation happens. FTL bills for 1m, but Flank says 2m.

adamfilipow92 · 2020-06-24T13:22:09Z

Cost report should contains seconds not rounded up minutes.

Hey, this is not a correct description of the problem. FTL does round to the nearest minute and that's how they bill.

The problem is that Flank needs to respect the timeout value as that's a cap for billing purposes.

Flank is over-reporting billable minutes for tests that hit their timeout limit. FTL caps the billed time at the timeout value, but Flank seems to report total time (e.g. a Robo test has a 1 minute timeout, but actually runs 75sec before the cancellation happens. FTL bills for 1m, but Flank says 2m.

Thanks for fast feedback! I check rounding time strategy and flank rounding time always up. Now if we have 1m 7s flank round it to 2 mins. I think this is problem too because flank should round 1m 7s to 1m right?

bootstraponline · 2020-06-24T14:37:42Z

Thanks for fast feedback! I check rounding time strategy and flank rounding time always up. Now if we have 1m 7s flank round it to 2 mins. I think this is problem too because flank should round 1m 7s to 1m right?

Firebase will bill us for 2m if a shard runs for 1m 7s. The only exception is if there's a timeout for 1m, then we'll only ever be billed up to the timeout.

pawelpasterz · 2020-06-24T15:03:08Z

Thanks for fast feedback! I check rounding time strategy and flank rounding time always up. Now if we have 1m 7s flank round it to 2 mins. I think this is problem too because flank should round 1m 7s to 1m right?

Firebase will bill us for 2m if a shard runs for 1m 7s. The only exception is if there's a timeout for 1m, then we'll only ever be billed up to the timeout.

agreed, flank needs to take timeout value under consideration.
This implementation will be improved :)

pawelpasterz · 2020-06-24T19:15:11Z

Description updated

bootstraponline · 2020-06-24T21:58:19Z

Feedback from Paul Davis via Slack:

That looks correct, as long as you also report zero billing for all infrastructure errors. Unfortunately, infra errors can be marked on either the last attempt within an execution, or on the execution itself.
In FTL billing logic, that looks like

if (!execution.attempts().isEmpty()) {
    Attempt lastAttempt = execution.attempts().get(execution.attempts().size() - 1);
    if (lastAttempt.getAttemptStatus() != AttemptStatus.ERROR && execution.state() != TestState.ERROR) {
        // do rounding/timeout calculations as described in the pull request
    }
}

pawelpasterz · 2020-06-25T03:31:52Z

Feedback from Paul Davis via Slack:

That looks correct, as long as you also report zero billing for all infrastructure errors. Unfortunately, infra errors can be marked on either the last attempt within an execution, or on the execution itself.
In FTL billing logic, that looks like
if (!execution.attempts().isEmpty()) {
    Attempt lastAttempt = execution.attempts().get(execution.attempts().size() - 1);
    if (lastAttempt.getAttemptStatus() != AttemptStatus.ERROR && execution.state() != TestState.ERROR) {
        // do rounding/timeout calculations as described in the pull request
    }
}

@bootstraponline
That's valuable information, thanks! (I was not able to find such info in FTL docs)

…f lower than timeout

bootstraponline · 2020-06-25T05:14:06Z

I was not able to find such info in FTL docs

we should probably document clearly in the Kotlin file what the business rules for billing are. 🙂

piotradamczyk5 · 2020-06-25T17:21:38Z

test_runner/src/main/kotlin/ftl/json/SavedMatrix.kt

@@ -27,7 +29,7 @@ class SavedMatrix(matrix: TestMatrix) {

    var billableVirtualMinutes: Long = 0
        private set
-    var billablePhysicalMinutes: Long = 0
+    var billablePhysicalSecondsMinutes: Long = 0


could we think about other name, because I am confused if we use seconds or minutes

Yeah, definitely. We have yet another round of refactor ahead of us

Thanks, its my fail. Its should be billablePhysicalMinutes

piotradamczyk5 · 2020-06-25T17:24:48Z

test_runner/src/test/kotlin/ftl/json/MatrixMapTest.kt

                .setResultStorage(createResultsStorage())
                .setState(MatrixState.FINISHED)
                .setTestMatrixId("123")
                .setTestExecutions(
                    listOf(
                        createStepExecution(executionId, "")
                    )
-                )
+                ),
+            testTimeout = defaultTestTimeout


maybe make it as a default parameter value

If you are thinking about default value for SavedMatrix constructor I don't think it's good idea. That would lead us to 2 places where default value (common args and SavedMatrix constructor) is hold, easy to forget about one when changing/updating second

Now we get testTimeout from api response so we don't need pass them by constructor.

pawelpasterz · 2020-06-26T04:32:40Z

According to Paul Davis, we can check just TestExecution for state and if it's ERROR skip billing

Just check the TestExecution state, and report $0.00 if the state is ERROR.

Description updated

Another comment:

Flank uses a Spark free-billing-tier project, so the billing step is skipped, which is why I think you're seeing that (deviceUsageDuration) null value.

In theory we could use deviceUsageDuration to exact billing calculation and assume $0.00 in case when it's null -- this needs to be verified

cc @bootstraponline @adamfilipow92

bootstraponline · 2020-06-26T04:53:22Z

In theory we could use deviceUsageDuration to exact billing calculation and assume $0.00 in case when it's null -- this needs to be verified

I'd like to support both Spark plan and the paid plan. It's useful to know what Flank runs will cost.

We probably need to setup a paid FTL account to verify deviceUsageDuration works as expected.

pawelpasterz · 2020-06-26T04:57:40Z

In theory we could use deviceUsageDuration to exact billing calculation and assume $0.00 in case when it's null -- this needs to be verified

I'd like to support both Spark plan and the paid plan. It's useful to know what Flank runs will cost.

Therefor deviceUsageDuration would be best for this.

We probably need to setup a paid FTL account to verify deviceUsageDuration works as expected.

I might have access to the project with paid plan -- will check it

pawelpasterz · 2020-06-26T13:17:48Z

@bootstraponline
I've tested fetching value from deviceUsageDuration on project with Blaze without any result -- null is all I got. I think for now we should go with 'error check' approach and come back to this once API would be more...reliable?

With 'error check' we would have correct numbers as well

cc @adamfilipow92

adamfilipow92 · 2020-06-26T13:54:05Z

So this is plan:

Skip billing if testExecution.state == ERROR.
Calculate cost even if we on spark plan.

bootstraponline · 2020-06-27T17:38:44Z

I've tested fetching value from deviceUsageDuration on project with Blaze without any result -- null is all I got. I think for now we should go with 'error check' approach and come back to this once API would be more...reliable?

With 'error check' we would have correct numbers as well

That sounds good. The FTL API is often completely broken so it's good that we always double check. 🙂

adamfilipow92 self-assigned this Jun 23, 2020

adamfilipow92 changed the title ~~Print cost report with seconds~~ Flank needs to respect the timeout value as that's a cap for billing purposes. Jun 24, 2020

pawelpasterz self-assigned this Jun 24, 2020

adamfilipow92 force-pushed the #836-Fix-billable-minutes branch from 1d7944d to 522ffbf Compare June 24, 2020 16:22

adamfilipow92 and others added 6 commits June 25, 2020 05:39

Added seconds to cost results and fix billing to calculate per seconds

2aad487

Remove dead code and refractor minutes to seconds

4bcc86b

Update tests

a77987e

Added tests, improve billing calculations to respect gcloud timeout i…

a598b27

…f lower than timeout

Update release_notes.md

14b649b

Refactor

e80131f

pawelpasterz force-pushed the #836-Fix-billable-minutes branch from bf58fa6 to e80131f Compare June 25, 2020 03:39

piotradamczyk5 reviewed Jun 25, 2020

View reviewed changes

adamfilipow92 added 2 commits June 26, 2020 15:10

Get TestTimeout from Specification

24717eb

Cr changes

b5dd431

OnErron no calculate billing added tests

b730c62

adamfilipow92 marked this pull request as ready for review June 26, 2020 15:41

pawelpasterz requested review from jan-goral and piotradamczyk5 June 29, 2020 04:36

Refactor

2a4e50b

jan-goral previously approved these changes Jun 29, 2020

View reviewed changes

piotradamczyk5 previously approved these changes Jun 29, 2020

View reviewed changes

Create deviceUsageDuration_always_null.md

2f4997e

adamfilipow92 dismissed stale reviews from piotradamczyk5 and jan-goral via 2f4997e June 29, 2020 13:01

adamfilipow92 requested review from pawelpasterz, piotradamczyk5 and jan-goral June 29, 2020 13:07

Update deviceUsageDuration_always_null.md

8393efa

piotradamczyk5 approved these changes Jun 29, 2020

View reviewed changes

jan-goral approved these changes Jun 29, 2020

View reviewed changes

adamfilipow92 merged commit 3978611 into master Jun 29, 2020

adamfilipow92 deleted the #836-Fix-billable-minutes branch June 29, 2020 14:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Flank needs to respect the timeout value as that's a cap for billing purposes. #865

Flank needs to respect the timeout value as that's a cap for billing purposes. #865

adamfilipow92 commented Jun 23, 2020 •

edited

Loading

bootstraponline commented Jun 23, 2020

adamfilipow92 commented Jun 24, 2020 •

edited

Loading

bootstraponline commented Jun 24, 2020

pawelpasterz commented Jun 24, 2020

pawelpasterz commented Jun 24, 2020

bootstraponline commented Jun 24, 2020

pawelpasterz commented Jun 25, 2020

bootstraponline commented Jun 25, 2020

piotradamczyk5 Jun 25, 2020

pawelpasterz Jun 25, 2020

adamfilipow92 Jun 26, 2020

piotradamczyk5 Jun 25, 2020

pawelpasterz Jun 25, 2020 •

edited

Loading

adamfilipow92 Jun 26, 2020

pawelpasterz commented Jun 26, 2020 •

edited

Loading

bootstraponline commented Jun 26, 2020

pawelpasterz commented Jun 26, 2020 •

edited

Loading

pawelpasterz commented Jun 26, 2020

adamfilipow92 commented Jun 26, 2020

bootstraponline commented Jun 27, 2020

Flank needs to respect the timeout value as that's a cap for billing purposes. #865

Flank needs to respect the timeout value as that's a cap for billing purposes. #865

Conversation

adamfilipow92 commented Jun 23, 2020 • edited Loading

Flank billing calculation rules:

Test Plan

Checklist

bootstraponline commented Jun 23, 2020

adamfilipow92 commented Jun 24, 2020 • edited Loading

bootstraponline commented Jun 24, 2020

pawelpasterz commented Jun 24, 2020

pawelpasterz commented Jun 24, 2020

bootstraponline commented Jun 24, 2020

pawelpasterz commented Jun 25, 2020

bootstraponline commented Jun 25, 2020

piotradamczyk5 Jun 25, 2020

Choose a reason for hiding this comment

pawelpasterz Jun 25, 2020

Choose a reason for hiding this comment

adamfilipow92 Jun 26, 2020

Choose a reason for hiding this comment

piotradamczyk5 Jun 25, 2020

Choose a reason for hiding this comment

pawelpasterz Jun 25, 2020 • edited Loading

Choose a reason for hiding this comment

adamfilipow92 Jun 26, 2020

Choose a reason for hiding this comment

pawelpasterz commented Jun 26, 2020 • edited Loading

bootstraponline commented Jun 26, 2020

pawelpasterz commented Jun 26, 2020 • edited Loading

pawelpasterz commented Jun 26, 2020

adamfilipow92 commented Jun 26, 2020

bootstraponline commented Jun 27, 2020

adamfilipow92 commented Jun 23, 2020 •

edited

Loading

adamfilipow92 commented Jun 24, 2020 •

edited

Loading

pawelpasterz Jun 25, 2020 •

edited

Loading

pawelpasterz commented Jun 26, 2020 •

edited

Loading

pawelpasterz commented Jun 26, 2020 •

edited

Loading