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

Removed AZURE_LOG_LEVEL:3 from test.yml in AzConfig which caused azure core to fail. #7914

Merged
merged 2 commits into from
Feb 4, 2020

Conversation

mssfang
Copy link
Member

@mssfang mssfang commented Feb 3, 2020

  • 'AZURE_LOG_LEVEL: 3' which caused nightly live tests to failed on HttpLoggingPolicyTests in azure core.
  • In addition, removed unnecessary slf4j log dependency.

@mssfang
Copy link
Member Author

mssfang commented Feb 3, 2020

/azp run java - appconfiguration - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mssfang
Copy link
Member Author

mssfang commented Feb 3, 2020

/azp run java - appconfiguration - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mssfang mssfang marked this pull request as ready for review February 3, 2020 23:22
@mssfang mssfang requested a review from anuchandy February 3, 2020 23:22
@mssfang mssfang self-assigned this Feb 3, 2020
@mssfang mssfang added Client This issue points to a problem in the data-plane of the library. App Configuration Azure.ApplicationModel.Configuration labels Feb 3, 2020
@mssfang mssfang requested a review from joshfree February 3, 2020 23:25
@mssfang mssfang changed the title delete slf4j dependency Removed delete slf4j dependency Feb 3, 2020
@mssfang mssfang changed the title Removed delete slf4j dependency Removed AZURE_LOG_LEVEL:3 from test.yml in AzConfig which caused azure core to fail. Feb 3, 2020
@mssfang
Copy link
Member Author

mssfang commented Feb 3, 2020

@mssfang mssfang merged commit 8a4c759 into Azure:master Feb 4, 2020
@mssfang mssfang deleted the Live_Test_failure branch February 4, 2020 00:01
@@ -10,4 +10,3 @@ jobs:
AZURE_CLIENT_ID: $(aad-azure-sdk-test-client-id)
AZURE_CLIENT_SECRET: $(aad-azure-sdk-test-client-secret)
AZURE_TENANT_ID: $(aad-azure-sdk-test-tenant-id)
AZURE_LOG_LEVEL: 3
Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't have been removed as it was pointing to a real issue with HttpLoggingPolicy.

Copy link
Member

Choose a reason for hiding this comment

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

+1 to Srikanta's comment. Logging shouldn't affect whether a test passes or fails.

Copy link
Member Author

Choose a reason for hiding this comment

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

created an issue to azure-core: #7919

Copy link
Member Author

Choose a reason for hiding this comment

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

KeyVault and Storage don't use AZURE_LOG_LEVEL in test.yml file. So I think azure core changes should not block Azconfig's nightly live tests.

Copy link
Member

Choose a reason for hiding this comment

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

Event Hubs uses AZURE_LOG_LEVEL. It helps a lot in understanding what in the SDK went wrong during a test rather than getting no spew at all.

The original reason we added AZURE_LOG_LEVEL was because we were trying to understand what was going wrong in the live tests because they were encountering a 429. The error could not be reproduced on our dev machines, but in the build machines. So, setting this log level would hopefully shed some light on the errors happening in the SDK.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is some limitation on test setup. In HttpLoggingPolicyTests, we have set the environment variable using System.setProperty, but this one will be overwritten if there is environment variable setup in system.

The HttpLoggingPolicyTests is working fine on AZURE_LOG_LEVEL<=2.

Copy link
Contributor

Choose a reason for hiding this comment

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

The current workaround is to set AZURE_LOG_LEVEL to 2 if log is needed in live tests

@JonathanGiles
Copy link
Member

JonathanGiles commented Feb 4, 2020

Edited
This PR should not have been merged - the issues raised by Srikanta and Connie should have been discussed prior to merging. We should always expect to have two approvals on a PR before merging.

I suggest you revert this merge and submit a new PR for discussion.

srnagar pushed a commit to srnagar/azure-sdk-for-java that referenced this pull request Feb 4, 2020
…e core to fail. (Azure#7914)

* delete slf4j dependency

* remove log level
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
App Configuration Azure.ApplicationModel.Configuration Client This issue points to a problem in the data-plane of the library.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants