POL-293 Add paging to the history of a single policy #119

pilhuhn · 2020-06-09T16:16:40Z

Signed-off-by: Heiko W. Rupp [email protected]

Signed-off-by: Heiko W. Rupp <[email protected]>

josejulio · 2020-06-09T17:26:54Z

src/main/java/com/redhat/cloud/policies/app/rest/PolicyCrudService.java

+         limit = min(limit,200);
+         int pageNum = pager.getOffset() / limit;
+
+         String alerts = engine.findLastTriggered(policyId.toString(), false, pageNum, limit,


I'll need a "paginated response", e.g. to know the total number of elements to decide if "next page" button is disabled or not.

Yes. I am not sure yet how to get that from the engine

josejulio · 2020-06-09T18:02:44Z

Something else to note, is that the UI would still need all the elements in the page for exporting (json/csv), should the UI consume all the pages to build this report?

josejulio · 2020-06-09T18:14:38Z

Some test, not sure if i'm wrongly fetching the second page

Page 1 with 50 elements (limit=50 and offset=0)

curl 'https://ci.foo.redhat.com:1337/api/policies/v1.0/policies/3f421b5c-babc-4cf4-9026-0b9083d5b1df/history/trigger?limit=50&offset=0' | jq ". | length"
50

Page 1 with 200 elements (limit=200 and offset=0)

curl 'https://ci.foo.redhat.com:1337/api/policies/v1.0/policies/3f421b5c-babc-4cf4-9026-0b9083d5b1df/history/trigger?limit=200&offset=0' | jq ". | length"
200

but

Page 2 with 50 elements (limit=50 and offset=50)

curl 'https://ci.foo.redhat.com:1337/api/policies/v1.0/policies/3f421b5c-babc-4cf4-9026-0b9083d5b1df/history/trigger?limit=50&offset=50' | jq ". | length"
0

and

Page 3 with 50 elements (limit=50 and offset=100)

curl 'https://ci.foo.redhat.com:1337/api/policies/v1.0/policies/3f421b5c-babc-4cf4-9026-0b9083d5b1df/history/trigger?limit=50&offset=100' | jq ". | length"
50

From there, all the pages seems to yield correctly

josejulio · 2020-06-09T18:26:58Z

@burmanm maybe a bug from the engine?
I see that the following arguments are being sent to the engine:

@GET
  @Consumes("application/json")
  String findLastTriggered(@QueryParam("triggerIds") String triggerIds,
                                @QueryParam("thin") boolean thin,
                                @QueryParam("page") int page,
                                @QueryParam("per_page") int per_page,
                                @HeaderParam("Hawkular-Tenant") String customerId);

engine.findLastTriggered("the-id", false, 1, 50, "the-account")

and the engine returns []

josejulio · 2020-06-09T19:50:15Z

I updated the engine, and now I'm getting [] always, but I can see "Last Triggered" date,maybe related to RedHatInsights/policies-engine#80 ?

burmanm · 2020-06-09T20:01:36Z

I don't see that [] in the master for all queries. Thin alerts shouldn't make a difference to this, but I'll have to investigate the apparent bug (maybe there's a double filtering done - so Infinispan + code and that causes the offsets to go wrong).

josejulio · 2020-06-09T20:07:52Z

Debugging the backend code, it looks like something changed on the engine, it is returning (for first page of 2 elements):

