Skip to content

[Identity] Performance tests#14634

Merged
14 commits merged intoAzure:masterfrom
sadasant:identity/performance-tests-14632
Apr 1, 2021
Merged

[Identity] Performance tests#14634
14 commits merged intoAzure:masterfrom
sadasant:identity/performance-tests-14632

Conversation

@sadasant
Copy link
Contributor

@sadasant sadasant commented Mar 31, 2021

This PR adds the package @azure-tests/perf-identity and a first performance test for Identity.

Here's the output from one run I did locally:

=== Calling globalSetup() once for (all) the instance(s) of ClientSecretCredentialPersistenceTest ===

=== warmup mode, iteration 0. Logs every 1s ===
Current         Total           Average
1589            1589            1590.49
=== warmup mode, results of iteration 1 ===
Completed 1,591 operations in a weighted-average of 1.00s (1,589.46 ops/s, 0.001 s/op)

=== test mode, iteration 0. Logs every 1s ===
Current         Total           Average
  credential?: ClientSecretCredential;
2439            2439            2442.35
3036            5475            2739.85
3080            8555            2856.42
2789            11344           2820.61
3186            14530           2893.90
3110            17640           2927.82
3258            20898           2973.09
2981            23879           2974.87
3188            27067           2997.42
=== test mode, results of iteration 1 ===
Completed 30,051 operations in a weighted-average of 10.00s (3,005.92 ops/s, 0.000 s/op)

I used Key Vault Keys as a reference to write these files: https://github.com/Azure/azure-sdk-for-js/tree/master/sdk/keyvault/perf-tests/keyvault-keys

As far as I could tell form Java's PR (link) the two goals for the Identity performance tests at the moment are:

  1. Testing the challenge processing features on core, for which I have made a separate issue, since this feature is not yet in our main branch: [core-rest-pipeline] Add performance tests for challenges #14633 .
    • The separation will also help reduce the number of changes to see when that perf test comes, since this one will have added all of the necessary files and folders around.
  2. Testing the persistence feature, which this PR includes.

Let me know how it looks! Any feedback appreciated.

Fixes #14632

@sadasant sadasant requested review from g2vinay and maorleger March 31, 2021 13:25
@sadasant sadasant self-assigned this Mar 31, 2021
@ghost ghost added the Azure.Identity label Mar 31, 2021
@check-enforcer

This comment has been minimized.

@sadasant
Copy link
Contributor Author

sadasant commented Apr 1, 2021

Hmmm pipeline errors got me for a bit, but I think I have them under control.


constructor() {
super();
this.credential = new DeviceCodeCredential({
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

replace this with Client Secret Credential so that it is easy to automate.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can do! Anything else?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nope, all fine otherwise, as long as perf test compiles and runs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@g2vinay the test does compile and run, but we can't make this run on CI because we currently can't have @azure/msal-node-extensions@1.0.0-alpha.6 as a dependency, at the moment 😵

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now I'm using ClientSecretCredential :)

@g2vinay g2vinay requested a review from HarshaNalluru April 1, 2021 17:58
@sadasant sadasant marked this pull request as ready for review April 1, 2021 18:05
**Important:**
These tests won't work on Node 8 nor Node 15.

1. Install `@azure/msal-node-extensions@1.0.0-alpha.6` globally with `npm i -g @azure/msal-node-extensions@1.0.0-alpha.6`.
Copy link
Contributor

@HarshaNalluru HarshaNalluru Apr 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why global?.. Is it not enough to add it as a dependency in the package.json?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't have this dependency in our package.json since this means developers need to have Visual Studio or build-essential installed, and in some Node versions this dependency doesn't compile. We're working on it with the MSAL team.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant.. dependency of the perf-test identity project, where you're mentioning @azure/identity.

Copy link
Contributor

@HarshaNalluru HarshaNalluru left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes as per the above comments.

}

async globalSetup(): Promise<void> {
await this.credential.getToken(scope);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this needed as a setup step?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This initial call "caches" the token.

@sadasant sadasant requested a review from HarshaNalluru April 1, 2021 20:01
@ghost
Copy link

ghost commented Apr 1, 2021

Hello @sadasant!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit 431e33a into Azure:master Apr 1, 2021
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Identity] Write performance tests for Identity

5 participants