Muhannad/UI visibality - #35
Conversation
…cs, and live operational events
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (47)
📝 WalkthroughWalkthroughThis PR introduces a complete operational observability stack for SW.Bus: an in-memory operational event pipeline with configurable buffering/batching/flushing, OpenTelemetry tracing and metrics instrumentation across publish/consume paths, a dashboard data aggregation service with alert evaluation, and a new ASP.NET Core web UI viewer dashboard with pluggable authentication (policy-based, HTTP Basic, or anonymous). ChangesOperational Events & Dashboard Infrastructure
Operations Viewer Dashboard
Documentation & Project Updates
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes The PR introduces substantial new infrastructure (operational events pipeline, metrics, dashboard service), extensive instrumentation across publish/consume paths, a complete new ASP.NET Core viewer project with UI/auth, and comprehensive documentation. The changes are heterogeneous (event contracts, channel-based buffering, ring-buffer storage, activity-based tracing, metrics, Razor Pages views, styling) and span multiple layers requiring understanding of the event flow, data aggregation logic, auth modes, and dashboard rendering patterns. Possibly related PRs
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
SW.Bus/ConsumerRunner.cs (1)
285-336:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
RunNodeMessageis missingMessageRetryScheduled/MessageMovedToDeadLetterevents.In
Run, the catch path emitsMessageProcessingFailedplus eitherMessageRetryScheduled(retry branch) orMessageMovedToDeadLetter(terminal branch).RunNodeMessageonly emitsMessageProcessingFailedat the end — the retry-scheduled and dead-letter operational events are missing for node listeners, which means the dashboard will under-report retries and DLQ moves originating from broadcast/listener flows.Consider emitting the same event triplet (or document why node listeners are intentionally excluded from retry/DLQ events).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@SW.Bus/ConsumerRunner.cs` around lines 285 - 336, RunNodeMessage currently only fires MessageProcessingFailed on exceptions, missing the MessageRetryScheduled and MessageMovedToDeadLetter operational events emitted by Run; update RunNodeMessage so that when the retry branch executes (where metrics.RetryScheduled.Add(1) and model.BasicReject(...) occur) it also FireAndForget a MessageRetryScheduled event with the same identifying fields used for MessageProcessingFailed, and when the terminal branch executes (where metrics.DeadLetterMoved.Add(1), model.BasicAck(...), and PublishBad(...) occur) it likewise FireAndForget a MessageMovedToDeadLetter event with the same contextual fields; locate the logic in RunNodeMessage, mirror the event payload construction used for MessageProcessingFailed, and reuse PublishBad, metrics.RetryScheduled, metrics.DeadLetterMoved and FireAndForget to emit the two missing events.SW.Bus/IServiceCollectionExtensions.cs (1)
147-160:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
AddBusPublishnow has an implicit ordering dependency onAddBus.
BasicPublisheris constructed withIOperationalEventPublisherandBusMetricsresolved viaserviceProvider.GetRequiredService<...>()— both are only registered insideAddBus(lines 78-80). Callers that invokeAddBusPublishwithout first callingAddBuswill hitInvalidOperationExceptionat scope creation. Consider documenting this requirement on theAddBusPublishXML comment or usingTryAddSingletondefaults so the publisher remains usable standalone.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@SW.Bus/IServiceCollectionExtensions.cs` around lines 147 - 160, AddBusPublish currently depends on services (IOperationalEventPublisher, BusMetrics) that are only registered by AddBus because BasicPublisher is constructed via serviceProvider.GetRequiredService<...>(), causing InvalidOperationException if AddBusPublish is used standalone; update AddBusPublish to either document the AddBus prerequisite in its XML comment or register safe defaults using Microsoft.Extensions.DependencyInjection.Extensions.TryAddSingleton for IOperationalEventPublisher and BusMetrics before calling AddScoped, or alter the BasicPublisher factory to use GetService and provide fallback instances when null so AddBusPublish can be used without AddBus.
🧹 Nitpick comments (26)
SW.Bus.RabbitMqViewer/wwwroot/bus-viewer.css (1)
29-29: 💤 Low valueMinor: Add empty line before declaration for consistency.
The stylelint rule expects an empty line before the
background-colordeclaration for better readability.♻️ Suggested fix
html[data-theme="dark"] { --pico-background-color: var(--bv-bg); + background-color: var(--bv-bg); color: var(--bv-text); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@SW.Bus.RabbitMqViewer/wwwroot/bus-viewer.css` at line 29, Insert a blank line immediately before the background-color declaration to satisfy the stylelint rule: locate the background-color: var(--bv-bg); declaration and add one empty line above it so there's a blank line separating it from the preceding rule or comment.SW.Bus/AlertEvaluator.cs (1)
70-76: ⚡ Quick winConsider making publish/ack imbalance thresholds configurable.
Lines 71 uses hardcoded values (
1.0msg/s for incoming rate,0.01msg/s for ack rate) while other alert thresholds (AlertRetryWarningThreshold,AlertDeadLetterCriticalThreshold, etc.) are configurable viaBusOptions. This inconsistency limits operators' ability to tune alert sensitivity for their specific workload patterns.♻️ Proposed fix to add configurable thresholds
First, add the configuration properties to
BusOptions.cs:public int AlertDeadLetterCriticalThreshold { get; set; } = 100; +/// <summary> +/// Gets or sets the minimum incoming message rate (msg/s) that triggers publish/ack imbalance checks. +/// Default: 1.0. +/// </summary> +public double AlertPublishRateThreshold { get; set; } = 1.0; + +/// <summary> +/// Gets or sets the maximum ack rate (msg/s) below which a publish/ack imbalance alert is triggered. +/// Default: 0.01. +/// </summary> +public double AlertAckRateThreshold { get; set; } = 0.01; + /// <summary> /// Gets or sets the cache duration in seconds for RabbitMQ management API monitoring data.Then update the alert evaluator:
// ── Publish / Ack imbalance ─────────────────────────────────────── -if (c.IncomingRate > 1.0 && c.AckRate < 0.01) +if (c.IncomingRate > busOptions.AlertPublishRateThreshold && c.AckRate < busOptions.AlertAckRateThreshold) alerts.Add(new DashboardAlert( AlertSeverity.Warning, "Publish / Ack Imbalance",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@SW.Bus/AlertEvaluator.cs` around lines 70 - 76, The Publish/Ack imbalance check in AlertEvaluator.cs currently uses hardcoded thresholds (c.IncomingRate > 1.0 and c.AckRate < 0.01); make these thresholds configurable by adding corresponding properties to BusOptions (e.g., PublishAckIncomingThreshold and PublishAckAckThreshold or similar names consistent with existing options like AlertRetryWarningThreshold), wire BusOptions into the AlertEvaluator (constructor or Evaluate method) and replace the hardcoded literals in the imbalance condition with the new BusOptions properties when creating the DashboardAlert so operators can tune sensitivity.SW.Bus/BusOptions.cs (1)
86-91: ⚡ Quick winAlign enforcement pattern with
MonitoringCacheSecondsfor consistency.The minimum value of 1000 is enforced in
InMemoryOperationalEventStore(line 23) viaMath.Max(1000, busOptions.OperationalEventsStoreCapacity), so the documentation is accurate. However, unlikeMonitoringCacheSecondswhich enforces constraints in its property setter (lines 130-135), enforcement forOperationalEventsStoreCapacityoccurs at the consumption point. Adding setter validation would make the design consistent across similar configuration properties:Suggested refactoring
-public int OperationalEventsStoreCapacity { get; set; } = 10000; +private int operationalEventsStoreCapacity = 10000; + +/// <summary> +/// Gets or sets the capacity of the in-memory operational event ring buffer. +/// When the buffer is full, oldest events are overwritten. +/// Default: 10000. Minimum enforced value: 1000. +/// </summary> +public int OperationalEventsStoreCapacity +{ + get => operationalEventsStoreCapacity; + set => operationalEventsStoreCapacity = Math.Max(value, 1000); +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@SW.Bus/BusOptions.cs` around lines 86 - 91, Add setter-side validation to BusOptions.OperationalEventsStoreCapacity so it enforces the same minimum as MonitoringCacheSeconds (e.g., set to Math.Max(1000, value) in the property setter) to keep configuration enforcement consistent; update the OperationalEventsStoreCapacity auto-property to a full property with a backing field or expression-bodied setter applying Math.Max(1000, value) and then you can remove or keep the defensive Math.Max in InMemoryOperationalEventStore but prefer removing redundant enforcement there to avoid double-normalization.SW.Bus.RabbitMqViewer/Areas/BusViewer/Pages/Queues.cshtml (1)
8-8: ⚡ Quick winRemove unused data fetch.
The page calls
GetQueueDetailsAsync()but never uses the result; the HTMX partial at line 12 immediately re-fetches the same data. This causes a redundant database/API call on every page load.♻️ Proposed fix
- var queues = await DataService.GetQueueDetailsAsync();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@SW.Bus.RabbitMqViewer/Areas/BusViewer/Pages/Queues.cshtml` at line 8, Remove the redundant call to DataService.GetQueueDetailsAsync() that fetches queue data but is never used; instead rely on the HTMX partial which re-fetches the same data. Locate the unused call to GetQueueDetailsAsync in the page code (the variable named "queues") and delete that line, ensuring there are no leftover references to the "queues" variable in Queues.cshtml and that the HTMX partial (the element triggering the same data fetch) remains responsible for retrieving and rendering queue details.SW.Bus.RabbitMqViewer/Areas/BusViewer/Pages/Events.cshtml (2)
17-21: ⚡ Quick winReplace hardcoded event names with a shared constant or enum.
The event names are hardcoded as magic strings in the view. If these names change or new events are added, they must be updated in multiple places. Consider defining them in a shared constant, enum, or fetching them from the backend.
♻️ Proposed approach
Define a shared constant in the backend:
public static class OperationalEventNames { public static readonly string[] All = new[] { "MessageProcessingStarted", "MessageProcessingCompleted", "MessageProcessingFailed", "MessageRetryScheduled", "MessageMovedToDeadLetter", "ConsumerConnected", "ConsumerDisconnected", "QueueBackpressureDetected", "PublishStarted", "PublishCompleted", "PublishFailed" }; }Then inject and use it in the view, or better yet, add a method to
IBusDashboardDataServiceto retrieve available event types.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@SW.Bus.RabbitMqViewer/Areas/BusViewer/Pages/Events.cshtml` around lines 17 - 21, The view currently hardcodes event name strings in the Events.cshtml foreach; extract these magic strings into a single shared source of truth (e.g., create a public static class OperationalEventNames with a public readonly string[] All or an enum) and update the view to consume that shared constant instead of the inline array; for a more robust approach add a method/property to IBusDashboardDataService (e.g., GetOperationalEventNames) that returns the list and have the Events.cshtml read from the injected service so future changes/additions are centralized.
39-39: 💤 Low valueConsider reducing the 5-second polling interval.
The events feed refreshes every 5 seconds, which is the most aggressive interval in the dashboard. For high-traffic systems, this could generate significant server load and network traffic. Consider increasing to 10 seconds to match other sections, or make the interval configurable.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@SW.Bus.RabbitMqViewer/Areas/BusViewer/Pages/Events.cshtml` at line 39, The hx-trigger attribute currently uses a 5s polling interval ("hx-trigger=\"load, every 5s\"") which is aggressive; change it to a 10s interval or make it configurable: replace the hard-coded "every 5s" with "every 10s" or bind it to a Razor/view-model/config value (e.g., EventsRefreshInterval) so the interval can be set from configuration and reused across pages; update any related documentation or config key name (like EventsRefreshInterval or BusViewerPollingInterval) and ensure the attribute uses that value.SW.Bus.RabbitMqViewer/Areas/BusViewer/Pages/Partials/Consumers.cshtml (1)
36-39: 💤 Low valueExtract health badge logic to a helper function.
The nested ternary for mapping
HealthStatusto display text is repeated logic that would benefit from extraction to a reusable helper.♻️ Proposed refactor
`@functions` { private string GetHealthBadgeText(AlertSeverity status) => status switch { AlertSeverity.Critical => "⚠ Critical", AlertSeverity.Warning => "△ Warning", _ => "✓ Healthy" }; }Then use:
- <td><span class="bv-badge bv-badge--@c.HealthStatus.ToString().ToLower()"> - @(c.HealthStatus == AlertSeverity.Critical ? "⚠ Critical" : - c.HealthStatus == AlertSeverity.Warning ? "△ Warning" : "✓ Healthy") - </span></td> + <td><span class="bv-badge bv-badge--@c.HealthStatus.ToString().ToLower()"> + `@GetHealthBadgeText`(c.HealthStatus) + </span></td>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@SW.Bus.RabbitMqViewer/Areas/BusViewer/Pages/Partials/Consumers.cshtml` around lines 36 - 39, Extract the nested ternary health-badge text logic into a helper method and call it from the Consumers.cshtml markup: add a private helper (e.g., GetHealthBadgeText(AlertSeverity status)) that returns "⚠ Critical" for AlertSeverity.Critical, "△ Warning" for AlertSeverity.Warning and "✓ Healthy" for all other values, then replace the inline ternary using c.HealthStatus with a call to GetHealthBadgeText(c.HealthStatus); keep the existing CSS class generation using c.HealthStatus.ToString().ToLower().SW.Bus.RabbitMqViewer/Areas/BusViewer/Pages/Retries.cshtml (2)
8-8: ⚡ Quick winRemove redundant data fetch.
Same pattern as in
Queues.cshtmlandDeadLetters.cshtml: data is fetched only to check if empty and display a count, then the HTMX partial re-fetches the same data immediately.♻️ Proposed fix
- var retries = await DataService.GetRetryAnalysisAsync(); -} - -@if (!retries.Any()) -{ - <article class="bv-card bv-card--ok"> - <p>✓ No retry backlogs detected. All consumers are processing cleanly.</p> - </article> -} -else -{ - <p class="bv-muted"> - <strong>@retries.Length</strong> consumer(s) have messages in retry queues. - Auto-refreshes every 10 s. - </p> - - <div id="bv-retry-table" +} + +<div id="bv-retry-table" hx-get="/bus-viewer/partials/retries" hx-trigger="load, every 10s" hx-swap="innerHTML"> - <p aria-busy="true">Loading…</p> - </div> -} + <p aria-busy="true">Loading…</p> +</div>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@SW.Bus.RabbitMqViewer/Areas/BusViewer/Pages/Retries.cshtml` at line 8, Remove the redundant upfront fetch of the full retry data in Retries.cshtml (the call to DataService.GetRetryAnalysisAsync())—instead either call a lightweight count-only method (e.g., DataService.GetRetryCountAsync()) to render the initial count or remove the fetch entirely and let the HTMX partial handle loading the full retry payload; update the view to use the count result (or placeholder) for the initial display and ensure the HTMX partial still targets the same partial endpoint so it immediately loads full data when triggered.
26-26: 💤 Low valueConsider standardizing HTMX refresh intervals.
The dashboard uses inconsistent auto-refresh intervals: Queues (10s), Retries (10s), Dead Letters (15s), Alerts (15s from
_Layout.cshtml). Standardizing these intervals would improve predictability and user experience.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@SW.Bus.RabbitMqViewer/Areas/BusViewer/Pages/Retries.cshtml` at line 26, The hx-trigger interval in Retries.cshtml is set to "load, every 10s" and should be standardized across the dashboard; update the hx-trigger attributes to a single agreed refresh interval (e.g., "load, every 15s") in Retries.cshtml (hx-trigger), Queues page (hx-trigger), DeadLetters page (hx-trigger), and any global refresh in _Layout.cshtml so all pages use the same interval for consistency. Ensure you update all occurrences of hx-trigger attributes or global HTMX refresh settings to the chosen value.SW.Bus.RabbitMqViewer/Areas/BusViewer/Pages/Partials/Queues.cshtml (1)
32-32: 💤 Low valueConsider simplifying nested ternary for row CSS class.
The nested ternary
@(q.DeadLetterMessages > 0 ? "bv-row--critical" : q.RetryMessages > 0 ? "bv-row--warning" : "")is readable but could be extracted to a helper for clarity.♻️ Proposed refactor
Add a helper function at the bottom of the file:
`@functions` { private string GetRowClass(int deadLetters, int retries) { if (deadLetters > 0) return "bv-row--critical"; if (retries > 0) return "bv-row--warning"; return string.Empty; } }Then use it:
- <tr class="@(q.DeadLetterMessages > 0 ? "bv-row--critical" : q.RetryMessages > 0 ? "bv-row--warning" : "")"> + <tr class="@GetRowClass(q.DeadLetterMessages, q.RetryMessages)">🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@SW.Bus.RabbitMqViewer/Areas/BusViewer/Pages/Partials/Queues.cshtml` at line 32, Extract the nested ternary used for the row CSS class into a small helper and call it from the markup: replace the expression @(q.DeadLetterMessages > 0 ? "bv-row--critical" : q.RetryMessages > 0 ? "bv-row--warning" : "") with a call to a new helper method GetRowClass(q.DeadLetterMessages, q.RetryMessages), and add the helper at the bottom of the Razor file (use `@functions` { private string GetRowClass(int deadLetters, int retries) { if (deadLetters > 0) return "bv-row--critical"; if (retries > 0) return "bv-row--warning"; return string.Empty; } } ) so the markup is clearer and behavior is unchanged.SW.Bus.RabbitMqViewer/Areas/BusViewer/Pages/DeadLetters.cshtml (1)
8-8: ⚡ Quick winRemove redundant data fetch.
Similar to
Queues.cshtml, this page fetches dead letter data only to check if the result is empty, then the HTMX partial immediately re-fetches the same data. Consider either:
- Removing the fetch and showing the HTMX container unconditionally, or
- Passing the fetched data to the partial on first load to avoid the double-fetch.
♻️ Option 1: Remove redundant fetch
- var deadLetters = await DataService.GetDeadLetterSummaryAsync(); -} - -@if (!deadLetters.Any()) -{ - <article class="bv-card bv-card--ok"> - <p>✓ No dead-letter messages detected. All consumers are healthy.</p> - </article> -} -else -{ - <p class="bv-muted"> - <strong>@deadLetters.Length</strong> consumer(s) have messages in dead-letter queues. - Auto-refreshes every 15 s. - </p> - - <div id="bv-dl-table" +} + +<div id="bv-dl-table" hx-get="/bus-viewer/partials/dead-letters" hx-trigger="load, every 15s" hx-swap="innerHTML"> - <p aria-busy="true">Loading…</p> - </div> -} + <p aria-busy="true">Loading…</p> +</div>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@SW.Bus.RabbitMqViewer/Areas/BusViewer/Pages/DeadLetters.cshtml` at line 8, The page currently calls DataService.GetDeadLetterSummaryAsync() into the deadLetters variable then lets the HTMX partial re-fetch the same data; either remove the initial fetch and render the HTMX container unconditionally, or use the fetched deadLetters as the model/parameter when rendering the partial so the partial can render immediately without a second fetch; locate the DataService.GetDeadLetterSummaryAsync() call and the deadLetters variable in DeadLetters.cshtml and either delete that call and show the HTMX container div always, or pass deadLetters into the HTMX partial render (replace the HTMX-only load with a partial render that supplies deadLetters).SW.Bus.RabbitMqViewer/Areas/BusViewer/Pages/Partials/DeadLetters.cshtml (1)
30-31: 💤 Low valueConsider extracting repeated severity CSS class logic.
The pattern
d.Severity.ToString().ToLower()is repeated twice on adjacent lines. While not a blocker, extracting to a local variable improves maintainability.♻️ Proposed refactor
`@foreach` (var d in dl) { - <tr class="bv-row--@d.Severity.ToString().ToLower()"> - <td><span class="bv-badge bv-badge--@d.Severity.ToString().ToLower()">@d.Severity</span></td> + @{ var severityCss = d.Severity.ToString().ToLower(); } + <tr class="bv-row--@severityCss"> + <td><span class="bv-badge bv-badge--@severityCss">@d.Severity</span></td>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@SW.Bus.RabbitMqViewer/Areas/BusViewer/Pages/Partials/DeadLetters.cshtml` around lines 30 - 31, Extract the repeated severity CSS expression into a local Razor variable to avoid duplicating d.Severity.ToString().ToLower(); inside the template: declare a local var (e.g., severityClass) near the top of the row rendering and assign severityClass = d.Severity.ToString().ToLower(); then replace both occurrences of d.Severity.ToString().ToLower() in the class attributes (bv-row-- and bv-badge--) with that variable and keep the displayed `@d.Severity` unchanged.SW.Bus.RabbitMqViewer/Areas/BusViewer/Pages/Partials/Retries.cshtml (1)
30-31: ⚡ Quick winUse
ToLowerInvariant()for CSS class generation.
ToString().ToLower()is culture-sensitive (e.g., Turkish locale will produce a dotlessi), which would yield broken CSS class names likebv-row--crıtıcal. PreferToLowerInvariant()here, and consider caching the result in a local since it's computed twice per row.♻️ Proposed refactor
`@foreach` (var r in retries) { - <tr class="bv-row--@r.Severity.ToString().ToLower()"> - <td><span class="bv-badge bv-badge--@r.Severity.ToString().ToLower()">@r.Severity</span></td> + var sev = r.Severity.ToString().ToLowerInvariant(); + <tr class="bv-row--@sev"> + <td><span class="bv-badge bv-badge--@sev">@r.Severity</span></td>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@SW.Bus.RabbitMqViewer/Areas/BusViewer/Pages/Partials/Retries.cshtml` around lines 30 - 31, Replace the culture-sensitive ToLower() calls used to generate CSS class names with ToLowerInvariant(), and compute it once per row into a local variable (e.g., var sev = r.Severity.ToString().ToLowerInvariant()) so both the tr class ("bv-row--" + sev) and the span class/text use that cached value; update usages around r.Severity in the template to reference the local and avoid duplicate ToString()/ToLowerInvariant() calls.SW.Bus.RabbitMqViewer/Areas/BusViewer/Pages/Index.cshtml (2)
87-88: ⚡ Quick winUse culture-invariant lowercasing for CSS class names.
Same issue as
Retries.cshtml:ToString().ToLower()is culture-sensitive and may break CSS class generation under non-invariant locales (Turkishi). PreferToLowerInvariant().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@SW.Bus.RabbitMqViewer/Areas/BusViewer/Pages/Index.cshtml` around lines 87 - 88, The CSS class generation uses culture-sensitive ToLower() on Severity string in Index.cshtml (the expression a.Severity.ToString().ToLower()), which can produce incorrect characters in some locales; update the rendering to call ToLowerInvariant() on the Severity string (i.e., replace the ToLower() call with ToLowerInvariant()) wherever a.Severity.ToString().ToLower() is used (including the class attribute and the badge text generation) to ensure culture-invariant lowercase CSS class names.
8-11: ⚡ Quick winParallelize independent dashboard fetches.
GetSummaryAsyncandGetConsumerHealthAsyncare independent and can run concurrently to halve dashboard load latency. Alerts depends on consumers and must remain sequential.♻️ Proposed refactor
- var summary = await DataService.GetSummaryAsync(); - var consumers = await DataService.GetConsumerHealthAsync(); - var alerts = await DataService.GetAlertsAsync(consumers); + var summaryTask = DataService.GetSummaryAsync(); + var consumersTask = DataService.GetConsumerHealthAsync(); + await Task.WhenAll(summaryTask, consumersTask); + var summary = summaryTask.Result; + var consumers = consumersTask.Result; + var alerts = await DataService.GetAlertsAsync(consumers);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@SW.Bus.RabbitMqViewer/Areas/BusViewer/Pages/Index.cshtml` around lines 8 - 11, GetSummaryAsync and GetConsumerHealthAsync are independent but currently awaited sequentially; start both calls as tasks (e.g., summaryTask = DataService.GetSummaryAsync(), consumersTask = DataService.GetConsumerHealthAsync()), await them together with Task.WhenAll, then read their results and call GetAlertsAsync(consumers) (alerts must remain after consumers). Update the code around DataService.GetSummaryAsync, DataService.GetConsumerHealthAsync and DataService.GetAlertsAsync to perform the concurrent fetch and keep alerts sequential.SW.Bus/ConsumersService.cs (2)
364-399: 🏗️ Heavy liftBackpressure check is one-shot, not continuous.
TryEmitQueueBackpressureis only called insideAttachConsumer, so aQueueBackpressureDetectedevent is emitted (at most) once when the consumer first attaches. If queue depth grows past the threshold during normal operation — which is the case the alert is meant to catch — no event will fire. Consider scheduling a periodic sample (e.g., a timer in the hosted service or piggybacking on the existing operational dispatcher) or sampling on every NthMessageProcessingCompleted.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@SW.Bus/ConsumersService.cs` around lines 364 - 399, TryEmitQueueBackpressure is only invoked from AttachConsumer so backpressure is only sampled once; make it run periodically or more frequently so alerts fire when depth grows after attach. Modify the consumer lifecycle to call TryEmitQueueBackpressure on a recurring basis (e.g., wire a timer in the hosted service or hook it into the existing operational dispatcher) or invoke it on every Nth MessageProcessingCompleted; ensure the scheduling logic is started when AttachConsumer registers the consumer and stopped when the consumer is detached, and keep emitting QueueBackpressureDetected as before when the threshold is exceeded.
179-200: 💤 Low valueShutdown event misses correlation context — consider populating
argsdetails.The handler passes
string.EmptyforMessageId/CorrelationId/CausationId/TraceId/SpanId, which is fine for a connection-level event. However,args(ShutdownEventArgs) carries useful diagnostic data —args.Initiator,args.ReplyCode,args.ClassId,args.MethodId— that would be valuable inDetailrather than justargs.ReplyText. Consider serializing a richer reason string.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@SW.Bus/ConsumersService.cs` around lines 179 - 200, The Shutdown handler for consumer (the consumer.Shutdown event) currently emits ConsumerDisconnected with empty MessageId/CorrelationId/CausationId/TraceId/SpanId and only uses args?.ReplyText for the final reason; update this to build a richer Detail/reason string by serializing relevant ShutdownEventArgs fields (e.g., args?.Initiator, args?.ReplyCode, args?.ClassId, args?.MethodId, args?.ReplyText) and pass that serialized string into the ConsumerDisconnected detail parameter (instead of string.Empty), so the emitted event contains useful diagnostic context; locate the consumer.Shutdown lambda and the ConsumerDisconnected constructor call to implement this change.SW.Bus/BasicPublisher.cs (3)
125-184: ⚡ Quick winActivity tags should be set before
BasicPublish, and on the failure path too.The
messaging.system/messaging.destination.name/messaging.operation/messaging.message.idtags are only attached on the success path (lines 127-130). WhenBasicPublishthrows, the exported span hasStatus=Error(good) but lacks the destination/system/operation tags, which makes the failed span hard to correlate with its destination in tracing back-ends. Move the tag assignments to beforeBasicPublish(or duplicate them in the catch).♻️ Proposed refactor
try { + activity?.SetTag("messaging.system", "rabbitmq"); + activity?.SetTag("messaging.destination.name", exchange); + activity?.SetTag("messaging.operation", "publish"); + activity?.SetTag("messaging.message.id", props.MessageId); model.BasicPublish(exchange, messageTypeName.ToLower(), props, message); stopwatch.Stop(); - activity?.SetTag("messaging.system", "rabbitmq"); - activity?.SetTag("messaging.destination.name", exchange); - activity?.SetTag("messaging.operation", "publish"); - activity?.SetTag("messaging.message.id", props.MessageId);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@SW.Bus/BasicPublisher.cs` around lines 125 - 184, The activity tags (calls to activity?.SetTag for "messaging.system", "messaging.destination.name", "messaging.operation", and "messaging.message.id") must be applied before calling model.BasicPublish so that spans for both success and failure include destination metadata; move the existing activity?.SetTag lines to immediately before the model.BasicPublish(...) call (or alternatively duplicate the same activity?.SetTag calls inside the catch (Exception ex) block) so failed publishes also export those tags—update references around BasicPublish, the activity?.SetTag calls, and the catch block accordingly.
64-186: 💤 Low value
Publishis declaredTask-returning but does no async work — make it synchronous orasync.The method body is fully synchronous (no
await), yet returnsTask.CompletedTaskat the end. This works but obscures the contract: callersawaitwhat is effectively a sync call, and the trailingreturn Task.CompletedTask;after atry/catch/finallyis dead code reachable only when the try succeeds. Either:
- mark the method
asyncand remove the trailing return, or- change the signature to
void/synchronous and let callers wrap as needed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@SW.Bus/BasicPublisher.cs` around lines 64 - 186, The Publish method is declared to return Task but contains no awaits; fix by making it truly asynchronous: add the async modifier to the Publish method signature (public async Task Publish(...)) and remove the trailing "return Task.CompletedTask;" so the method returns implicitly; keep the try/catch/finally as-is so exceptions still propagate and Activity disposal works. Reference: method Publish, variable props, and the existing try/catch/finally block.
91-99: ⚡ Quick winConsider using
DistributedContextPropagatorfor W3C traceparent propagation.While
props.Headers[OperationalEventEnvelope.TraceParentHeader] = activity.Id;works correctly in this codebase (targets .NET 8.0 where W3C format is the guaranteed default), usingDistributedContextPropagator.Current.Inject()is the recommended pattern for trace context propagation. It provides better future-proofing and follows the W3C Trace Context specification more explicitly, making the code's intent clearer to maintainers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@SW.Bus/BasicPublisher.cs` around lines 91 - 99, Replace the manual header assignments in the activity propagation block (the props.Headers[...] assignments and use of OperationalEventEnvelope.TraceParentHeader/TraceStateHeader/TraceIdHeader/SpanIdHeader/CausationIdHeader) with a call to DistributedContextPropagator.Current.Inject using the activity's Context (or Activity.Current.Context) and a simple setter that writes into props.Headers; for example call DistributedContextPropagator.Current.Inject(activity.Context, props.Headers, (headers, key, value) => headers[key] = value) and remove the manual TraceId/SpanId/CausationId/TraceParent/TraceState writes so propagation follows the W3C Trace Context pattern and stays future-proof.SW.Bus/InMemoryOperationalEventStore.cs (2)
31-41: 💤 Low valueLock-free write is racey under wrap-around — acceptable for observability, but document it.
Interlocked.Increment(ref _writeIndex)produces unique logical slots, but two writers whose logical indices map to the same physical slot via% _capacitycan interleave theirVolatile.Writecalls, andGetRecentmay then surface a "newer" event in what it interprets as an "older" position (or skip an event that was just overwritten). This is acceptable for an observability ring buffer, but the XML doc above (lines 9-13) only mentions overwrite of oldest events; it would be worth noting thatGetRecentreturns an eventually-consistent snapshot under high write contention so callers don't expect strict ordering.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@SW.Bus/InMemoryOperationalEventStore.cs` around lines 31 - 41, Update the XML documentation for the in-memory ring buffer to explicitly state that writes in PublishBatch use lock-free increments of _writeIndex and volatile writes into _buffer at index (_writeIndex % _capacity), and under wrap-around concurrent writers can interleave volatile writes so GetRecent may return an eventually-consistent snapshot (i.e., not strictly ordered and older slots can be briefly observed as newer or skipped); mention that overwrites of oldest events and eventual consistency under high write contention are expected behavior so callers should not rely on strict ordering or transactional guarantees.
44-63: 💤 Low value
GetRecentdoes not honorfilter.Limitfor negative or zero values.
var limit = filter?.Limit ?? 200;— if a caller passesLimit: 0, the loop'sif (results.Count >= limit) break;triggers immediately and returns an empty list, even though there are matching events. IfLimitis negative, the same happens. Consider clamping to a positive value (e.g.,Math.Max(1, filter?.Limit ?? 200)) or treating non-positive as "use default".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@SW.Bus/InMemoryOperationalEventStore.cs` around lines 44 - 63, GetRecent currently uses var limit = filter?.Limit ?? 200 which treats zero or negative Limits as valid and causes an immediate early return; change the logic in GetRecent to normalize/clamp filter?.Limit to a positive value (for example use Math.Max(1, filter?.Limit ?? 200) or treat non-positive as the default 200) before iterating the ring buffer, so the loop and the results.Count >= limit check behave correctly; update the variable named limit in GetRecent (used with _writeIndex, _capacity, Matches and results) accordingly.SW.Bus/ConsumerRunner.cs (1)
60-92: ⚖️ Poor tradeoffConsider extracting the operational-event payload boilerplate.
RunandRunNodeMessageconstruct each operational event with ~15 positional arguments, with most fields (machine name, env, app, exchange, queue, IDs, retry count) duplicated acrossMessageProcessingStarted/Completed/Failed/RetryScheduled/MovedToDeadLetter. Extracting a small helper (e.g.,BuildEventContext(consumerDefinition, ea, …)returning a struct or a delegate factory) would meaningfully reduce duplication and the risk of drift between event types when a field is added.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@SW.Bus/ConsumerRunner.cs` around lines 60 - 92, Extract the duplicated operational-event payload assembly into a single helper (e.g., BuildOperationalEventContext) that gathers common fields used across Run and RunNodeMessage: Environment.MachineName, busOptions.EnvironmentName, busOptions.ApplicationName, busOptions.ProcessExchange, consumerDefinition.QueueName, consumerDefinition.ServiceType?.Name, consumerDefinition.MessageTypeName, OperationalEventEnvelope.GetMessageId/GetCorrelationId/GetCausationId/GetTraceId/GetSpanId(ea.BasicProperties), ea.DeliveryTag, currentRetryCount, payloadSizeBytes and any activity trace/span values; return a small struct or factory delegate with those properties and helper methods to produce each event type. Replace the inline construction of MessageProcessingStarted (and the other event types MessageProcessingCompleted/Failed/RetryScheduled/MovedToDeadLetter) in Run and RunNodeMessage to use this helper to populate shared fields, keeping unique fields per event passed in locally. Ensure the helper references OperationalEventEnvelope, consumerDefinition, ea and busOptions so callers only supply event-specific values.README.md (3)
399-409: 💤 Low valueAdd language tag to fenced code block.
For better rendering and accessibility, tag this pipeline architecture diagram with a language identifier. Use
```textfor ASCII diagrams.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` around lines 399 - 409, Update the fenced ASCII diagram block so it includes a language tag for correct rendering and accessibility: replace the opening ``` with ```text for the diagram that begins with "Consumer / Publisher hot path" and contains the BoundedChannel<IOperationalEvent>, OperationalEventDispatcher, InMemoryOperationalEventStore, and IOperationalEventBatchSink lines; ensure only the opening fence is changed to ```text and the closing fence remains ``` so the diagram content is unchanged.
735-770: 💤 Low valueAdd language tag to fenced code block.
Tag this architecture diagram with
```textfor consistent rendering across markdown viewers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` around lines 735 - 770, The fenced ASCII diagram in README.md uses plain triple backticks without a language tag; update the opening fence from ``` to ```text so the block is marked as text (ensure only the opening fence is changed and the closing ``` remains), this will apply to the ASCII-art RabbitMQ Broker / SimplyWorks.Bus Runtime diagram block shown between the triple backticks.
774-785: 💤 Low valueAdd language tags to queue naming examples.
Both the convention block (line 774) and the example block (line 781) should use
```textfor consistent rendering.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` around lines 774 - 785, Change the fenced code blocks showing the queue naming convention and the example to use the text language tag so they render consistently; specifically update the block containing the pattern "{env}.{app}.{ConsumerClass}.{MessageType}" and the example block showing "v3.development.orderservice.ordercreatedconsumer.ordercreated" (and the .retry/.bad variants) to use ```text instead of ``` so both blocks render with the same language tag.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b02a794b-b6f0-4883-b186-7eac9865f3d8
📒 Files selected for processing (35)
README.mdSW.Bus.RabbitMqExtensions/BusDashboardContracts.csSW.Bus.RabbitMqExtensions/IConsumeExtended.csSW.Bus.RabbitMqExtensions/OperationalEvents.csSW.Bus.RabbitMqViewer/Areas/BusViewer/Pages/Consumers.cshtmlSW.Bus.RabbitMqViewer/Areas/BusViewer/Pages/DeadLetters.cshtmlSW.Bus.RabbitMqViewer/Areas/BusViewer/Pages/Events.cshtmlSW.Bus.RabbitMqViewer/Areas/BusViewer/Pages/Index.cshtmlSW.Bus.RabbitMqViewer/Areas/BusViewer/Pages/Partials/Alerts.cshtmlSW.Bus.RabbitMqViewer/Areas/BusViewer/Pages/Partials/Consumers.cshtmlSW.Bus.RabbitMqViewer/Areas/BusViewer/Pages/Partials/DeadLetters.cshtmlSW.Bus.RabbitMqViewer/Areas/BusViewer/Pages/Partials/Events.cshtmlSW.Bus.RabbitMqViewer/Areas/BusViewer/Pages/Partials/Queues.cshtmlSW.Bus.RabbitMqViewer/Areas/BusViewer/Pages/Partials/Retries.cshtmlSW.Bus.RabbitMqViewer/Areas/BusViewer/Pages/Queues.cshtmlSW.Bus.RabbitMqViewer/Areas/BusViewer/Pages/Retries.cshtmlSW.Bus.RabbitMqViewer/Areas/BusViewer/Pages/Shared/_AlertBanner.cshtmlSW.Bus.RabbitMqViewer/Areas/BusViewer/Pages/Shared/_Layout.cshtmlSW.Bus.RabbitMqViewer/Areas/BusViewer/Pages/_ViewImports.cshtmlSW.Bus.RabbitMqViewer/Areas/BusViewer/Pages/_ViewStart.cshtmlSW.Bus.RabbitMqViewer/Auth/BasicAuthFilter.csSW.Bus.RabbitMqViewer/BusViewerExtensions.csSW.Bus.RabbitMqViewer/SW.Bus.RabbitMqViewer.csprojSW.Bus.RabbitMqViewer/ViewerOptions.csSW.Bus.RabbitMqViewer/wwwroot/bus-viewer.cssSW.Bus.slnSW.Bus/AlertEvaluator.csSW.Bus/BasicPublisher.csSW.Bus/BusDashboardDataService.csSW.Bus/BusOptions.csSW.Bus/ConsumerRunner.csSW.Bus/ConsumersService.csSW.Bus/IServiceCollectionExtensions.csSW.Bus/InMemoryOperationalEventStore.csSW.Bus/OperationalEventInfrastructure.cs
| <link rel="stylesheet" href="https://cdn.jsdelivr.net/npm/@@picocss/pico@@2/css/pico.slate.min.css"/> | ||
| <link rel="stylesheet" href="/_content/SimplyWorks.Bus.RabbitMqViewer/bus-viewer.css"/> | ||
| <script src="https://unpkg.com/htmx.org@1.9.12" defer></script> |
There was a problem hiding this comment.
Add Subresource Integrity (SRI) to CDN resources.
Loading CSS and JavaScript from external CDNs without SRI hashes exposes the application to supply-chain attacks if the CDN is compromised or serves malicious content.
🔒 Proposed fix with SRI hashes
- <link rel="stylesheet" href="https://cdn.jsdelivr.net/npm/@@picocss/pico@@2/css/pico.slate.min.css"/>
+ <link rel="stylesheet" href="https://cdn.jsdelivr.net/npm/@picocss/pico@2/css/pico.slate.min.css"
+ integrity="sha384-..." crossorigin="anonymous"/>
<link rel="stylesheet" href="/_content/SimplyWorks.Bus.RabbitMqViewer/bus-viewer.css"/>
- <script src="https://unpkg.com/htmx.org@1.9.12" defer></script>
+ <script src="https://unpkg.com/htmx.org@1.9.12"
+ integrity="sha384-..." crossorigin="anonymous" defer></script>Generate SRI hashes using:
curl -s https://cdn.jsdelivr.net/npm/@picocss/pico@2/css/pico.slate.min.css | openssl dgst -sha384 -binary | openssl base64 -A
curl -s https://unpkg.com/htmx.org@1.9.12 | openssl dgst -sha384 -binary | openssl base64 -A🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@SW.Bus.RabbitMqViewer/Areas/BusViewer/Pages/Shared/_Layout.cshtml` around
lines 10 - 12, Add Subresource Integrity (SRI) to the external CDN assets by
generating sha384 SRI hashes for the two referenced resources
("https://cdn.jsdelivr.net/npm/@picocss/pico@2/css/pico.slate.min.css" and
"https://unpkg.com/htmx.org@1.9.12") and update the corresponding <link> and
<script> tags to include integrity="sha384-..." and crossorigin="anonymous";
ensure the integrity values match the generated base64 sha384 digests and keep
the existing href/src values unchanged so the <link rel="stylesheet"
href="https://cdn.jsdelivr.net/npm/@picocss/pico@2/css/pico.slate.min.css"/> and
<script src="https://unpkg.com/htmx.org@1.9.12" defer></script> lines are
replaced with versions that include both integrity and crossorigin attributes.
…lowAnonymous attribute
…ng and causation ID logic
2 participants
Summary by CodeRabbit
Release Notes
New Features
Documentation