[
   {
      "eventType":"ALERT",
      "tenantId":"940527",
      "id":"fe5a82ed-129c-4ce0-8e3b-63ab4aac045e-1591732136644-2243f530-f544-410b-bb4c-3d6090f65cc4",
      "ctime":1591732136644,
      "dataSource":"_none_",
      "dataId":"fe5a82ed-129c-4ce0-8e3b-63ab4aac045e",
      "category":"ALERT",
      "text":"Fail if we are running on a x86_64 - We should be using arm.",
      "trigger":{
         "tenantId":"940527",
         "id":"fe5a82ed-129c-4ce0-8e3b-63ab4aac045e",
         "name":"Copy of Not arch x86_64",
         "description":"Fail if we are running on a x86_64 - We should be using arm.",
         "type":"STANDARD",
         "eventType":"ALERT",
         "eventCategory":null,
         "eventText":null,
         "severity":"MEDIUM",
         "actions":[
            {
               "tenantId":"940527",
               "actionPlugin":"email",
               "actionId":"_managed-instance-email-35e48b6487d9568e"
            }
         ],
         "autoDisable":false,
         "autoEnable":false,
         "autoResolve":false,
         "autoResolveAlerts":false,
         "autoResolveMatch":"ALL",
         "enabled":true,
         "firingMatch":"ALL",
         "source":"_none_"
      },
      "severity":"MEDIUM",
      "status":"OPEN",
      "lifecycle":[
         {
            "status":"OPEN",
            "stime":1591732136644
         }
      ]
   },
   {
      "eventType":"ALERT",
      "tenantId":"940527",
      "id":"fe5a82ed-129c-4ce0-8e3b-63ab4aac045e-1591732136583-20d4a793-fa41-4321-9f6d-006e4d062cfc",
      "ctime":1591732136583,
      "dataSource":"_none_",
      "dataId":"fe5a82ed-129c-4ce0-8e3b-63ab4aac045e",
      "category":"ALERT",
      "text":"Fail if we are running on a x86_64 - We should be using arm.",
      "trigger":{
         "tenantId":"940527",
         "id":"fe5a82ed-129c-4ce0-8e3b-63ab4aac045e",
         "name":"Copy of Not arch x86_64",
         "description":"Fail if we are running on a x86_64 - We should be using arm.",
         "type":"STANDARD",
         "eventType":"ALERT",
         "eventCategory":null,
         "eventText":null,
         "severity":"MEDIUM",
         "actions":[
            {
               "tenantId":"940527",
               "actionPlugin":"email",
               "actionId":"_managed-instance-email-35e48b6487d9568e"
            }
         ],
         "autoDisable":false,
         "autoEnable":false,
         "autoResolve":false,
         "autoResolveAlerts":false,
         "autoResolveMatch":"ALL",
         "enabled":true,
         "firingMatch":"ALL",
         "source":"_none_"
      },
      "severity":"MEDIUM",
      "status":"OPEN",
      "lifecycle":[
         {
            "status":"OPEN",
            "stime":1591732136583
         }
      ]
   }
]

and the backend query "$.[*].evalSets..value" is the one returning [] after update.

burmanm · 2020-06-09T20:25:14Z

Well, the backend is now removing evalsets from alerts storage if that is set (in the application.properties), so searching them would be impossible.

josejulio · 2020-06-09T20:50:59Z

Ok, when setting engine.backend.ispn.alerts-thin to false it returns the evalSet as did before.

josejulio · 2020-06-09T21:02:40Z

