Add EF Core-backed failed message queries - #5671
Conversation
9d1bc40 to
aa1a6fd
Compare
rbev
left a comment
rbev
left a comment
There was a problem hiding this comment.
Approved with a few suggestions
| static readonly string[] PayloadColumns = | ||
| [ | ||
| "MessageId", "MessageType", "TimeSent", "ConversationId", "QueueAddress", | ||
| "MessageId", "MessageType", "TimeSent", "ConversationId", "FailingEndpointAddress", |
There was a problem hiding this comment.
I notice that FailingEndpointAddress was added in a different PR. If this list having correct coverage of the entity is important there should be a test added for it.
| .AsNoTracking() | ||
| .FilterByStatus(status) | ||
| .FilterByLastModifiedRange(modified) | ||
| .FilterByQueueAddress(queueAddress) |
There was a problem hiding this comment.
Does this need to be added to the Status, Modified index?
There was a problem hiding this comment.
FailingEndpointAddress already has its own index, but the ToLower() below stops it being used, so adding the column to the composite wouldn't buy anything until that's resolved. See #5671 (comment)
| public Task<QueryStatsInfo> GetFailedMessagesStats(string? status, string? modified, string? queueAddress) => | ||
| ExecuteWithDbContext(dbContext => dbContext.FailedMessages | ||
| .AsNoTracking() | ||
| .FilterByStatus(status) |
There was a problem hiding this comment.
When status is null I'm pretty sure this will miss the index as the Where doesn't get added
There was a problem hiding this comment.
I looked at this and it seems the only place this method is called is from the
which is never called from ServicePulse so I am hesitant in adding another index to the hot path ingestion table!|
|
||
| continue; | ||
| } | ||
|
|
||
| if (Enum.TryParse<FailedMessageStatus>(filter, true, out var included)) | ||
| { | ||
| includes.Add(included); | ||
| } |
There was a problem hiding this comment.
else instead of continue might read better.
| continue; | |
| } | |
| if (Enum.TryParse<FailedMessageStatus>(filter, true, out var included)) | |
| { | |
| includes.Add(included); | |
| } | |
| } | |
| else | |
| { | |
| if (Enum.TryParse<FailedMessageStatus>(filter, true, out var included)) | |
| { | |
| includes.Add(included); | |
| } | |
| } |
|
|
||
| foreach (var exclude in excludes) | ||
| { | ||
| // Captured per iteration so each exclusion closes over its own value. |
There was a problem hiding this comment.
Could it just be !excludes.Contains(message.Status) instead of this?
|
|
||
| var address = queueAddress.ToLowerInvariant(); | ||
|
|
||
| // The ToLower() here causes a full table scan! |
There was a problem hiding this comment.
queueAddress only arrives from the Pending Retries screen, which does poll every 5 seconds, so this isn't a cold path. But the predicate always runs after the status and modified filters, so it's a residual test over retry-issued messages in the selected range rather than a scan of the whole table, and that set is normally small. Not worth a schema change on that evidence. Worth noting the ToLower() is redundant on SQL Server anyway, since the default collation is already case-insensitive, and only PostgreSQL genuinely needs it, so if this ever does show up in a profile the fix is a normalised column rather than a new index.
| var descending = sortInfo?.Direction != "asc"; | ||
| var sort = sortInfo?.Sort; | ||
|
|
||
| if (sort == null || !SortInfo.AllowedSortOptions.Contains(sort)) |
There was a problem hiding this comment.
is this check redundant given the fallback in the switch statement?
| Assert.That(row.TimeSent, Is.EqualTo(failure.TimeSent)); | ||
| Assert.That(row.ConversationId, Is.EqualTo(failure.ConversationId)); | ||
| Assert.That(row.QueueAddress, Is.EqualTo(failure.QueueAddress)); | ||
| Assert.That(row.FailingEndpointAddress, Is.EqualTo(failure.QueueAddress)); |
| throw new NotImplementedException(); | ||
| return (IDictionary<string, object>)new Dictionary<string, object> | ||
| { | ||
| ["Endpoints"] = endpoints, |
There was a problem hiding this comment.
Where do these magic strings come from?
Are they magic strings on the raven side too?
There was a problem hiding this comment.
Yes they are, I have made them more explicit and sharing between Raven and EF
Implements IFailedMessageQueryDataStore for the EF Core persisters, covering the failed message lists, the summary and the single-message lookups. Exception source and stack trace, and the Edited and EditOf markers, are read from the stored headers rather than duplicated into columns.
2c79e96 to
ab1a1b5
Compare
2 participants
Implements IFailedMessageQueryDataStore for the EF Core persisters, covering the failed message lists, the summary and the single-message lookups. Exception source and stack trace, and the Edited and EditOf markers, are read from the stored headers rather than duplicated into columns.