Support SNS batch publishing #1335

martincostello · 2024-02-01T15:14:10Z

Add support for batch publishing messages to SNS.

Supersedes #1098.

TODO

~~Review original feedback from GH-1095 Add support to batch operation #1098 and address.~~
~~Check test coverage.~~
~~Rebase on Add public API analyzer #1330 and add updated baselines.~~
~~Update version to 7.2.0.~~

samples/src/JustSaying.Sample.Restaurant.OrderingApi/Program.cs

codecov · 2024-02-01T15:18:05Z

Codecov Report

Attention: Patch coverage is 74.55471% with 100 lines in your changes missing coverage. Please review.

Project coverage is 78.34%. Comparing base (0abb907) to head (a23f503).
Report is 28 commits behind head on main.

Files with missing lines	Patch %	Lines
src/JustSaying/JustSayingBus.cs	66.66%	23 Missing and 2 partials ⚠️
...ng/AwsTools/MessageHandling/SqsMessagePublisher.cs	68.42%	20 Missing and 4 partials ⚠️
...ng/AwsTools/MessageHandling/SnsMessagePublisher.cs	79.12%	15 Missing and 4 partials ⚠️
...JustSaying/Fluent/MessagingConfigurationBuilder.cs	27.77%	10 Missing and 3 partials ⚠️
...ng/Fluent/PublishConfig/DynamicMessagePublisher.cs	77.77%	6 Missing and 2 partials ⚠️
....StructureMap/ConfigurationExpressionExtensions.cs	76.47%	3 Missing and 1 partial ⚠️
.../AwsTools/MessageHandling/PublishBatchException.cs	33.33%	4 Missing ⚠️
...tSaying/Fluent/TopicAddressPublicationBuilder`1.cs	77.77%	2 Missing ⚠️
src/JustSaying/MessagingBusBuilder.cs	88.88%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1335      +/-   ##
==========================================
- Coverage   79.49%   78.34%   -1.16%     
==========================================
  Files         138      140       +2     
  Lines        3234     3556     +322     
  Branches      448      501      +53     
==========================================
+ Hits         2571     2786     +215     
- Misses        433      522      +89     
- Partials      230      248      +18

Flag	Coverage Δ
linux	`78.34% <74.55%> (-1.10%)`	⬇️
macos	`46.45% <35.62%> (-1.60%)`	⬇️
windows	`46.45% <35.62%> (-1.54%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

martincostello · 2024-02-01T15:20:25Z

Directory.Packages.props

+    <PackageVersion Include="AWSSDK.SimpleNotificationService" Version="3.7.300" />
+    <PackageVersion Include="AWSSDK.SQS" Version="3.7.300" />


These are the minimum versions that support .NET 8 and AoT, so I think they're worth raising the baseline to as part of getting the batch publishing support.

martincostello · 2024-02-01T15:42:55Z

src/JustSaying/Fluent/AccountAddressProvider.cs

@@ -106,10 +106,12 @@ public Uri GetQueueUriByConvention<T>()
    /// <returns>The <see cref="Uri"/> for this queue.</returns>
    public Uri GetQueueUri(string queueName)
    {
+#pragma warning disable CS0618 // Type or member is obsolete
        var hostname = _regionEndpoint.GetEndpointForService("sqs").Hostname;


This is obsolete, but using the new way is difficult to do for libraries without breaking compatibility or making things harder for consumers, so keeping using the old way.

This will need revisiting with AWSSDK v4 as this has been removed completely.

martincostello · 2024-02-01T15:44:13Z

src/JustSaying/Fluent/TopicAddressPublisher.cs

This was internal and just syntactic sugar, so I just removed it.

src/JustSaying/Fluent/PublishConfig/DynamicMessagePublisher.cs

Add support for batch publishing messages to SNS. Supersedes justeattakeaway#1098. Co-Authored-By: Rafael Lillo <7280959+lillo42@users.noreply.github.com>

- Remove some new methods to reduce new public API surface. - Add missing XML documentation. - Some minor code style fix-ups.

Apply outstanding review comments from justeattakeaway#1098.

Add more coverage for batch publishing.

Fix two broken tests.

Ensure that messages with dynamic topics are not all sent to the topic associated with the first message in a batch.

Extend the `Interrogate()` response for a dynamic message publisher to return the publishers and the batch publishers.

Condense the syntax.

Manual editing meant a missed comma.

Refactor things to make the public API analyser happy.

Add new unshipped members to baselines.

Fix test broken by 366fd64.

Use the explicit token option, rather than setting an environment variable.

Add new batch publishing APIs.

Bump version to 7.2.

martincostello · 2024-08-28T13:37:26Z

This should be ready to test at your convenience now @hwoodiwiss.

hwoodiwiss · 2024-08-28T16:00:06Z

Tested internally, looks good to me, it worked well. 🎉

martincostello · 2024-08-28T16:01:04Z

@slang25 Do we want to make this part of 8? If so, I'll update the version number again before merging.

hwoodiwiss

I've reviewed down to src/JustSaying/IPublishBatchConfiguration.cs, I'll try and get back to this soon. Just a few minor changes so far.

src/JustSaying/AwsTools/MessageHandling/SnsMessagePublisher.cs

src/JustSaying/AwsTools/JustSayingConstants.cs

src/JustSaying/AwsTools/MessageHandling/SnsMessagePublisher.cs

hwoodiwiss · 2024-08-29T09:48:40Z

src/JustSaying/AwsTools/MessageHandling/SnsMessagePublisher.cs

+
+                if (response.Successful.Count > 0 && _logger.IsEnabled(LogLevel.Information))
+                {
+                    _logger.LogInformation(


I didn't see this success log in my testing, I only saw the one that I'd added, though it did definitely successfully send the notifications.

Did you have a log filter configured on JustSaying messages?

No, and in the previous version, we do get the logs for individual message publishes. I'll have another look tomorrow.

src/JustSaying/AwsTools/MessageHandling/SqsMessagePublisher.cs

src/JustSaying/Fluent/MessagingConfigurationBuilder.cs

Apply feedback to comments and log messages.

hwoodiwiss

Just a few more spelling bits, and a comment on the API surface

src/JustSaying/AwsTools/MessageHandling/SnsMessagePublisher.cs

src/JustSaying/AwsTools/MessageHandling/SqsMessagePublisher.cs

src/JustSaying/JustSayingBus.cs

src/JustSaying/MessagingBusBuilder.cs

hwoodiwiss

I don't think these are worth blocking over, I just think it would be clearer if we disambiguated the log messages for batches.

hwoodiwiss · 2024-09-01T19:46:01Z

src/JustSaying/JustSayingBus.cs

+    {
+        if (PublishBatchConfiguration.PublishFailureReAttempts == 0)
+        {
+            _log.LogWarning("You have not set a re-attempt value for publish failures. If the publish location is not available you may lose messages.");


Suggested change

_log.LogWarning("You have not set a re-attempt value for publish failures. If the publish location is not available you may lose messages.");

_log.LogWarning("You have not set a re-attempt value for batch publish failures. If the publish location is not available you may lose messages.");

src/JustSaying/JustSayingBus.cs

hwoodiwiss

A few more spellings

tests/JustSaying.IntegrationTests/Fluent/Publishing/WhenAMessageIsPublishedToAQueue.cs

tests/JustSaying.IntegrationTests/Fluent/Publishing/WhenAMessageIsPublishedToATopic.cs

- Fix typos. - Make messages more specific to batches. - Style refactor.

- Extend the integration tests to include publishing to multiple topics in a single batch. - Fix broken test assertion after previous refactor. - Disable IDE0130.

hwoodiwiss · 2024-09-02T15:56:11Z

Do we still want to bump the major version for this, or do we want to have this as the last big thing for v7, as it should be backwards-compatible.

slang25 · 2024-09-02T16:06:23Z

A big part of me wants to put this in to a V8. The compression work is almost done (but there is a lot to review).

martincostello · 2024-09-24T08:06:29Z

@slang25 What do you want to do with this PR? Is it ready to merge, or does it need to wait on something else?

Use 3.7.400 series.

Fix-up attempt to use newer AWS SDK packages.

martincostello · 2024-10-01T10:17:26Z

Unless there's any objections in the next 24 hours, I'm going to merge this tomorrow and it'll just sit in main until whenever we do another release.

martincostello added enhancement dependencies Pull requests that update a dependency file .NET Pull requests that update .net code labels Feb 1, 2024

martincostello added this to the Future milestone Feb 1, 2024

github-advanced-security bot found potential problems Feb 1, 2024

View reviewed changes

samples/src/JustSaying.Sample.Restaurant.OrderingApi/Program.cs Fixed Show fixed Hide fixed

martincostello commented Feb 1, 2024

View reviewed changes

src/JustSaying/Fluent/TopicAddressPublisher.cs Outdated

Copy link

Member Author

martincostello Feb 1, 2024

Choose a reason for hiding this comment

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

This was internal and just syntactic sugar, so I just removed it.

martincostello commented Feb 1, 2024

View reviewed changes

src/JustSaying/Fluent/PublishConfig/DynamicMessagePublisher.cs Outdated Show resolved Hide resolved

martincostello modified the milestones: Future, v7.2.0 Feb 1, 2024

martincostello force-pushed the support-batch-publishing branch from 1cc99f5 to 40d62da Compare February 2, 2024 09:01

This was referenced Feb 17, 2024

Support Native AOT #1348

Draft

Add .NET 8 TFM #1349

Merged

martincostello force-pushed the support-batch-publishing branch from 3dad5d1 to 51797df Compare February 19, 2024 08:21

martincostello and others added 14 commits April 8, 2024 09:41

Support SNS batch publishing

6582497

Add support for batch publishing messages to SNS. Supersedes justeattakeaway#1098. Co-Authored-By: Rafael Lillo <7280959+lillo42@users.noreply.github.com>

Code clean-up

be7382e

- Remove some new methods to reduce new public API surface. - Add missing XML documentation. - Some minor code style fix-ups.

Apply code review feedback

8421ff4

Apply outstanding review comments from justeattakeaway#1098.

Extend test coverage

ad89219

Add more coverage for batch publishing.

Fix tests

c49b351

Fix two broken tests.

Group batch by topic

714f999

Ensure that messages with dynamic topics are not all sent to the topic associated with the first message in a batch.

Fix test

f60e93d

Extend the `Interrogate()` response for a dynamic message publisher to return the publishers and the batch publishers.

Simplify scope

efa5332

Condense the syntax.

Fix approval

35f8115

Manual editing meant a missed comma.

Fix compatibility warnings

e5737a2

Refactor things to make the public API analyser happy.

Update public API

43524f0

Add new unshipped members to baselines.

Fix test

96d378d

Fix test broken by 366fd64.

Use token option

7b7f138

Use the explicit token option, rather than setting an environment variable.

Update baseline for net8.0

5aa9601

Add new batch publishing APIs.

martincostello force-pushed the support-batch-publishing branch from 51797df to 5aa9601 Compare April 8, 2024 08:42

martincostello added 2 commits April 8, 2024 09:43

Bump version

c50fe43

Bump version to 7.2.

Merge branch 'main' into support-batch-publishing

346407e

martincostello marked this pull request as ready for review August 28, 2024 16:00

martincostello requested a review from a team as a code owner August 28, 2024 16:00

hwoodiwiss requested changes Aug 29, 2024

View reviewed changes

Fix comments and logs

4923c4a

Apply feedback to comments and log messages.

hwoodiwiss requested changes Sep 1, 2024

View reviewed changes

hwoodiwiss reviewed Sep 1, 2024

View reviewed changes

tests/JustSaying.IntegrationTests/Fluent/Publishing/WhenAMessageIsPublishedToAQueue.cs Outdated Show resolved Hide resolved

tests/JustSaying.IntegrationTests/Fluent/Publishing/WhenAMessageIsPublishedToATopic.cs Outdated Show resolved Hide resolved

martincostello added 2 commits September 2, 2024 08:47

Review feedback

96c994d

- Fix typos. - Make messages more specific to batches. - Style refactor.

Extend tests

42dbb52

- Extend the integration tests to include publishing to multiple topics in a single batch. - Fix broken test assertion after previous refactor. - Disable IDE0130.

hwoodiwiss approved these changes Sep 2, 2024

View reviewed changes

martincostello requested a review from slang25 September 2, 2024 15:50

martincostello added 2 commits September 27, 2024 10:41

Update AWSSDKs

fd6cb49

Use 3.7.400 series.

Fix build

a23f503

Fix-up attempt to use newer AWS SDK packages.

martincostello merged commit 42fe0a8 into justeattakeaway:main Oct 3, 2024
12 of 14 checks passed

martincostello deleted the support-batch-publishing branch October 3, 2024 10:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support SNS batch publishing #1335

Support SNS batch publishing #1335

martincostello commented Feb 1, 2024 •

edited

Loading

codecov bot commented Feb 1, 2024 •

edited

Loading

martincostello Feb 1, 2024 •

edited

Loading

martincostello Feb 1, 2024

martincostello Aug 28, 2024

martincostello Feb 1, 2024

martincostello commented Aug 28, 2024

hwoodiwiss commented Aug 28, 2024

martincostello commented Aug 28, 2024

hwoodiwiss left a comment

hwoodiwiss Aug 29, 2024

martincostello Aug 29, 2024

hwoodiwiss Aug 29, 2024

hwoodiwiss left a comment

hwoodiwiss left a comment

hwoodiwiss Sep 1, 2024

hwoodiwiss left a comment

hwoodiwiss commented Sep 2, 2024

slang25 commented Sep 2, 2024 •

edited

Loading

martincostello commented Sep 24, 2024

martincostello commented Oct 1, 2024

		<PackageVersion Include="AWSSDK.SimpleNotificationService" Version="3.7.300" />
		<PackageVersion Include="AWSSDK.SQS" Version="3.7.300" />

	_log.LogWarning("You have not set a re-attempt value for publish failures. If the publish location is not available you may lose messages.");
	_log.LogWarning("You have not set a re-attempt value for batch publish failures. If the publish location is not available you may lose messages.");

Support SNS batch publishing #1335

Support SNS batch publishing #1335

Conversation

martincostello commented Feb 1, 2024 • edited Loading

TODO

codecov bot commented Feb 1, 2024 • edited Loading

Codecov Report

martincostello Feb 1, 2024 • edited Loading

Choose a reason for hiding this comment

martincostello Feb 1, 2024

Choose a reason for hiding this comment

martincostello Aug 28, 2024

Choose a reason for hiding this comment

martincostello Feb 1, 2024

Choose a reason for hiding this comment

martincostello commented Aug 28, 2024

hwoodiwiss commented Aug 28, 2024

martincostello commented Aug 28, 2024

hwoodiwiss left a comment

Choose a reason for hiding this comment

hwoodiwiss Aug 29, 2024

Choose a reason for hiding this comment

martincostello Aug 29, 2024

Choose a reason for hiding this comment

hwoodiwiss Aug 29, 2024

Choose a reason for hiding this comment

hwoodiwiss left a comment

Choose a reason for hiding this comment

hwoodiwiss left a comment

Choose a reason for hiding this comment

hwoodiwiss Sep 1, 2024

Choose a reason for hiding this comment

hwoodiwiss left a comment

Choose a reason for hiding this comment

hwoodiwiss commented Sep 2, 2024

slang25 commented Sep 2, 2024 • edited Loading

martincostello commented Sep 24, 2024

martincostello commented Oct 1, 2024

martincostello commented Feb 1, 2024 •

edited

Loading

codecov bot commented Feb 1, 2024 •

edited

Loading

martincostello Feb 1, 2024 •

edited

Loading

slang25 commented Sep 2, 2024 •

edited

Loading