What i said above (#119 (comment)) is still an issue.

Using pages of 50 elements:

If I have 99 elements, page 1 and page 2 show correctly.
When I have 100 elements, the page 2 yields no elements.
Having 102 elements page 1 shows OK, page 2 empty and page 3 shows 2 elements.

pilhuhn · 2020-06-10T07:37:43Z

Something else to note, is that the UI would still need all the elements in the page for exporting (json/csv), should the UI consume all the pages to build this report?

I think the UI would need to iterate over the pages. Sending with unlimited could be much too much data that is dealt with in one go.

burmanm · 2020-06-10T09:06:26Z

Something else to note, is that the UI would still need all the elements in the page for exporting (json/csv), should the UI consume all the pages to build this report?

If there's export functionality, then it should probably just stream the data from engine? No point keeping any of this in memory elsewhere.

Signed-off-by: Heiko W. Rupp <[email protected]>

josejulio · 2020-06-15T17:06:02Z

src/main/java/com/redhat/cloud/policies/app/rest/PolicyCrudService.java

@@ -782,26 +784,51 @@ public Response getPolicy(@PathParam("id") UUID policyId) {

  @Operation(summary = "Retrieve the trigger history of a single policy")
  @APIResponse(responseCode = "200", description = "History could be retrieved",
-      content = @Content (schema = @Schema(type = SchemaType.ARRAY, implementation = HistoryItem.class)))
+      content = @Content (schema = @Schema(implementation = PagingUtils.PagedResponse.class)),


I have a problem here, using this syntax removes the HistoryItem from the openapi.json plus the PagedResponse doesn't have any info about its contents.

I found the following [1] workaround:

// workaround for returning generic types: https://github.com/swagger-api/swagger-core/issues/498#issuecomment-74510379 // This class is used only for swagger return type private static class PagedResponseOfHistoryItem extends PagingUtils.PagedResponse<HistoryItem> { private List<HistoryItem> data; private PagedResponseOfHistoryItem() {} } @Operation(summary = "Retrieve the trigger history of a single policy") @APIResponse(responseCode = "200", description = "History could be retrieved", content = @Content (schema = @Schema(implementation = PagedResponseOfHistoryItem.class)),

WDYT?

[1] swagger-api/swagger-core#498 (comment)

I was thinking about something similar, as I don't like the type-erasure.
Same for getPolicies

Change openapi types to not erase the inner type Signed-off-by: Heiko W. Rupp <[email protected]>

pilhuhn · 2020-06-16T16:19:44Z

This depends now on RedHatInsights/policies-engine#82

josejulio · 2020-06-17T21:50:17Z

Tested this PR, is working fine.

Openapi has the types for the paged policy and paged triggers.
Paginated response is good

Already iterating over the pages when exporting.

Signed-off-by: Heiko W. Rupp <[email protected]>

POL-293 Add paging to the history of a single policy

52e5fe6

Signed-off-by: Heiko W. Rupp <[email protected]>

pilhuhn requested a review from josejulio June 9, 2020 16:16

josejulio reviewed Jun 9, 2020

View reviewed changes

pilhuhn added 2 commits June 10, 2020 16:57

Turn into paged list.

73ad09b

Signed-off-by: Heiko W. Rupp <[email protected]>

Fix test

8b092f7

Signed-off-by: Heiko W. Rupp <[email protected]>

josejulio reviewed Jun 15, 2020

View reviewed changes

Fetch total count from engine.

1b12dc5

Change openapi types to not erase the inner type Signed-off-by: Heiko W. Rupp <[email protected]>

josejulio mentioned this pull request Jun 17, 2020

POL-293 Backend paging for trigger history RedHatInsights/policies-ui-frontend#165

Merged

Adjust tests.

6de5a8f

Signed-off-by: Heiko W. Rupp <[email protected]>

pilhuhn marked this pull request as ready for review June 18, 2020 08:33

pilhuhn requested a review from josejulio June 18, 2020 08:33

josejulio approved these changes Jun 18, 2020

View reviewed changes

josejulio merged commit ebf1dfa into RedHatInsights:master Jun 18, 2020

POL-293 Add paging to the history of a single policy #119

POL-293 Add paging to the history of a single policy #119

Uh oh!

Conversation

pilhuhn commented Jun 9, 2020

Uh oh!

josejulio Jun 9, 2020

Choose a reason for hiding this comment

Uh oh!

pilhuhn Jun 10, 2020

Choose a reason for hiding this comment

Uh oh!

josejulio commented Jun 9, 2020

Uh oh!

josejulio commented Jun 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

josejulio commented Jun 9, 2020

Uh oh!

josejulio commented Jun 9, 2020

Uh oh!

burmanm commented Jun 9, 2020

Uh oh!

josejulio commented Jun 9, 2020

Uh oh!

burmanm commented Jun 9, 2020

Uh oh!

josejulio commented Jun 9, 2020

Uh oh!

josejulio commented Jun 9, 2020

Uh oh!

pilhuhn commented Jun 10, 2020

Uh oh!

burmanm commented Jun 10, 2020

Uh oh!

josejulio Jun 15, 2020

Choose a reason for hiding this comment

Uh oh!

pilhuhn Jun 15, 2020

Choose a reason for hiding this comment

Uh oh!

pilhuhn commented Jun 16, 2020

Uh oh!

josejulio commented Jun 17, 2020

Uh oh!

Uh oh!

josejulio commented Jun 9, 2020 •

edited

Loading