-
Notifications
You must be signed in to change notification settings - Fork 686
Add tracing support for Azure App Configuration component #9323
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
Add tracing support for Azure App Configuration component #9323
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This pull request upgrades the Azure App Configuration package to version 8.2.0 and adds OpenTelemetry tracing support along with the corresponding configuration and tests.
- Updated JSON configuration schema and tests to include the new "DisableTracing" property.
- Implemented tracing enablement/disablement logic in component settings and extension methods.
- Updated package references to the new version.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
tests/Aspire.Microsoft.Extensions.Configuration.AzureAppConfiguration.Tests/ConformanceTests.cs | Added "DisableTracing" configuration property handling and implemented tracing method logic. |
tests/Aspire.Microsoft.Extensions.Configuration.AzureAppConfiguration.Tests/ConfigurationTests.cs | Added unit test to validate the default tracing behavior. |
src/Components/Aspire.Microsoft.Extensions.Configuration.AzureAppConfiguration/ConfigurationSchema.json | Updated schema to include the new "DisableTracing" property with description and default value. |
src/Components/Aspire.Microsoft.Extensions.Configuration.AzureAppConfiguration/AzureAppConfigurationSettings.cs | Added the "DisableTracing" property with documentation. |
src/Components/Aspire.Microsoft.Extensions.Configuration.AzureAppConfiguration/AspireAppConfigurationExtensions.cs | Modified extension method to conditionally add tracing support based on the new property. |
Directory.Packages.props | Upgraded package versions to 8.2.0. |
tests/Aspire.Microsoft.Extensions.Configuration.AzureAppConfiguration.Tests/ConformanceTests.cs
Outdated
Show resolved
Hide resolved
=> throw new NotImplementedException(); | ||
{ | ||
// wait for 1 seconds to ensure the refresh interval is exceeded | ||
Task.Delay(1000).ConfigureAwait(false).GetAwaiter().GetResult(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a better way to do this? Is there a way to "force" the refresh?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We used to have a "force" refresh api. But for some reason, we removed it. In our own testsuite, we also used "waiting for some time" way to test refresh. example
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good - assuming there is a better way to test besides waiting for a second.
Description
Upgraded
Microsoft.Extensions.Configuration.AzureAppConfiguration
to 8.2.0.Added tracing support for Azure App Configuration component.
Fixes # (issue)
Checklist
<remarks />
and<code />
elements on your triple slash comments?doc-idea
templatebreaking-change
templatediagnostic
template