Conversation
|
@codex could you review this please? |
|
@codex could you review the most recent commit on this PR please? |
…ent parsing JsonSerializer.SerializeToUtf8Bytes on an already-JSON string wraps it in quotes, breaking DecodeStructuredModeMessageAsync. Switch to Encoding.UTF8.GetBytes to pass the raw JSON bytes unchanged across Kafka, Service Bus, and Google Pub/Sub workers. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
All messaging drivers (Kafka, Service Bus, EventBridge, GCP Pub/Sub) and Aspire dashboard commands were publishing raw JSON payloads. Consumers call DecodeStructuredModeMessageAsync and expect a full CloudEvents envelope, so messages were never successfully processed. - Use JsonEventFormatter + JsonDocument to encode structured CloudEvents - Fix GooglePubSubMessaging to route by topic via a per-topic publisher dictionary instead of a hardcoded stickerClaimed client - Add CloudNative.CloudEvents.SystemTextJson to the Aspire project Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
…per poll Both KafkaStickerPrintedWorker and KafakStickerClaimedWorker were recreating a new IConsumer on every PollAsync() call, which triggered a Kafka group rebalance cycle on each iteration. In CI environments this rebalance overhead (2-3s) competed with the 2s Consume() timeout, causing the WhenStickerIsPrintedThenAUsersPrintedCountShouldIncrement integration test to fail intermittently. Consumers are now created once in StartAsync() and subscribed to their topic, reused across all PollAsync() calls, and closed in StopAsync(). Service scope creation is also moved inside the message-present branch to avoid unnecessary allocations on idle polls. Additional changes: - Add IKafkaConsumerFactory abstraction to enable unit testing of consumer lifecycle - Add 8 unit tests covering StartAsync/PollAsync/StopAsync lifecycle for both workers - Fix Log.ReceivedMessage level from Critical to Information Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
|
@jeastham1993 give us a ping when you would like me to look at this! |
|
@codex could you review this please? |
print-service/src/Stickerlandia.PrintService.Agnostic/KafkaEventPublisher.cs
Outdated
Show resolved
Hide resolved
print-service/src/Stickerlandia.PrintService.Agnostic/ServiceExtensions.cs
Outdated
Show resolved
Hide resolved
| var data = formatter.EncodeStructuredModeMessage(cloudEvent, out _); | ||
| var jsonString = System.Text.Encoding.UTF8.GetString(data.Span); | ||
|
|
||
| if (Tracer.Instance.ActiveScope is not null) |
There was a problem hiding this comment.
This adds DSM support to the AWS publishing
There was a problem hiding this comment.
We never call TrackTransactionAsync in this path, only in the kafka worker, which seems like it is needed
print-service/src/Stickerlandia.PrintService.Client/Telemetry/DatadogTransactionTracker.cs
Outdated
Show resolved
Hide resolved
print-service/src/Stickerlandia.PrintService.Core/Observability/DatadogTransactionTracker.cs
Show resolved
Hide resolved
...t-service/src/Stickerlandia.PrintService.Core/PrintJobs/AcknowledgePrintJobCommandHandler.cs
Show resolved
Hide resolved
user-management/src/Stickerlandia.UserManagement.Agnostic/KafkaStickerPrintedWorker.cs
Show resolved
Hide resolved
…ntBridge SetHeader was mutating a parsed JsonNode copy but never serializing it back, so _datadog tracing/DSM headers were silently dropped from all EventBridge payloads. Parse jsonString into a JsonNode before injection, pass it as the carrier, then reassign jsonString from ToJsonString() so PutEventsAsync sends the updated payload. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 70623caee6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
...t-service/src/Stickerlandia.PrintService.Core/PrintJobs/AcknowledgePrintJobCommandHandler.cs
Outdated
Show resolved
Hide resolved
user-management/src/Stickerlandia.UserManagement.Aspire/AppBuilderExtensions.cs
Show resolved
Hide resolved
… adapter Without these registrations, resolving AcknowledgePrintJobCommandHandler in DRIVEN=AWS deployments failed with an unresolved-service exception. Also adds Activity span tagging for dsm.transaction_id and dsm.transaction.checkpoint in both Core and Client transaction trackers. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
WithServiceBusTestCommands sends structured CloudEvents with ContentType application/cloudevents+json, but the worker was deserializing the raw body directly to StickerClaimedEventV1, missing the nested data field and producing empty AccountId/StickerId values that dead-lettered. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ecd11fe3da
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
user-management/src/Stickerlandia.UserManagement.Agnostic/KafkaStickerPrintedWorker.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0346c2f6c1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
print-service/src/Stickerlandia.PrintService.AWS/EventBridgeEventPublisher.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8dff6e93cb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ce1040cbfd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
user-management/src/Stickerlandia.UserManagement.AWS/SqsStickerPrintedWorker.cs
Outdated
Show resolved
Hide resolved
| // Complete the message | ||
| await args.CompleteMessageAsync(args.Message, CancellationToken.None); |
print-service/src/Stickerlandia.PrintService.Core/Observability/LogMessages.cs
Outdated
Show resolved
Hide resolved
print-service/src/Stickerlandia.PrintService.Core/Observability/LogMessages.cs
Show resolved
Hide resolved
user-management/tests/Stickerlandia.UserManagement.IntegrationTest/AccountTests.cs
Outdated
Show resolved
Hide resolved
...anagement/src/Stickerlandia.UserManagement.Core/StickerPrintedEvent/StickerPrintedHandler.cs
Show resolved
Hide resolved
user-management/src/Stickerlandia.UserManagement.Agnostic/KafkaStickerClaimedWorker.cs
Show resolved
Hide resolved
- Fix wrong SubscribeOperation type in ServiceBusStickerPrintedWorker (was StickerClaimedEventV1, now StickerPrintedEventV1) - Align envelope handling in ServiceBusStickerPrintedWorker to match ServiceBusStickerClaimedWorker (check ContentType, handle both CloudEvents and plain JSON) - Register printJobs.completed.v1 Service Bus queue in Aspire setup so ServiceBusStickerPrintedWorker doesn't fail on Azure startup - Fix activity tag in StickerPrintedHandler: stickerClaim.failed -> stickerPrint.failed - Add unit tests for StickerPrintedHandler covering null event, empty userId, user not found, and successful update paths - Inject DatadogTransactionTracker into EventBridgeEventPublisher and call TrackTransactionAsync after publishing PrintJobQueuedEvent, matching the Kafka publisher behaviour - Fix DLQ producer in KafkaStickerPrintedWorker: create once in StartAsync and reuse, rather than creating a new instance per call - Remove duplicate StopAsync override in StickerPrintedWorker and StickerClaimedWorker that was calling messagingWorker.StopAsync twice (already called in ExecuteAsync finally block) - Fix log level mismatches in LogMessages.cs: ReceivedMessage Critical->Information, GenericWarning Error->Warning - Fix duplicate EventId 5 in LogMessages.cs: StoppingMessageProcessor reassigned to EventId 13 - Fix copy-pasta comment in integration test: "claimed sticker count" -> "printed sticker count" - Rename KafakStickerClaimedWorker -> KafkaStickerClaimedWorker (filename and class name typo) Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
scottgerring
left a comment
There was a problem hiding this comment.
@jeastham1993 went through this again; can you fix merge conflicts and address or resolve the codex comments?
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a237e7128e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| request.Content.Headers.Add("DD-API-KEY", _ddApiKey); | ||
|
|
||
| var response = await client.SendAsync(request); | ||
| response.EnsureSuccessStatusCode(); |
There was a problem hiding this comment.
Skip Datadog POST when API key is unset
TrackTransactionAsync always sends a request to the Datadog pipeline endpoint even though _ddApiKey defaults to an empty string when DD_API_KEY is missing. In environments without that secret (common in local/dev/CI), every queued/acknowledged print path now does an outbound HTTP call that is guaranteed to fail and is still awaited by callers, adding avoidable latency and warning noise for each job. Add an early return when the key is empty (using the existing TransactionTrackingSkipped log) before constructing/sending the request.
Useful? React with 👍 / 👎.
Code reviewFound 3 issues:
🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
- SqsStickerPrintedWorker: unwrap EventBridge envelope before deserializing (was deserializing message.Body directly as StickerPrintedEventV1, missing the CloudWatchEvent<JsonElement> outer envelope; mirrored StickerPrintedSqsHandler) - ServiceBusStickerPrintedWorker: add return after DeadLetterMessageAsync calls so CompleteMessageAsync is never called on an already-settled message - PrintService.Client: remove duplicate DatadogTransactionTracker; reference Core implementation via IDatadogTransactionTracker instead
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 148ea84281
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (_task is not null) | ||
| { | ||
| await _task; |
There was a problem hiding this comment.
Avoid blocking poll loop on Pub/Sub subscriber task
PollAsync awaits _task, but _task is created by subscriber.StartAsync(ProcessMessageAsync) and is the long-running subscriber task that only completes after StopAsync. This makes the first poll call block indefinitely, so the hosted worker loop can no longer observe cancellation promptly or reach its finally block to stop the subscriber during shutdown/redeploy in DRIVEN=GCP worker runs.
Useful? React with 👍 / 👎.
| }; | ||
|
|
||
|
|
||
| var serviceBusMessage = new ServiceBusMessage(Encoding.UTF8.GetBytes(messageJson)); |
There was a problem hiding this comment.
Set CloudEvent content type in Azure test messaging driver
The integration driver now sends raw JSON bytes without a ContentType, but the printed Service Bus worker only unwraps structured CloudEvents when ContentType is application/cloudevents+json. For printJobs.completed.v1 test injections (which are now structured CloudEvents), this path falls back to direct StickerPrintedEventV1 deserialization and fails to extract userId, so Azure integration runs can loop/retry instead of incrementing printed counts.
Useful? React with 👍 / 👎.
…nts content type in Azure test driver - Add early return in TrackTransactionAsync when _ddApiKey is empty to avoid guaranteed-to-fail outbound HTTP calls in local/dev/CI environments - Set ContentType = "application/cloudevents+json" on ServiceBusMessage in AzureServiceBusMessaging so test-injected printJobs.completed.v1 messages hit the structured CloudEvents deserialization path in the worker
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3e01435131
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| var serviceBusMessage = new ServiceBusMessage(Encoding.UTF8.GetBytes(messageJson)) | ||
| { | ||
| ContentType = "application/json" | ||
| ContentType = "application/cloudevents+json" |
There was a problem hiding this comment.
Send claimed Azure test messages with JSON content type
AzureServiceBusMessaging.SendMessageAsync now labels every injected message as application/cloudevents+json, but claimed-message tests still publish plain StickerClaimedEventV1 JSON (see AccountDriver.InjectStickerClaimedMessage). In ServiceBusStickerClaimedWorker, that content type forces the CloudEvent parsing branch, which expects a top-level data field and cannot deserialize the claimed payload, so claimed events are dead-lettered instead of incrementing counts in DRIVEN=AZURE integration runs.
Useful? React with 👍 / 👎.
Add additional event handler to the user management service to handle when a user prints a sticker.
This PR adds a new event flow: when a print job completes, the user-management service handles that event and increments the user's printed sticker count.
Deets
Print-service side: The print service now publishes a
printJobs.completed.v1event (via Kafka and EventBridge) when a print job is acknowledged, with manual DSM header injection for Datadog tracing. It also addsDatadogTransactionTrackerfor tracking transactions through the pipeline.User-management side: New
StickerPrintedHandlerin Core handles the event by looking up the user and incrementingPrintedStickerCount(new DB column via migration). New workers consume this event across all four platform adapters — Kafka, SQS, ServiceBus, GCP Pub/Sub, and Lambda — mirroring the existingStickerClaimedpattern.Infrastructure: AWS CDK adds SQS queues/DLQs and Lambda wiring for the new event. Aspire adds Kafka topics. Docker-compose gets updated for local dev. We've not tested the azure stuff yet.