New endpoint to support migration of SI to SP #4918

johnsimons · 2025-03-31T23:43:13Z

As part of the ongoing work to migrate SI to SP, we identified that paging in SI does not return the correct results once the user reaches page 2 or higher. The only correct page is page 1.

The TF working on this has evaluated whether paging is needed at all for the screens in SP and decided that, to provide accurate results, we are not migrating paging, given the issues.

This PR introduces a new endpoint that:

returns only the top x messages
supports filtering the messages by time sent range
fixed a bug where the results could return more than the page size limit

ramonsmits

Can you explain why this new endpoint it needed? Second, why is the filtering needed on "time sent"? This to be feels more sorting/seeking requirement but that could conflicting with SortInfo.

As each index uses a different type of ordering we need to "seek" using the ordered index attributes.

This is a new API and the sorting/seeking seems incorrect.

src/ServiceControl.Audit/Auditing/MessagesView/GetMessagesController.cs

src/ServiceControl.Audit/Auditing/MessagesView/GetMessages2Controller.cs

src/ServiceControl.Audit.Persistence/IAuditDataStore.cs

src/ServiceControl.Audit.Persistence.RavenDB/Extensions/RavenQueryExtensions.cs

ramonsmits · 2025-04-01T08:15:10Z

src/ServiceControl/CompositeViews/Messages/ScatterGatherApiMessageView.cs

            }

-            return combined;
+            return combined.Take(input.PagingInfo.PageSize).ToList();


This is a fix right? Better to isolated that in a separate PR.

I don't think this is a fix to the existing behaviour - this is what's required by the new behaviour (getting first x results only)

That being said, for old versions of SP that still use the old API, this is going to result in even more broken behaviour where they can't even see large chunks of messages. It would make sense to have this behaviour conditional on the new API only

I thought of that @PhilBastian, but I don't understand why you think it would be worse in SI, we would only send the correct amount of records to it, how does that break it even further?

Happy to toggle it only conditionally though

current behaviour on a 50 page size

first page: SGa 1-50 + SGb 1-50

second page: SGa 50-100 + SGb 50-100
this solution

first page: SGa 1-25 + SGb 1-25 (approx, depending on sort)

second page: SGa 50-75 + SGb 50-75.
The user will never see 25-50 from either of the ScatterGather endpoints

That is not the way it works.
We still return 50 results from all the instances, we then merge them all together and sort them and return the top 50.

src/ServiceControl/CompositeViews/Messages/GetMessages2Controller.cs

Also fixed a bug where the results could return more than the page size

Changes time range from string to DateTimeRange object for message queries. This provides type safety and removes parsing logic and error handling from the data access layer. Also allows for optional To property on the DateTimeRange.

PhilBastian · 2025-04-08T06:07:34Z

src/ServiceControl.Audit.Persistence/Infrastructure/DateTimeRange.cs

+
+    public DateTimeRange(string from = null, string to = null)
+    {
+        if (from != null)


exception handling for invalid date string?

Exceptions in REST apis are hard, they don't really bubble to the UI,
So not sure what can we do better?

The point is that this is currently throwing an exception. At least ignore the value if it's not valid so that the client still receives data

But then they don't know they made a mistake, and they would get unexpected results.
This is a tricky topic that, unfortunately, our REST API does not handle well.
So, I am cautious about introducing a new pattern that is not well thought out.

PhilBastian · 2025-04-08T06:34:48Z

src/ServiceControl.Persistence/Infrastructure/DateTimeRange.cs

+
+    public DateTimeRange(string from = null, string to = null)
+    {
+        if (from != null)


exception handling for invalid date string?

PhilBastian · 2025-04-08T06:37:33Z

src/ServiceControl/CompositeViews/Messages/ScatterGatherApiMessageView.cs

            }

-            return combined;
+            return combined.Take(input.PagingInfo.PageSize).ToList();


I don't think this is a fix to the existing behaviour - this is what's required by the new behaviour (getting first x results only)

That being said, for old versions of SP that still use the old API, this is going to result in even more broken behaviour where they can't even see large chunks of messages. It would make sense to have this behaviour conditional on the new API only

johnsimons · 2025-04-09T04:30:24Z

@ramonsmits can you unblock this PR, so we can merge it?

ramonsmits · 2025-04-09T09:25:15Z

src/ServiceControl.Audit/Auditing/MessagesView/GetMessages2Controller.cs

+[ApiController]
+[Route("api")]
+public class GetMessages2Controller(IAuditDataStore dataStore) : ControllerBase


Wouldn't it be possible to use ApiVersionAttribute by using the Microsoft.AspNetCore.Mvc.Versioning package?

This is just pseudo typing but it would then become something like:

Suggested change

[ApiController]

[Route("api")]

public class GetMessages2Controller(IAuditDataStore dataStore) : ControllerBase

[ApiController]

[Route("api")]

[ApiVersion("2.0")]

public class GetMessages2Controller(IAuditDataStore dataStore) : ControllerBase

This also allows for a more clean usage of the deprecated API later on.

Not in this PR

@ramonsmits I am not too concerned with api versioning given that we control the clients and this is not a publicly documented api so we can evolve it anyway we want as long our clients do not break.
So if you feel strongly about api versioning we can have that discussion separately from this work.
But otherwise can you unblock the PR so we can proceed with the port of SI to SP.

Ramon, I am unblocking this PR

johnsimons self-assigned this Mar 31, 2025

ramonsmits previously requested changes Apr 1, 2025

View reviewed changes

johnsimons changed the title ~~New endpoint to return top x messages list~~ New endpoint to support migration of SI to SP Apr 2, 2025

johnsimons force-pushed the john/sp_end branch 4 times, most recently from 859397d to 39657d1 Compare April 2, 2025 05:07

johnsimons added 3 commits April 8, 2025 12:04

Adding time sent range api

ab91a77

Also fixed a bug where the results could return more than the page size

Updates time range to DateTimeRange object

19f662f

Changes time range from string to DateTimeRange object for message queries. This provides type safety and removes parsing logic and error handling from the data access layer. Also allows for optional To property on the DateTimeRange.

Updated approval files

cc8183b

johnsimons force-pushed the john/sp_end branch from 39657d1 to cc8183b Compare April 8, 2025 02:09

johnsimons requested a review from PhilBastian April 8, 2025 02:09

Add missing paging info

ed60f9f

PhilBastian approved these changes Apr 8, 2025

View reviewed changes

johnsimons requested a review from ramonsmits April 8, 2025 07:49

ramonsmits reviewed Apr 9, 2025

View reviewed changes

johnsimons enabled auto-merge April 9, 2025 10:43

johnsimons merged commit 9b09977 into master Apr 9, 2025
32 checks passed

johnsimons deleted the john/sp_end branch April 9, 2025 22:50

New endpoint to support migration of SI to SP #4918

New endpoint to support migration of SI to SP #4918

Uh oh!

Conversation

johnsimons commented Mar 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ramonsmits left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johnsimons Apr 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johnsimons commented Apr 9, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

johnsimons commented Mar 31, 2025 •

edited

Loading

johnsimons Apr 8, 2025 •

edited

Loading