Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MetricsAdvisor] Added tests.yml file for automated test runs #17894

Merged
merged 1 commit into from
Jan 12, 2021

Conversation

kinelski
Copy link
Member

@kinelski kinelski commented Jan 11, 2021

Part of #15924.

@kinelski kinelski added Client This issue points to a problem in the data-plane of the library. Cognitive - Metrics Advisor labels Jan 11, 2021
@kinelski kinelski self-assigned this Jan 11, 2021
@kinelski kinelski requested a review from annelo-msft January 11, 2021 23:32
@kinelski kinelski marked this pull request as ready for review January 11, 2021 23:32
@kinelski kinelski requested a review from danieljurek as a code owner January 11, 2021 23:32
@kinelski kinelski merged commit e11d64b into Azure:master Jan 12, 2021
@kinelski kinelski deleted the ma-test branch January 12, 2021 00:40
@kinelski
Copy link
Member Author

kinelski commented Jan 12, 2021

It seems we're getting PermissionDenied from the service, but tests work locally with the same settings:
https://dev.azure.com/azure-sdk/internal/_build/results?buildId=682791&view=logs&j=9e4810bd-6c8e-5aff-29e0-d5fb4fb1935f&t=aebe1dc3-650d-52a7-62d2-615892e27a94&l=14919

Is there anything wrong with the way we're declaring the env vars, danieljurek ?

EnvVars:
METRICSADVISOR_ACCOUNT_NAME: js-metrics-advisor
METRICSADVISOR_SUBSCRIPTION_KEY: $(metricsadvisor-test-subscription-key)
METRICSADVISOR_PRIMARY_API_KEY: $(net-metricsadvisor-test-api-key)
METRICSADVISOR_DATA_FEED_ID: 9860df01-e740-40ec-94a2-6351813552ba
METRICSADVISOR_METRIC_ID: 27e3015f-04fd-44ba-a20b-bc529a0aebae
METRICSADVISOR_DETECTION_CONFIGURATION_ID: fb5a6ed6-2b9e-4b72-8b0c-0046ead1c15c
METRICSADVISOR_ALERT_CONFIGURATION_ID: 204a211a-c5f4-45f3-a30e-512fb25d1d2c
METRICSADVISOR_HOOK_ID: e3499688-99c9-4a6c-bbcb-6866fcd593a5
METRICSADVISOR_ALERT_ID: 17548749000
METRICSADVISOR_INCIDENT_ID: 2c8a72ba0e8eb41643cc0e5cff368412-17548749000
METRICSADVISOR_FEEDBACK_ID: 50a63795-d631-43ae-9a05-d744df4b986d
METRICSADVISOR_SQL_SERVER_CONNECTION_STRING: $(metricsadvisor-test-sql-server-connection-string)
METRICSADVISOR_SQL_SERVER_QUERY: select * from adsample2 where Timestamp = @StartTime and City = 'Mexico City' and Category = 'Shoes Handbags & Sunglasses'

Edit: problem solved. We just needed to add quotes to the string env vars.

@danieljurek
Copy link
Member

@kinelski - I'm behind on the context here. Can we not use dynamic resources for these live tests? Also, .NET tests should not use a metrics advisor account name that starts with js. Keeping these test resources organized and consistent with the rest of our live tests will make debugging problems much easier.

For example in the future (assuming we need static resources), if someone on the engineering system or JS team sees an account called js-metrics-advisor and knows that the JS SDK has migrated away from it and deletes that account, we wouldn't expect the .NET tests to break.

cc @benbp

@kinelski
Copy link
Member Author

kinelski commented Jan 12, 2021

@danieljurek We fixed the issue. We were only missing quotes in the env vars declarations (fixed here #17910).

Can we not use dynamic resources for these live tests?

Unfortunately this is not feasible in Metrics Advisor. The service takes too long to create the resource (30 minutes or so). Also, once the resource is created, it still needs an indefinite amount of time to process the data we need to request in our tests.

Also, .NET tests should not use a metrics advisor account name that starts with js.

The service limits the amount of resources we can create per subscription (only 2, if I'm not mistaken), so all languages are using the same resource for testing. It has the js prefix because it was only being used by JS originally (I can agree that this was not a wise decision). We can create and set up a new resource with a better name, but it will take some effort from other languages as well.

@danieljurek
Copy link
Member

Thanks for that info! This all makes sense. :)

Please track the work to fix this in a bug so it doesn't get lost.

@kinelski
Copy link
Member Author

Tracking it here: #17918

minnieliu pushed a commit to minnieliu/azure-sdk-for-net that referenced this pull request Jan 23, 2021
annelo-msft pushed a commit to annelo-msft/azure-sdk-for-net that referenced this pull request Feb 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Client This issue points to a problem in the data-plane of the library. Cognitive - Metrics Advisor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants