[Perf Framework] BatchPerfTest & Service Bus Receive Batch perf test #18811

HarshaNalluru · 2021-11-24T10:20:05Z

Fixes #18329

Changes

Adds PerfTestBase and BatchPerfTest.
- PerfTestBase abstract class with the skeleton of the perf tests.
- BatchPerfTest extends PerfTestBase, BatchPerfTest enables writing perf tests with more flexibility where the number of operations are dynamic for the method/call being tested.
- PerfTest now extends BatchPerfTest, a single call in the "run" method for the method being tested counts as one operation - has no breaking changes now and is supposed to work as it did before.
Adding ReceiveBatch perf test for service-bus track 2.
New template type ParsedPerfOptions<TOptions = Record<string, unknown>> for the parsed options, which doesn't require to add/bypass the "value" existence check.
- Example - this.parsedOptions.optionName.value! -> this.parsedOptions.optionName.value in the perf tests.
- It instead throws during validation.
Make sure the NoOp test doesn't have any regressions.
Make sure other tests don't have regressions
- tested storage blob download and there seems to be no significant difference in the performance with the new changes

TODO

Perf framework for streaming methods
Add perf tests for batch APIs in other SDKs [Perf Tests] Add/Convert perf tests for batch APIs in AMQP based SDKs #18982
- event hubs - track 1
- event hubs - track 2
- service bus - track 1
Migrate perf-js-libs rhea and others to follow the new perf tests for better comparison and delete the older perf-js-libs folder [Service Bus] [Perf Tests] Migrate perf-js-libs rhea and others to follow the new perf tests #18983 logged
parsePerfOption in the PerfTestBase calls parsePerfOption method internally with the this.options and defaultPerfOptions arguments. This is fine because they both don't have an overlap. However, we don't make sure there is no overlap during the validation. Overlapping props can lead to unexpected behaviors. Make sure the validation method or the type validation throw/complain about re-declaring in options bag with the names from default options.

…ates

Co-authored-by: Daniel Rodríguez <sadasant@users.noreply.github.com>

sdk/test-utils/perf/src/perfTestBase.ts

Co-authored-by: Daniel Rodríguez <sadasant@users.noreply.github.com>

sdk/test-utils/perf/src/perfTestBase.ts

sdk/test-utils/perf/src/options.ts

mikeharder

Approve changes to perf framework. Requested one change to the receiveBatch test.

mikeharder · 2021-11-30T00:27:26Z

sdk/servicebus/perf-tests/service-bus/test/receiveBatch.spec.ts

+  "message-body-size-in-bytes": number;
+}
+
+export class BatchReceiveTest extends ServiceBusTest<ReceiverOptions> {


I think BatchReceiveTest should extend BatchPerfTest rather than PerfTest (which ServiceBusTest extends).

While it is possible in JS to extend PerfTest and then override the runBatch() method, it should not be recommended, and this is not possible in other languages like .NET, where PerfTest seals the runBatch() method to prevent further overriding:

https://github.com/Azure/azure-sdk-for-net/pull/24653/files#diff-427d3566282e8c258aa70a4b5d4eddbbfc5cf250b38a8e0dcfcb7b43126b9f18R15

TypeScript does not support "sealed/final" methods, so I'm not sure if there is any reasonable way to prevent overriding PerfTest.runBatch(), but we should at least discourage it and make sure we don't do it in our examples.

So, I'd either create two base classes like ServiceBusTest extends PerfTest and ServiceBusBatchTest extends BatchPerfTest, or just use ServiceBusTest extends BatchPerfTest, and have all tests override runBatch() (and return 1 if only performing a single operation).

ServiceBusTest extends BatchPerfTest, and have all tests override runBatch() (and return 1 if only performing a single operation)

My intention was to reuse the setup hence updated PerfTest to have both.
I like your suggested approach, will update.

We don't have "sealed"

We could try decorators, but they are experimental yet and subject to change

We have "abstract", "public", "private", "protected" and "override" keywords

We use "abstract" as intended

We can't use "private"/"protected" for run/runBatch

Override is not relevant

Looks like we need to make run and runBatch public

However, I updated the base SBTestBase to extend the BatchPerfTest instead.

…thub.com/HarshaNalluru/azure-sdk-for-js into harshan/issue/18471-event-perf-framework

sadasant

Looks good! I’m excited about this project.

… harshan/issue/18471-event-perf-framework

timovv

Looks good :)

