Fixes #49: Upgrade to new Azure Storage New SDK #51
Conversation
Removes message envelope for Azure Storage Queue This change removes the message envelope used to wrap queue data with metadata (correlation id, properties) in the Azure Storage Queue implementation. Azure Storage Queue only supports a message body, and the envelope was previously used to support CorrelationId and Properties. However, this approach breaks backward compatibility with existing messages. This commit prioritizes backward compatibility by directly serializing and deserializing the message body. As a result, CorrelationId and Properties from QueueEntryOptions are no longer persisted. Additionally, the tests that rely on the envelope functionality have been skipped.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
niemyjski
added
enhancement
dependencies
|
@dviry can you please try this nightly and let us know if you find any issues. |
There was a problem hiding this comment.
Pull request overview
This PR upgrades Foundatio.AzureStorage from the legacy Microsoft.Azure.Storage SDK v11 to the modern Azure.Storage SDK v12. The most significant change is the removal of the message envelope wrapper for Azure Storage Queue to maintain backward compatibility with existing queued messages. This means CorrelationId and Properties from QueueEntryOptions are no longer persisted.
Key Changes
- SDK Migration: Replaced Microsoft.Azure.Storage.Blob v11.2.3 and Microsoft.Azure.Storage.Queue v11.2.3 with Azure.Storage.Blobs v12.24.0 and Azure.Storage.Queues v12.22.0
- Queue Message Format: Removed envelope wrapper to ensure backward compatibility, directly serializing message bodies (CorrelationId and Properties no longer supported)
- Retry Logic Modernization: Replaced IRetryPolicy with functional Func<int, TimeSpan> RetryDelay for more flexible retry handling
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Foundatio.AzureStorage/Foundatio.AzureStorage.csproj | Updated package references from v11 to v12 Azure Storage SDKs |
| src/Foundatio.AzureStorage/Storage/AzureFileStorage.cs | Migrated to Azure.Storage.Blobs v12 API with updated error handling, synchronous CreateIfNotExists, and new BlobClientOptions for retry configuration |
| src/Foundatio.AzureStorage/Storage/AzureFileStorageOptions.cs | Added ConfigureRetry action for Azure SDK retry configuration |
| src/Foundatio.AzureStorage/Queues/AzureStorageQueue.cs | Migrated to Azure.Storage.Queues v12 with lazy QueueClient initialization, improved error handling for expired visibility timeouts, and functional retry delay |
| src/Foundatio.AzureStorage/Queues/AzureStorageQueueOptions.cs | Added RetryDelay function, UseBase64Encoding migration flag, and ConfigureRetry action |
| src/Foundatio.AzureStorage/Queues/AzureStorageQueueEntry.cs | Updated to use QueueMessage from v12 SDK and added PopReceipt tracking for message updates |
| src/Foundatio.AzureStorage/Extensions/StorageExtensions.cs | Removed obsolete extension methods (moved inline) |
| tests/Foundatio.AzureStorage.Tests/Storage/AzureStorageTests.cs | Updated test to use new v12 BlobClient API |
| tests/Foundatio.AzureStorage.Tests/Queues/AzureStorageQueueTests.cs | Updated tests with clarified skip reasons and new RetryDelay function usage |
tests/Foundatio.AzureStorage.Tests/Queues/AzureStorageQueueTests.cs
Outdated
Show resolved
Hide resolved
tests/Foundatio.AzureStorage.Tests/Storage/AzureStorageTests.cs
Outdated
Show resolved
Hide resolved
ejsmith
left a comment
ejsmith
left a comment
There was a problem hiding this comment.
It would be nice if correlation id and other metadata passed through with the queue message. I wonder instead of having the UseBase64Encoding setting if we should have a backwards compat mode that uses base64 and no message envelope and then by default it uses an envelope.
Introduces a compatibility mode to handle legacy messages and enables support for CorrelationId and Properties. This change introduces a `CompatibilityMode` option to the `AzureStorageQueue` to support different message formats. The `Default` mode uses an envelope to wrap the message, preserving `CorrelationId` and `Properties`. The `Legacy` mode supports older messages serialized using the v11 SDK, ensuring backward compatibility during migration.
| /// A function that returns the delay before retrying a failed message based on the attempt number. | ||
| /// Default is exponential backoff with jitter: 2^attempt seconds + random 0-100ms. | ||
| /// </summary> | ||
| public Func<int, TimeSpan> RetryDelay { get; set; } = attempt => |
There was a problem hiding this comment.
a helper used to exist, and was replaced in favor of ResiliencePolicy - which IMO is a LOT more flexible and should be restored in here...
There was a problem hiding this comment.
Retries are a native part of the latest azure sdk, and will attempt to use failover and georeplicas if necessary, all transparent to the user. I'd not use a resilience policy here, as that would actually exponentially change the overall retries
You can shut that feature of the sdk off, but if using ms.extensions.azure and DI, a lot of it is just baked in and impacts app wide azure sdks unless you do per-client disabling.
Just a thought
There was a problem hiding this comment.
This is only used for AbandonAsync for updating the message visibility. This has nothing todo with our resilience policy or something we would retry. I however, wonder if we should build some helpers into foundatio for making this easier to define.
May I ask what was the reason for removing those? Without a CorrelationId the whole Telemetry logging is worthless...
Another reason to keep it... Not to mention future features (stuff like Req/Res, Inbox/Outbox, etc...) will need additionally metadata that can only be stored in the Envelope (Azure Queue Messages unfortunately have no Metadata)
IMO there should always be an Envelope without any "compat mode" switches. |
|
See the last commit where I added a compatibility mode. We have to support backwards compatibility for anyone upgrading. We enqueue millions of events a day for exceptionless service and use these queues, we need to make sure if needed we can revert and process old queued message format without downgrading, also allows for rolling upgrade and then can make the switch when queues are drained via configuration. We will get rid of compatibility mode in the future. |
|
The azure storage sdk supports credential / token /identity based authentication and also has an entire DI/configuration library (Microsoft.Extensions.Azure). I'm wondering if there's a way you can support configuration with a pre-configured client so that people using the Ms.E library to configure all their azure sdk usage can continue to use it with Foundatio as well. It sets up retries, logging, telemetry, geo-replica fallback, and more on a global or per-client scale and works with all different types of clients (queues, blob, vault, service bus). It feels like you may be precluding an audience by instantiating the queueclient yourself |
Upgrades the Azure Storage SDK to the latest version and bumps the MinVer package version. This ensures compatibility with the latest Azure Storage features and benefits from bug fixes and performance improvements. Also, removes some unnecessary code in the AzureStorageQueue.cs.
@pinkfloydx33 if you'd like to create a pr for this, we could probably pass in a queue reference via options. I'm not sure if other changes would need to be made to ignore a bunch of options. |
Updates the Azure Storage Blobs and Queues SDKs to the latest versions. This ensures access to the newest features, performance improvements, and security patches offered by the Azure Storage services.
…null' Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 17 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tests/Foundatio.AzureStorage.Tests/Queues/LegacyAzureStorageQueueTests.cs
Show resolved
Hide resolved
Updates the method used to retrieve blobs from Azure Blob Storage to use the GetBlobsOptions object for specifying the prefix. This change ensures compatibility with newer versions of the Azure Blob Storage SDK.
Ensures azurite is healthy before other services depend on it. Azure/Azurite#2623 This change adds a healthcheck to the azurite service in docker-compose.yml. Also ensures that the `ready` service waits for azurite to be healthy, rather than just running, preventing potential issues if azurite is not fully initialized. Improves reliability and robustness during startup.
niemyjski
force-pushed
the
feature/upgrade-storage-sdk
branch
from
7829829 to
1666c2e
Compare
Updates Azure Storage Queue to handle message retrieval cancellation gracefully and correctly processes messages when moving to the deadletter queue in Default mode. This ensures data integrity and prevents potential issues with message processing. Also adds logging to indicate when an Azure File Storage container already exists.
|
Thank you all for your help to get this across! Let's continue to improve this via feedback and pr's :) |
Removes message envelope for Azure Storage Queue
This change removes the message envelope used to wrap queue data with metadata (correlation id, properties) in the Azure Storage Queue implementation.
Azure Storage Queue only supports a message body, and the envelope was previously used to support CorrelationId and Properties. However, this approach breaks backward compatibility with existing messages.
This commit prioritizes backward compatibility by directly serializing and deserializing the message body. As a result, CorrelationId and Properties from QueueEntryOptions are no longer persisted. Additionally, the tests that rely on the envelope functionality have been skipped.
This needs to be tested but local integration tests are passing. We need to see if any additional changes are needed to handle existing queued messages.