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

Xiao/Enrich metadata failure message during metadata refresh interval #2010

Merged
merged 3 commits into from Feb 14, 2023

Conversation

ciaozhang
Copy link
Contributor

@ciaozhang ciaozhang commented Feb 1, 2023

Enrich the fetch metadata failure details from its inner exception. That enabled us to distinguish if the error was due to client misconfiguration (e.g.: tenant not found) or some transient error (e.g.: networking issue) during the 5 mins of metadata refresh interval.

@ciaozhang ciaozhang changed the title Xiao/fetch metadata log Xiao/Enrich metadata failure message during metadata refresh interval Feb 1, 2023
@ciaozhang ciaozhang requested review from brentschmaltz and dannybtsai and removed request for dannybtsai February 1, 2023 22:14
@@ -148,6 +149,7 @@ public async Task<T> GetConfigurationAsync(CancellationToken cancel)
}
catch (Exception ex)
{
_fetchMetadataFailure = ex;
Copy link
Contributor

Choose a reason for hiding this comment

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

What requires this to be a variable at the class level instead of just a local variable? Seems like there's an oppertunity here for race conditions and I'm not clear on the advantage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If ConfigurationManager.GetConfigurationAsync is called again before the syncAfter, then the IDX20803 exception at Line 167 gets thrown instead. We need the exception at the class level so that we can return it as the inner exception.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, this seems reasonable to me. Makes sense we wouldn't just want to have the exception in logs the first time from a usibility/servicability standpoint.

Copy link
Contributor

@TimHannMSFT TimHannMSFT left a comment

Choose a reason for hiding this comment

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

LGTM

@ciaozhang ciaozhang merged commit 6aa3efd into dev Feb 14, 2023
renovate bot added a commit to orso-co/Orso.Arpa.Api that referenced this pull request Apr 7, 2023
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
|
[System.IdentityModel.Tokens.Jwt](https://togithub.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet)
| nuget | minor | `6.27.0` -> `6.28.1` |

---

### Release Notes

<details>

<summary>AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet</summary>

###
[`v6.28.1`](https://togithub.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/releases/tag/6.28.1)

[Compare
Source](https://togithub.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/compare/6.28.0...6.28.1)

- Bug fix where internal cache was not instantiated
[AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet#2045

###
[`v6.28.0`](https://togithub.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/releases/tag/6.28.0)

[Compare
Source](https://togithub.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/compare/6.27.0...6.28.0)

- Enrich metadata failure message during metadata refresh interval
[AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet#2010
- Updated fix for controlling depth
[AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet#2024
- Update Wilson logs with aka.ms pointers to known wikis in
[AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet#2027
- Fix typo in documentation
[AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet#2034
- Introduce a LKG configuration cache to store each valid base
configuration instead of a single entry of configuration
[AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet#2007
- Add encryption keys to base configuration
[AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet#2023
- Updated CHANGELOG link
[AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet#2026

</details>

---

### Configuration

📅 **Schedule**: Branch creation - "after 10pm every weekday,every
weekend,before 5am every weekday" in timezone Europe/Berlin, Automerge -
At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://app.renovatebot.com/dashboard#github/orso-co/Orso.Arpa.Api).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNS4zNC4xIiwidXBkYXRlZEluVmVyIjoiMzUuMzQuMSJ9-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
@brentschmaltz brentschmaltz deleted the Xiao/FetchMetadataLog branch June 2, 2023 21:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FeatureRequest] Cache the previous "richer error details" in ConfigurationManager.GetConfigurationAsync.
2 participants