deyaaeldeen · 2021-12-07T23:23:42Z

sdk/servicebus/perf-tests/service-bus/test/receiveBatch.spec.ts

+  public async runBatch(): Promise<number> {
+    const messages = await this.receiver.receiveMessages(
+      this.parsedOptions["max-message-count"].value,
+      { maxWaitTimeInMs: 500 }


could this be another parameter to the test?

deyaaeldeen · 2021-12-07T23:56:34Z

sdk/test-utils/perf/src/batchPerfTest.ts

+export abstract class BatchPerfTest<TOptions = Record<string, unknown>> extends PerfTestBase<
+  TOptions
+> {
+  private readonly testProxy!: string;


could testProxy be optional instead?

same goes for the other two members.

deyaaeldeen · 2021-12-08T00:06:35Z

sdk/test-utils/perf/src/batchPerfTest.ts

+   *
+   * Note: httpClient must be part of the options bag, it is required for the perf framework to update the underlying client properly
+   */
+  public configureClientOptionsCoreV1<T>(options: T & { httpClient?: HttpClient }): T {


the function type signature suggests it removes httpClient from the input but it does not. I think the following can be slightly more descriptive:

public configureClientOptionsCoreV1<T extends { httpClient?: HttpClient }>(options: T): T

deyaaeldeen · 2021-12-08T00:10:40Z

sdk/test-utils/perf/src/batchPerfTest.ts

+  }
+
+  /**
+   * Runs the test in scope repeatedly, without waiting for any promises to finish,


waiting for any promises to finish

I am confused by this comment because the only promise in scope, this.runBatch(abortController.signal), is being awaited.

…vice Bus Track 1 (#19790) **Packages impacted by this PR:** - `@azure-tests/perf-service-bus-track-1` **Issues associated with this PR:** - #18982 **Describe the problem that is addressed by this PR:** Add a test for receiveBatch using the track 1 service bus SDK in line with what we already have for the track 2 SDK **Provide a list of related PRs** _(if any)_ - #18811

[App Configuration] Promote 2022-05-01 stable version (Azure#18811) * Add 2022-05-01 folder based on 2021-10-01-preview * Update 2022-05-01 folder * Fix readme issue

HarshaNalluru added 12 commits November 24, 2021 07:13

clean up old scripts

3037243

tests -> perfTest

e437726

base, event, batch perf test classes

2a13a51

program.ts

1d00bd8

incorrect comment

b6303ed

postSetup and preCleanup

47692c1

ParsedPerfOptions<TOptions = Record<string, unknown>> and related upd…

d20e8d8

…ates

remove ! from options

4d1db88

MockReceiverTest

c5b6ff5

import "@azure/abort-controller" at perf classes

daa37e8

BatchReceiveTest

3cc719c

remove !

8c8ceb1

ghost added Service Bus Storage Storage Service (Queues, Blobs, Files) labels Nov 24, 2021

HarshaNalluru added 8 commits November 24, 2021 19:48

changelog

b30f0f0

receiveBatch test

9191472

remove aborter

6987bd9

docs

8a1c33c

BatchSendTest

7fb6d39

receiveMessages

d42de56

no need of timer

c7e2f21

- Adds PerfTestBase and BatchPerfTest.

d5b7c5d

HarshaNalluru marked this pull request as ready for review November 24, 2021 20:10

HarshaNalluru requested review from deyaaeldeen, EmmaZhu, jeremymeng, jiacfan, ljian3377, ramya-rao-a and richardpark-msft as code owners November 24, 2021 20:10

Update sdk/test-utils/perf/src/perfTest.ts

17cec72

Co-authored-by: Daniel Rodríguez <sadasant@users.noreply.github.com>

sadasant reviewed Nov 24, 2021

View reviewed changes

sdk/test-utils/perf/src/perfTestBase.ts Outdated Show resolved Hide resolved

Update sdk/test-utils/perf/src/perfTest.ts

e4b2735

Co-authored-by: Daniel Rodríguez <sadasant@users.noreply.github.com>

sadasant reviewed Nov 24, 2021

View reviewed changes

sdk/test-utils/perf/src/perfTestBase.ts Show resolved Hide resolved

sadasant reviewed Nov 24, 2021

View reviewed changes

sdk/test-utils/perf/src/perfTestBase.ts Show resolved Hide resolved

timovv reviewed Nov 24, 2021

View reviewed changes

sdk/test-utils/perf/src/options.ts Outdated Show resolved Hide resolved

HarshaNalluru mentioned this pull request Nov 29, 2021

[Perf] [Service Bus] [Event Hubs] Event Based SDKs Perf Framework - part 2 #18859

Merged

mikeharder approved these changes Nov 30, 2021

View reviewed changes

HarshaNalluru added 5 commits November 30, 2021 02:00

just a comment

33dcd45

just a comment

4dd4872

more comments

34578e5

stylize

2a35d43

Merge branch 'harshan/issue/18471-event-perf-framework' of https://gi…

5338852

…thub.com/HarshaNalluru/azure-sdk-for-js into harshan/issue/18471-event-perf-framework

sadasant mentioned this pull request Nov 30, 2021

[Perf Framework] Figure out a way to merge "parsedOptions" into "options" #18032

Closed

sadasant approved these changes Nov 30, 2021

View reviewed changes

HarshaNalluru added 4 commits December 2, 2021 23:00

better Type

c3e3f6c

Merge branch 'main' of https://github.com/Azure/azure-sdk-for-js into…

8e019a0

… harshan/issue/18471-event-perf-framework

SBTest extends BatchPerfTest

13e261a

run the tests with this command

b60a6fa

timovv approved these changes Dec 6, 2021

View reviewed changes

HarshaNalluru merged commit 46164b9 into Azure:main Dec 6, 2021

HarshaNalluru deleted the harshan/issue/18471-event-perf-framework branch December 6, 2021 23:10

HarshaNalluru mentioned this pull request Dec 6, 2021

[Perf Tests] Add/Convert perf tests for batch APIs in AMQP based SDKs #18982

Closed

3 tasks

deyaaeldeen reviewed Dec 7, 2021

View reviewed changes

deyaaeldeen reviewed Dec 8, 2021

View reviewed changes

timovv mentioned this pull request Jan 11, 2022

[Perf tests/service-bus-track-1] Add batch receive perf tests for Service Bus Track 1 #19790

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Perf Framework] BatchPerfTest & Service Bus Receive Batch perf test #18811

[Perf Framework] BatchPerfTest & Service Bus Receive Batch perf test #18811

HarshaNalluru commented Nov 24, 2021 •

edited

Loading

mikeharder left a comment

mikeharder Nov 30, 2021

mikeharder Nov 30, 2021

HarshaNalluru Nov 30, 2021

HarshaNalluru Dec 6, 2021

sadasant left a comment

timovv left a comment

deyaaeldeen Dec 7, 2021

deyaaeldeen Dec 7, 2021

deyaaeldeen Dec 7, 2021

deyaaeldeen Dec 8, 2021

deyaaeldeen Dec 8, 2021

[Perf Framework] BatchPerfTest & Service Bus Receive Batch perf test #18811

[Perf Framework] BatchPerfTest & Service Bus Receive Batch perf test #18811

Conversation

HarshaNalluru commented Nov 24, 2021 • edited Loading

Fixes #18329

Changes

TODO

mikeharder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sadasant left a comment

Choose a reason for hiding this comment

timovv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HarshaNalluru commented Nov 24, 2021 •

edited

Loading