[Monitor-Query] Update monitor-query to use new recorder #20178

JonathanCrd · 2022-02-01T23:34:49Z

Packages impacted by this PR

@azure/monitor-query

Issues associated with this PR

[core-rest-pipeline] Make FetchClient the default browser httpclient #19859

Describe the problem that is addressed by this PR

This PR migrate monitor query to use new recorder and discard any use of the old one. It also updates the recordings. For more information check out this migration guide.

Checklists

Added impacted package name to the issue description
Does this PR needs any fixes in the SDK Generator?** (If so, create an Issue in the Autorest/typescript repository and link it here)
Added a changelog (if necessary)

sdk/monitor/monitor-query/test/public/shared/testShared.ts

…ry-to-new-recorder

timovv

Thanks for doing this! Overall it's looking good, just had a couple small comments/questions :)

timovv · 2022-02-11T19:40:22Z

sdk/monitor/monitor-query/test/public/shared/testShared.ts

    console.log(
      `TODO: live tests skipped until test-resources + data population is set up (missing ${variableName} env var).`
    );
    mochaContext.skip();
+    // NOTE: This function should be throwing


Just wondering, what do you mean by this? If the function should throw we can use the assertEnvironmentVariable helper exposed by the recorder instead of defining this function.

This was a note that came out during a sync, I forgot to update the function to add a Throw new Error(... in case the environment variable is not defined (right now it is just skipping it).

Can you provide more info about the assertEnvironmentVariable helper and how it could be used?

We have an assertEnvironmentVariable function exported from the recorder package which either returns the value of the environment variable or throws an error if it's not set. Maybe you could use that instead of the helper that's here at the moment (and remove old the helper entirely).

Thank you for the explanation, I also think that method can replace this function. So, since CI is green for this PR already and test are working, I'm going to file an issue with this suggestion, and see if this is the case for other packages.

sdk/monitor/monitor-query/test/public/shared/testShared.ts

...onitor/monitor-query/recordings/node/metricsclient_live_tests/recording_listdefinitions.json

.vscode/settings.json

sdk/monitor/ci.yml

timovv · 2022-02-11T19:50:44Z

sdk/monitor/monitor-query/karma.conf.js

-} = require("@azure-tools/test-recorder");
+const { relativeRecordingsPath } = require("@azure-tools/test-recorder");
+
+process.env.RECORDINGS_RELATIVE_PATH = relativeRecordingsPath();


A passing observation: this has absolutely nothing to do with your changes and you don't need to do anything about it, but it irks me slightly that the environment variable is RECORDINGS_RELATIVE_PATH ('recordings', then 'relative'), but the function is the other way around ('relative', then 'recordings').

sdk/monitor/monitor-query/test/public/shared/testShared.ts

Co-authored-by: Timo van Veenendaal <me@timo.nz>

HarshaNalluru · 2022-02-12T00:01:51Z

/azp run js - monitor-query - tests

azure-pipelines · 2022-02-12T00:01:57Z

No pipelines are associated with this pull request.

azure-pipelines · 2022-02-12T00:02:30Z

Azure Pipelines successfully started running 1 pipeline(s).

HarshaNalluru

Looks like all the tests are not being run for @azure/monitor-query. 😢

New

[node-tests] 9 passing (389ms)
[node-tests] 2 pending

https://dev.azure.com/azure-sdk/public/_build/results?buildId=1363590&view=results

Old

25 passing (982ms)
2 pending
https://dev.azure.com/azure-sdk/public/_build/results?buildId=1360303&view=results

JonathanCrd · 2022-02-12T00:17:44Z

Looks like all the tests are not being run for @azure/monitor-query. 😢

Update on this: Only windows is running the 25 tests. Investigating why the other OS are not running all the tests.

KarishmaGhiya · 2022-02-12T01:04:03Z

sdk/monitor/monitor-query/recordings/node/metricsclient_live_tests/recording_listnamespaces.js

@@ -1,5 +0,0 @@
-let nock = require('nock');
-


Any idea why is this file deleted? This comes from one of the tests right?

This was a generated file; it is no longer generated with the new recorder. File sdk/monitor/monitor-query/recordings/browsers/metricsclient_live_tests/recording_listnamespaces.json is being generated instead.

JonathanCrd · 2022-02-12T05:25:57Z

Looks like all the tests are not being run for @azure/monitor-query. 😢

✅Fixed:
The reason why not all 25 tests were running on ubuntu/macOS was due to the structure of the test/ folder and the path specified on the packaje.json script:

test/
┣ internal/
┃ ┗ unit/
┃   ┣ logsQueryClient.unittest.spec.ts
┃   ┣ modelConverters.unittest.spec.ts
┃   ┗ utils.unittest.spec.ts
┣ public/
┃ ┣ shared/
┃ ┃ ┗ testShared.ts
┃ ┣ logsQueryClient.spec.ts
┃ ┗ metricsQueryClient.spec.ts
┗ setupOpenTelemetry.ts

"unit-test:node": "dev-tool run test:node-ts-input -- --timeout 1200000 --exclude 'test/**/browser/*.spec.ts' 'test/**/*.spec.ts'"

As you can see, the internal/ directory has a unit/ folder inside, so the script was only specifying to run test in path 'test/**/*.spec.ts', and for Unix based OS this was not being done recursively.

💡To fix this, I added an extra path test/**/**/*.spec.ts', so it will reach the tests inside internal/unit/ all times.

Kudos to @joheredi for helping with this bug.👏 CC: @HarshaNalluru

JonathanCrd · 2022-02-12T05:26:18Z

/azp run js - monitor-query - tests

azure-pipelines · 2022-02-12T05:26:31Z

Azure Pipelines successfully started running 1 pipeline(s).

joheredi

LGTM!

timovv

Looks great!

JonathanCrd added 3 commits January 29, 2022 00:35

Update test-recorder version

141a940

Update test scripts

6cc629b

Removing old recorder instances

aac3280

JonathanCrd self-assigned this Feb 1, 2022

ghost added the Monitor Monitor, Monitor Ingestion, Monitor Query label Feb 1, 2022

JonathanCrd added 5 commits February 11, 2022 09:59

Making Karma changes

2efecc7

Update node recordings

0741942

Fixing missing recordings_relative_path in environment vars

86deb27

Update browser recordings

df5f80e

Update browser recordings

7273438

JonathanCrd changed the title ~~[Monitor-Query] Migrate monitor query to new recorder~~ [Monitor-Query] Update monitor-query to use new recorder Feb 11, 2022

JonathanCrd marked this pull request as ready for review February 11, 2022 19:20

JonathanCrd requested review from mikeharder, praveenkuttappan, deyaaeldeen, benbp, KarishmaGhiya and maorleger as code owners February 11, 2022 19:20

JonathanCrd requested review from HarshaNalluru and timovv February 11, 2022 19:20

timovv reviewed Feb 11, 2022

View reviewed changes

sdk/monitor/monitor-query/test/public/shared/testShared.ts Outdated Show resolved Hide resolved

Merge remote-tracking branch 'upstream/main' into migrate-monitor-que…

6c1d7df

…ry-to-new-recorder

timovv reviewed Feb 11, 2022

View reviewed changes

JonathanCrd and others added 5 commits February 11, 2022 13:48

Discard unintentional change in vscode settings

8efdf36

Throw error if required env var is not defined

5d18bba

Apply suggestions from code review

921e6a9

Co-authored-by: Timo van Veenendaal <me@timo.nz>

Adressing comments and fixing build with last changes from main

f556b2d

Regenerate recordings/ folder

f4b03c1

JonathanCrd mentioned this pull request Feb 11, 2022

[core-rest-pipeline] Make FetchClient the default browser httpclient #19859

Closed

23 tasks

format files

c7f1916

HarshaNalluru requested changes Feb 12, 2022

View reviewed changes

KarishmaGhiya reviewed Feb 12, 2022

View reviewed changes

Discover all test inside test/internal/unit/

527b127

JonathanCrd mentioned this pull request Feb 12, 2022

[DevTool] Run test:node command doesn't ignore browser folder in Windows #20346

Closed

joheredi approved these changes Feb 12, 2022

View reviewed changes

HarshaNalluru approved these changes Feb 14, 2022

View reviewed changes

timovv approved these changes Feb 14, 2022

View reviewed changes

JonathanCrd merged commit 39e4830 into Azure:main Feb 14, 2022

This was referenced Mar 23, 2022

[Monitor Query] Nit: getRequiredEnvVar helper method is no longer necessary with latest recorder #21005

Closed

Replace getRequiredEnvVar function with assertEnvironmentVariable helper from new recorder #21009

Merged

JonathanCrd deleted the migrate-monitor-query-to-new-recorder branch June 17, 2022 19:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Monitor-Query] Update monitor-query to use new recorder #20178

[Monitor-Query] Update monitor-query to use new recorder #20178

JonathanCrd commented Feb 1, 2022 •

edited

Loading

timovv left a comment

timovv Feb 11, 2022

JonathanCrd Feb 11, 2022

timovv Feb 11, 2022

JonathanCrd Feb 11, 2022

timovv Feb 11, 2022

HarshaNalluru commented Feb 12, 2022 •

edited

Loading

azure-pipelines bot commented Feb 12, 2022

azure-pipelines bot commented Feb 12, 2022

HarshaNalluru left a comment

JonathanCrd commented Feb 12, 2022

KarishmaGhiya Feb 12, 2022

JonathanCrd Feb 12, 2022

JonathanCrd commented Feb 12, 2022

JonathanCrd commented Feb 12, 2022

azure-pipelines bot commented Feb 12, 2022

joheredi left a comment

timovv left a comment

[Monitor-Query] Update monitor-query to use new recorder #20178

[Monitor-Query] Update monitor-query to use new recorder #20178

Conversation

JonathanCrd commented Feb 1, 2022 • edited Loading

Packages impacted by this PR

Issues associated with this PR

Describe the problem that is addressed by this PR

Checklists

timovv left a comment

Choose a reason for hiding this comment

timovv Feb 11, 2022

Choose a reason for hiding this comment

JonathanCrd Feb 11, 2022

Choose a reason for hiding this comment

timovv Feb 11, 2022

Choose a reason for hiding this comment

JonathanCrd Feb 11, 2022

Choose a reason for hiding this comment

timovv Feb 11, 2022

Choose a reason for hiding this comment

HarshaNalluru commented Feb 12, 2022 • edited Loading

azure-pipelines bot commented Feb 12, 2022

azure-pipelines bot commented Feb 12, 2022

HarshaNalluru left a comment

Choose a reason for hiding this comment

New

Old

JonathanCrd commented Feb 12, 2022

KarishmaGhiya Feb 12, 2022

Choose a reason for hiding this comment

JonathanCrd Feb 12, 2022

Choose a reason for hiding this comment

JonathanCrd commented Feb 12, 2022

JonathanCrd commented Feb 12, 2022

azure-pipelines bot commented Feb 12, 2022

joheredi left a comment

Choose a reason for hiding this comment

timovv left a comment

Choose a reason for hiding this comment

JonathanCrd commented Feb 1, 2022 •

edited

Loading

HarshaNalluru commented Feb 12, 2022 •

edited

Loading